ThorsSQL Lib: Part 3: Layer 1

Question

Have a working version of MySQL implementation of ThorsSQL library done.

If you want to check it out you can find the whole thing on github ThorsSQL.

This is a follow on to previous code Reviews:
Part 2
Part 1

The documentation for these classes is here:

Part 3 (Layer 1): The MySQL Implementation

Layer 1: Simple Stream

The lowest layer is the stream. This is simply an open bidirectional unix socket to the server. This is implemented by the MySQLStream class which implements the Interface PackageStream. By providing this layer of abstraction I can mock out the stream object for testing; thus allowing the testing framework to replay with a specific stream of bytes without actually needing to use a socket.

Class:
==========
MySQLStream: PackageStream
Interface:
==========
PackageStream

PackageStream.h

#ifndef THORS_ANVIL_MYSQL_PACKAGE_STREAM_H
#define THORS_ANVIL_MYSQL_PACKAGE_STREAM_H
#include <string>
#include <cstddef>
namespace ThorsAnvil
{
 namespace MySQL
 {
class PackageStream
{
 public:
 virtual ~PackageStream() = 0;
 virtual void read(char* buffer, std::size_t len) = 0;
 virtual void write(char const* buffer, std::size_t len) = 0;
 virtual void startNewConversation() = 0;
 virtual void flush() = 0;
 virtual void reset() = 0;
 virtual void drop() = 0;
 virtual bool isEmpty() = 0;
 virtual std::string readRemainingData() = 0;
};
 }
}
#endif

MySQLStream.h

#ifndef THORS_ANVIL_MYSQL_MY_SQL_STREAM_H
#define THORS_ANVIL_MYSQL_MY_SQL_STREAM_H
#include "PackageStream.h"
#include <string>
#include <cstddef>
namespace ThorsAnvil
{
 namespace MySQL
 {
class MySQLStream: public PackageStream
{
 static std::size_t constexpr ErrorResult = static_cast<std::size_t>(-1);
 int socket;
 public:
 MySQLStream(std::string const& host, int port);
 MySQLStream(int socket);
 ~MySQLStream();
 virtual void read(char* buffer, std::size_t len) override;
 virtual void write(char const* buffer, std::size_t len) override;
 virtual void startNewConversation() override {}
 virtual void flush() override {}
 virtual void reset() override {}
 virtual void drop() override {}
 virtual bool isEmpty() override {return true;}
 virtual std::string readRemainingData() override {return "";}
};
 }
}
#endif

MySQLStream.cpp

