A simple TCP command executer

Question

In https://github.com/pc-magas/tcp_command as an attempt to learn C++ I made a simplistic TCP server in GNU/Linux that will receives simple "commands" in ASCII form. Each command ends with '\n' and a user can send multiple commands at once.

My background is a web developer in php using symfony mostly and sometimes PSR-4 autoloading and heavily on Depedency Injection. Also I use Node.js as well and because in the future I plan to use C++ as Node.js/electron bindings I try to learn C++.

The application is in this github repo:

My main.cpp file is simple like as hell:

#include<iostream>
#include<string>

#include "socket/network.h"
#include "socket/simple_command.h"
#include "tools/command_parser.h"

int main(){
    try{
        CommandParser p;
        SimpleCommandHandler c(10,&p);
        TCPServer server(7070, "127.0.0.1", &c);
        server.listen();
    }catch(Exception e){
        std::cout<< e.getMessage();
        return -1;
    }
    return 0;
}

And I use the following implementations for the TCP server:

• network.h

#ifndef HTTP_NETWORK
#define HTTP_NETWORK

#include <string>
#include <arpa/inet.h>

//Dummy Value to be changed
#define MAXPENDING 5

class Exception {
    public:
    Exception(std::string message):message(message){}
    std::string getMessage();
    private:
    std::string message;
};

class NetworkException:public Exception {
    public:
    NetworkException(std::string message) : Exception(message) {}
};

//A generic way to handle Network Connections
class ConnectionHandler {
    public:
    /**
    * @return 0 Close Connection 1 do not close
    */
    virtual int handle(int socketid) = 0;
};

class TCPServer {
    public:
        TCPServer(int port, std::string address, ConnectionHandler *c);
        ~TCPServer();
        void listen();
    private:
        int port;
        //Socket file Descriptor
        int servSock;
        struct sockaddr_in ServAddr;
        ConnectionHandler *c = NULL;
};


#endif

• network.cpp

#include"network.h"

#include <sys/socket.h>
#include <arpa/inet.h>
#include <unistd.h>

#include<iostream>
#include<cstring>
#include<string>
#include <thread>

std::string Exception::getMessage(){
    return this->message;
}

