C++ Socket Part-2

In my ongoing attempts to become a better blog writer I have some written some more code that needs reviewing.

Full Source.

First Article.

This is a simple C++ wrapper for Sockets.
Here the bit I am not 100% happy with is needing to introduce a Socket.tpp file to hold getMessageData(). This is because the function is templated with a callback function that is used to decide if we have read enough. This is not used by the ProtocolSimple (in Version 2) but will be used by ProtocolHTTP (in Version 3).

##Socket.h

```
#ifndef THORSANVIL_SOCKET_SOCKET_H
#define THORSANVIL_SOCKET_SOCKET_H

#include
#include
#include

namespace ThorsAnvil
{
namespace Socket
{

// An RAII base class for handling sockets.
// Socket is movable but not copyable.
class BaseSocket
{
int socketId;
protected:
// Designed to be a base class not used used directly.
BaseSocket(int socketId);
int getSocketId() const {return socketId;}
public:
~BaseSocket();

// Moveable but not Copyable
BaseSocket(BaseSocket&& move) noexcept;
BaseSocket& operator=(BaseSocket&& move) noexcept;
void swap(BaseSocket& other) noexcept;
BaseSocket(BaseSocket const&) = delete;
BaseSocket& operator=(BaseSocket const&) = delete;

// User can manually call close
void close();
};

// A class that can read/write to a socket
class DataSocket: public BaseSocket
{
public:
DataSocket(int socketId)
: BaseSocket(socketId)
{}

template
std::size_t getMessageData(char* buffer, std::size_t size, F scanForEnd = [](std::size_t){return false;});
void putMessageData(char const* buffer, std::size_t size);
void putMessageClose();
};

// A class the conects to a remote machine
// Allows read/write accesses to the remote machine
class ConnectSocket: public DataSocket
{
public:
Co

A public non-virtual destructor in a base class is risky. You should consider making it protected, or otherwise virtual, if there's need for polymorphic deletion of the BaseSocket.

I don't think 0 is a good value to indicate an uninitialized socket id. The socket function will return:


Upon successful completion, socket() shall return a non-negative integer, the socket file descriptor. Otherwise, a value of -1 shall be returned and errno set to indicate the error.

Zero is non-negative, so it could actually return the value of 0 for a valid socket. I would recommend defining a constant, InvalidSocketId and setting it to -1. Then you can use that constant inside your class to denote the invalid/uninitialized socket.

Don't repeat yourself, not even in the error messages.

if (getSocketId() == 0)
{
    throw std::logic_error(buildErrorMessage("DataSocket::getMessage: accept called on a bad socket object (this object was moved)"));
}

All functions that throw give a nice error message with the function name in it, and I commend you for taking that care, but writing down the name of the function as a string is a task that should be automated. By the way, notice that in the excerpt above the name in the error message (getMessage) isn't matching the name of the function throwing the exception (getMessageData). That's a very common little mistake we find in this pattern. The programmer will never remember to update the error messages when the name of the function changes.

YMMV, but such case seems like a valid case for a macro. Unfortunately, __func__ is not standard, but supported by GCC and Clang. MSVC provides the equivalent macro __FUNCTION__, which it appears is also supported by GCC for compatibility.

#define THORSANVIL_SOCKET_ERROR(exception, message) \
    throw exception{ buildErrorMessage(__FILE__, ":", __LINE__, " at ", __func__, ": ", message) }

THORSANVIL_SOCKET_ERROR(std::logic_error, "accept called on a bad socket object (this object was moved)");

Should print something like:

Socket.tpp:20 at DataSocket::getMessageData: accept called on a bad socket object (this object was moved)

Nitpickings

Please don't use bzero. It is very archaic and non-standard. You can use std::memset to the same effect, or better, call the default zero-initializing constructor: sockaddr_in serverAddr{}. While you are at it, you could also remove those struct tags in the variable declarations, so the code doesn't look like it was copied from some old C book ;).

Not so sure if domain_error has a place here. It is usually associated with numerical and mathematical errors. You could very well just go with runtime_errors there, or extend runtime_error with your own exception class(es).

StackExchange Code Review Q#131137, answer score: 6