#include "MySQLStream.h"
#include "ThorSQL/SQLUtil.h"
#include <stdexcept>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/uio.h>
#include <netdb.h>
#include <unistd.h>
#include <errno.h>
using namespace ThorsAnvil::MySQL;
MySQLStream::MySQLStream(int socket)
 : socket(socket)
{}
MySQLStream::MySQLStream(std::string const& host, int port)
{
 port = port ? port : 3306;
 sockaddr_in serv_addr;
 memset(&serv_addr, '0', sizeof(serv_addr));
 serv_addr.sin_family = AF_INET;
 serv_addr.sin_port = htons(port);
 hostent* serv = ::gethostbyname(host.c_str());
 if (serv == NULL)
 {
 throw std::runtime_error(
 errorMsg("ThorsAnvil::MySQL::MySQLStream::MySQLStream: ",
 "::gethostbyname() Failed: ", strerror(errno)
 ));
 }
 bcopy((char *)serv->h_addr, (char *)&serv_addr.sin_addr.s_addr, serv->h_length);
 if ((socket = ::socket(AF_INET, SOCK_STREAM, 0)) < 0)
 {
 throw std::runtime_error(
 errorMsg("ThrosAnvil::MySQL::MySQLStream::MySQLStream: ",
 "::socket() Failed: ", strerror(errno)
 ));
 }
 using SockAddr = struct sockaddr;
 if (::connect(socket, reinterpret_cast<SockAddr*>(&serv_addr), sizeof(serv_addr)) < 0)
 {
 ::close(socket);
 throw std::runtime_error(
 errorMsg("ThorsAnvil::MySQL::MySQLStream::MySQLStream: ",
 "::connect() Failed: ", strerror(errno)
 ));
 }
}
MySQLStream::~MySQLStream()
{
 ::close(socket);
}
void MySQLStream::read(char* buffer, std::size_t len)
{
 std::size_t readSoFar = 0;
 while (readSoFar != len)
 {
 std::size_t read = ::read(socket, buffer + readSoFar, len - readSoFar);
 if ((read == ErrorResult) && (errno == EAGAIN || errno == EINTR))
 {
 /* Recoverable error. Try again. */
 continue;
 }
 else if (read == 0)
 {
 throw std::runtime_error(
 errorMsg("ThorsAnvil::MySQL::MySQLStream::read: "
 "::read() Failed: ",
 "Tried to read ", len, "bytes but only found ", readSoFar, " before EOF"
 ));
 }
 else if (read == ErrorResult)
 {
 throw std::runtime_error(
 errorMsg("ThorsAnvil::MySQL::MySQLStream::read: ",
 "::read() Failed: ",
 "errno=", errno, " Message=", strerror(errno)
 ));
 }
 readSoFar += read;
 }
}
void MySQLStream::write(char const* buffer, std::size_t len)
{
 std::size_t writenSoFar = 0;
 while (writenSoFar != len)
 {
 std::size_t writen = ::write(socket, buffer + writenSoFar, len - writenSoFar);
 if ((writen == ErrorResult) && (errno == EAGAIN || errno == EINTR))
 {
 /* Recoverable error. Try again. */
 continue;
 }
 else if (writen == 0)
 {
 throw std::runtime_error(
 errorMsg("ThorsAnvil::MySQL::MySQLStream::write: ",
 "::write() Failed: ",
 "Tried to write ", len, "bytes but only found ", writenSoFar, " before EOF"
 ));
 }
 else if (writen == ErrorResult)
 {
 throw std::runtime_error(
 errorMsg("ThorsAnvil::MySQL::MySQLStream::write: ",
 "::write() Failed: ",
 "errno=", errno, " Message=", strerror(errno)
 ));
 }
 writenSoFar += writen;
 }
}

test/MockStream.h

#include <sstream>
#include <stdexcept>
class MockStream: public ThorsAnvil::MySQL::PackageStream
{
 char const* input;
 unsigned char*output;
 std::size_t len;
 std::size_t readSoFar;
 std::size_t writSoFar;
 public:
 MockStream(char const* data, std::size_t len, unsigned char* output = nullptr)
 : input(data)
 , output(output)
 , len(len)
 , readSoFar(0)
 , writSoFar(0)
 {
 }
 virtual void read(char* buffer, std::size_t size) override {if (readSoFar + size > len) {
 std::stringstream msg;
 msg << "Read too much: readSoFar(" << readSoFar << ") Size(" << size << ") Len(" << len << ")";
 throw std::runtime_error(msg.str());
 }
 std::copy(input + readSoFar, input + readSoFar + size, buffer);readSoFar += size;
 }
 virtual void write(char const* buffer, std::size_t len) override {std::copy(buffer, buffer + len, output + writSoFar); writSoFar += len;}
 virtual void startNewConversation() override {}
 virtual void flush() override {}
 virtual void drop() override {readSoFar = len;}
 virtual void reset() override {}
 virtual bool isEmpty() override {return len == readSoFar;}
 virtual std::string readRemainingData() override {return std::string(input + readSoFar, input + readSoFar + len);}
 std::size_t readLen() const {return readSoFar;}
 std::size_t writLen() const {return writSoFar;}
};