TCPServer::TCPServer(int port,std::string address, ConnectionHandler *c)
:port(port){

    if ((this->servSock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) {
        throw NetworkException(std::string("SOCKET Error: could not create basic socket"));
    }

    memset(&this->ServAddr,0,sizeof(this->ServAddr));

    ServAddr.sin_family = AF_INET;
    ServAddr.sin_addr.s_addr = inet_addr(address.c_str());
    ServAddr.sin_port = htons(port);


    if (bind(this->servSock, (struct sockaddr *) &this->ServAddr, sizeof(this->ServAddr)) < 0) {
        throw NetworkException(std::string("SOCKET Error: Failed to bind a socket"));
    }

    if (::listen(this->servSock, MAXPENDING) < 0) {
        throw NetworkException(std::string("SOCKET Error: Failed to Listen"));
    }

    if(c==NULL){
       throw Exception("You should provice a connection Handler"); 
    }

    this->c=c;
}

void TCPServer::listen(){

    struct sockaddr_in ClntAddr;     /* Client address */
    socklen_t clntLen= (socklen_t)sizeof(ClntAddr);
    int clntSock;                    /* Socket descriptor for client */
    //@todo Dummy Logic Depedency Inject Socket Handler
    for (;;) {
       if ((clntSock = accept(servSock, (struct sockaddr *) &ClntAddr, &clntLen)) < 0) {
           std::cout<<"Failed to fetch"<<std::endl;
       }

       std::cout << "Handling client: " << inet_ntoa(ClntAddr.sin_addr) << std::endl;
       send(clntSock, "WELCOME\n", 6, 0);
       this->c->handle(clntSock);
       close(clntSock);
    }
}

TCPServer::~TCPServer(){
 close(this->servSock);
}

Also, I have the SimpleCommandHandler to handle my network traffic:

• simple_command.h: (I use the flag NETWORK_TELNET as a leftover from initial plan to use the telnet protocol.)

#ifndef NETWORK_TELNET
#define NETWORK_TELNET

#include"network.h"
#include"../tools/command_parser.h"
#include<string>

class SimpleCommandHandler:public ConnectionHandler{
    public:
    SimpleCommandHandler(int readBufferSize, CommandParser *commandParser):buffLen(readBufferSize),parser(commandParser){};
    ~SimpleCommandHandler(){};
    int handle(int socketid);

    private:
    std::string readLine(int socketid);
    void sendResult(int socketid, std::string result);
    const int buffLen;
    CommandParser* parser;
};
#endif

• simple_command.cpp:

#include "simple_command.h"
#include<string>
#include<cstring>
#include<iostream>

std::string SimpleCommandHandler::readLine(int socketid){
    int recvSize=0;
    char buffer[this->buffLen];

    memset(&buffer,'\0',this->buffLen*sizeof(char));
    //reCeive a Byte less in order each received data to be a string (Initialized as an empty String with \0 encding char)
    while ((recvSize = recv(socketid, buffer, this->buffLen-1, 0)) > 0) {
        this->parser->addData(socketid,(const char*) buffer,strlen(buffer));
        memset(buffer,'\0',this->buffLen*sizeof(char)); //Reset Data in order to avoid Garbage
    }

    return this->parser->getCommand(socketid);
}

void SimpleCommandHandler::sendResult(int socketid, std::string result){
    send(socketid, result.c_str(), result.length() + 1, 0);
}

int SimpleCommandHandler::handle(int socketid){
    std::string command = this->readLine(socketid);
    std::cout << "Command Sent: " << command << std::endl;
    if(command.compare("exit") == 0){
        this->sendResult(socketid,"Thank You Very Much\nExiting\n");
        return 0;
    } else {
        this->sendResult(socketid,"Wrong Command\n");
    }

    return 1;
}

And in the end the utility for parsing the commands:

• command_parser.h:

#ifndef COMMANDPARSER
#define COMMANDPARSER

#include<string>
#include<map>

class CommandParser{
    private:
        std::map<int,std::string> commandBuff;
    public:
        std::string getCommand(int socketid);
        void addData(int socketid, const char* data, int length);
};

#endif

*command_parser.cpp:

#include "command_parser.h"
#include<string>
#include<iostream>

void CommandParser::addData(int socketid, const char* data, int length){
    this->commandBuff[socketid].append(data,length);
}

std::string CommandParser::getCommand(int socketid){
    std::size_t pos = this->commandBuff[socketid].find('\n');

    if (pos==std::string::npos){
        return "";
    }

    std::string fetchedCommand = this->commandBuff[socketid].substr(0,pos);
    this->commandBuff[socketid] = this->commandBuff[socketid].substr(pos+1);
    return fetchedCommand;
}

I would like to hear the comments towards my code, mabe by me, the c++ virgin ;).

Answer 1

I'll start the serious bug, then work inwards from main().

---

Biggest bug

The biggest problem with the code is that it only reads one command from each connection, leaving any subsequent commands for the next client to execute. Even if we read more commands, by looping until handle() returns false before closing the client socket, this still won't help if the client has commands after exit:

netcat -4 localhost 7070  <<<$'exit\nstop\n'

---

main()

}catch(Exception e){
    std::cout<< e.getMessage();
    return -1;
}

-1 is a strange exit code - we normally use small positive numbers.

Error messages should go to std::cerr, not to standard output.

Most C++ runtime environments already do something similar - there's probably no need to catch all exceptions at this level.

    TCPServer server(7070, "127.0.0.1", &c);

Port and bind address should be configurable - ideally from command arguments, but a short-term start would be to define file-scope constants.

return 0;

Although not wrong, there's little benefit to explicitly returning zero from main().

For a real Internet server, you'll normally want to daemonize the process (but retain user option to run in foreground, for debugging).

---

TCPServer::TCPServer()

