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;}
};

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.