• Maikel: Is there a reason to not use any libraries like gsl for byte and span or boost asio? This is a prime example where both would shine. I searched your docs but couldn't find a motivation to not use any libraries.
• D. Jurcau: My I ask what the reason is for reimplemeting the mysql communication protocol?
• Loki Astari: @Maikel: I did not add GSL support yet. But that was mainly because I don't want to force its requirement. I plan to add an optional GSL extension in the future.
• Loki Astari: @Maikel: Why did I not use boost asio. I am still on the fence about boost:asio; I think it overcomplicates things that could be simpler. Also with C++17 and co-routines a better alternative to asio should be easier to write and I am looking for that library.
• Loki Astari: @D.Jurcau: Because I did not want to simply wrap the C API layer (and add the associated overhead).

Answer 1

Design

class MySQLStream: public PackageStream {
 //...
 virtual void startNewConversation() override {}
 virtual void flush() override {}
 virtual void reset() override {}
 virtual void drop() override {}
 virtual bool isEmpty() override {return true;}
 virtual std::string readRemainingData() override {return "";}
 //...
};

I hate to break it to you, but MySQLStream is not really a PackageStream, at least not in the sense that you define it right now.

The problem is this: If you have a base class, and you choose to derive from it, you should have a (meaningful) implementation for everything the base class requires you to implement. It takes no expert to see that this currently is not the case at all.

But where is the actual problem? One issue is the interface of PackageStream, in particular what methods it requires:

• flush makes only sense in context of a buffered stream, which clearly is not the case here, so it should be the first thing to go.

• reset is unclear. What do you want to reset? As far as my understanding of patterns goes, you can't really reset a stream, because a stream is a continuous source of something that is consumed directly. However, if it is stream settings or something akin you are talking about, this method might be fine.

• drop seems sensible, but I would expect some kind of how-many parameter (i.e. how many packages to drop).

• isEmpty also does not seem too strange, if you assume to be working with some concept of finiteness here. For PackageStream, this could return true, for example, when all data has been received and the connection has been closed normally (a case which you don't seem to have considered so far).

• The same reasoning applies to readRemainingData, although I am not convinced that this method is a good idea in general, since in most cases you might not be able to guess how much data remains.

The other issue is the inheritance relationship itself. In my opinion, MySQLStream does not qualify as a PackageStream simply because it has nothing to do with packages. In fact, MySQLStream seems to be nothing more than a thin abstraction over raw sockets, wrapping the concept of a simple connection. You should rethink your structural approach to categorizing these classes.

Error checking

Since it appears that you are writing a library, you absolutely have to add some sanity checks unless you want users to hate you because their programs go crashing and they can't find out why. For example, you assume that pointers you receive from calling code are valid, which they might not be. One can easily cause undefined behavior by, for example, passing a null pointer to read, or to write, a case you should definitely guard against.

---

1. static std::size_t constexpr ErrorResult = static_cast<std::size_t>(-1); should be ... = std::numeric_limits<std::size_t>::max();. I hope you agree that the latter expresses your intent much clearer.
2. Instead of errno.h, include cerrno. The former is a C header, the latter is the C++ equivalent. Also, MockStream.h should include cstddef (or similar) for std::size_t.
3. Even when dealing with C APIs, please try and stick to your C++ coding style as well as possible. This includes not using C-casts, using nullptr instead of NULL, using functions from the C++-version of the C standard library headers (looking at you here, memset) etc.
4. If your member attributes have default values (such as writSoFar and readSoFar of MockStream have), you should assign them in default member initializers, not in the constructor. This prevents undefined behavior from missing an initialization somewhere in more complex cases and also declutters your constructors a bit.
5. About C-style strings and string buffers: Don't use them. C++ has better things to offer:std::string, std::string_view (since C++17), even std::vector. Even if those are not to your liking, please take the time to find a better method of passing data around than pointer-length-pairs.
6. Some typos: writen should probably be written. Similarly, writenSoFar should likely read writtenSoFar.