TCPServer::TCPServer(int port,std::string address, ConnectionHandler *c)
:port(port){

We initialize port, but not the other members of the object.

That said, port and ServAddr don't need to be members - they are used only to create servSock and then never used again. Member c needs a better name - remember, variable name verbosity should be proportional to the scope of the variable; members need more descriptive names than two-line locals. Here's how I'd write the initializer list:

TCPServer::TCPServer(int port, const std::string&, ConnectionHandler *connection)
    : servSock(socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)),
      connection(connection)
{

Perhaps consider passing a reference to the connection handler. This will have the side effect of making TCPServer non-assignable - but you probably want it to be non-copyable anyway.

It's idiomatic to use the truthiness of a pointer to test for null pointers:

if (!connection) {
   throw Exception("You should provide a connection Handler"); 
}

There's a lot of unnecessary qualification of members with this-> - you should almost never need to use this with -> like that.

memset should be std::memset.

    throw NetworkException(std::string("SOCKET Error: could not create basic socket"));

No need to explicitly construct std::string there - there's an implicit constructor that will be used. It might be nice to incorporate the error message from errno (use errstr() to create a human-friendly version). That applies to the checks after bind() and listen(), too.

---

TCPServer::listen

    std::cout << "Handling client: " << inet_ntoa(address.sin_addr) << std::endl;

Status messages should go to std::clog, not standard output. And for this error, we don't want to use the bad client socket descriptor, so return immediately.

   send(client, "WELCOME\n", 6, 0);

Why write only WELCOM here? We probably want to send all 8 characters:

    static auto constexpr message = "WELCOME\n";
    send(client, message, std::strlen(message), 0);

---

SimpleCommandHandler::readLine()

char buffer[buffLen];

That's not valid C++, as buffLen isn't a compile-time constant expression. We could perhaps use a vector for this:

std::vector<char> storage(buffLen);
char *const buffer = storage.data();

Again, std::memset() needs its namespace qualification.

Remember that sizeof reports in units of char, so sizeof (char) must be 1 by definition. Adding * sizeof (char) just serves to make the code harder to read.

There's no need to null out the whole of the buffer every time we read a line. All that's required is to pass the correct length to the addData():

std::string SimpleCommandHandler::readLine(int socketid){
    int recvSize=0;
    char buffer[buffLen];

    while ((recvSize = recv(socketid, buffer, buffLen-1, 0)) > 0) {
        parser->addData(socketid, buffer, recvSize);
    }

    return parser->getCommand(socketid);
}

I'm not sure why we need a map of client socket to command string, since we have a blocking server, that services only one client at a time. And since we don't clear the command buffer after a client disconnects, we end up executing old commands of previous sessions when a new client connects. (Part of the cause is that readLine() actually reads entire transmissions, not necessarily a single line).

---

CommandParser::getCommand()

Instead of repeatedly looking up in the map, it's better to make a local reference to the command buffer:

std::string CommandParser::getCommand(int socketid)
{
    std::string& buffer = commandBuff[socketid];

    std::size_t pos = buffer.find('\n');
    if (pos == std::string::npos) {
        return "";
    }

    std::string fetchedCommand = buffer.substr(0, pos);
    buffer = buffer.substr(pos+1);
    return fetchedCommand;
}

---

Other bits and pieces

ConnectionHandler has virtual methods; that's normally a sign that you want a virtual destructor. Conversely, SimpleCommandHandler doesn't need to specify that its destructor is virtual. It should declare its override with override, though.

Exception would be simpler if it just inherits std::exception - or better, std::runtime_error.

---

Modified code

Incorporating some of the improvements suggested above (and all as one source file, because that's how I built it):

#include <string>
#include <map>

class CommandParser
{
private:
    std::map<int,std::string> commandBuff = {};

public:
    std::string getCommand(int socketid);
    void addData(int socketid, const char* data, int length);
    void clear(int socketid);
};

//A generic way to handle Network Connections
class ConnectionHandler
{
public:
    virtual ~ConnectionHandler() = default;

    /**
     * @return true if we should read more commands, else false
     */
    virtual bool handle(int socketid) = 0;

    bool finished = false;
};

class SimpleCommandHandler: public ConnectionHandler
{
public:
    SimpleCommandHandler(std::size_t readBufferSize, CommandParser& commandParser)
        : buffLen(readBufferSize),
          parser(commandParser)
    {}
    SimpleCommandHandler(const SimpleCommandHandler&) = delete;
    bool handle(int socketid) override;

private:
    std::string readLine(int socketid);
    void sendResult(int socketid, std::string result);
    const std::size_t buffLen;
    CommandParser& parser;
};

#include <string>

//Dummy Value to be changed
#define MAXPENDING 5

class Exception
{
public:
    Exception(std::string message)
        : message(message)
    {}
    std::string getMessage();
private:
    std::string message;
};

#include <cstring>
#include <exception>

class NetworkException: public std::runtime_error {
public:
    NetworkException(std::string message)
        : std::runtime_error(message + ": " + std::strerror(errno))
    {}
};

class TCPServer
{
public:
    TCPServer(int port, const std::string&, ConnectionHandler& connection);
    ~TCPServer();
    void listen();
private:
    //Socket file Descriptor
    int socket;
    ConnectionHandler& connection;
};


int main()
{
    CommandParser p;
    SimpleCommandHandler connection(10, p);
    TCPServer server(7070, "127.0.0.1", connection);
    server.listen();
}


#include <sys/socket.h>
#include <arpa/inet.h>
#include <unistd.h>

#include <cstring>
#include <iostream>
#include <string>

std::string Exception::getMessage() {
    return message;
}

TCPServer::TCPServer(int port, const std::string& address, ConnectionHandler& connection)
    : socket(::socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)),
      connection(connection)
{
    if (socket < 0) {
        throw NetworkException("SOCKET Error: could not create basic socket");
    }

    struct sockaddr_in s_address; // server address
    std::memset(&s_address, 0, sizeof s_address);
    s_address.sin_family = AF_INET;
    s_address.sin_addr.s_addr = ::inet_addr(address.c_str());
    s_address.sin_port = ::htons(port);

    if (::bind(socket, (struct sockaddr*)&s_address, sizeof s_address) < 0) {
        throw NetworkException("SOCKET Error: Failed to bind a socket");
    }

    if (::listen(socket, MAXPENDING) < 0) {
        throw NetworkException("SOCKET Error: Failed to listen");
    }

}

void TCPServer::listen()
{
    while (!connection.finished) {
        struct sockaddr_in address;     /* Client address */
        socklen_t length = sizeof address;
        int client;                    /* Socket descriptor for client */
        if ((client = ::accept(socket, (struct sockaddr*)&address, &length)) < 0) {
            std::cerr << "Failed to accept: " << std::strerror(errno) << std::endl;
            return;
        }
        std::clog << "Handling client: " << ::inet_ntoa(address.sin_addr) << std::endl;

        static auto constexpr message = "WELCOME\n";
        send(client, message, std::strlen(message), 0);

        while (connection.handle(client))
            ;
        close(client);

        // discard remaining received commands

    }
}

TCPServer::~TCPServer()
{
    ::close(socket);
}


#include <cstring>
#include <iostream>
#include <string>
#include <vector>

std::string SimpleCommandHandler::readLine(int socketid)
{
    std::vector<char> storage(buffLen);
    char *const buffer = storage.data();

    int recvSize = 0;
    while ((recvSize = ::recv(socketid, buffer, buffLen-1, 0)) > 0) {
        parser.addData(socketid, buffer, recvSize);
    }

    return parser.getCommand(socketid);
}

void SimpleCommandHandler::sendResult(int socketid, std::string result) {
    send(socketid, result.c_str(), result.length() + 1, 0);
}

bool SimpleCommandHandler::handle(int socketid)
{
    std::string command = readLine(socketid);
    std::clog << "Command received: " << command << std::endl;

    if (command == "exit") {
        sendResult(socketid, "Thank You Very Much.\nBye.\n");
        parser.clear(socketid);
        return false;
    } else if (command == "stop") {
        sendResult(socketid, "Server exiting.\n");
        parser.clear(socketid);
        finished = true;
        return false;
    } else {
        sendResult(socketid, "Ignoring command '" + command + "'.\n");
        return true;
    }
}

#include <string>

void CommandParser::addData(int socketid, const char* data, int length) {
    commandBuff[socketid].append(data,length);
}

std::string CommandParser::getCommand(int socketid)
{
    std::string& buffer = commandBuff[socketid];

    std::size_t pos = buffer.find('\n');
    if (pos == std::string::npos) {
        return "";
    }

    std::string fetchedCommand = buffer.substr(0, pos);
    buffer = buffer.substr(pos+1);
    return fetchedCommand;
}

void CommandParser::clear(int socketid)
{
    commandBuff[socketid].clear();
}