Porting C-style socket to CPP class

Question

I am porting the C-style socket to design a simple wrapper around the telnet client in CPP. The telnet protocol is accomplished by using libtelnet in C. The C-style code for this wrapper is reviewed here and here as well.

I want to follow standard practices in my CPP code, such as using smart pointers in CPP instead of raw pointers, etc. Therefore, please see my implementation below:

1. simple_telnet_client.cpp

#include <simple_telnet_client/simple_telnet_client.hpp>

namespace simple_telnet_client {

class NotImplemented : public std::logic_error {
 public:
  NotImplemented() : std::logic_error("Function not yet implemented"){};
};

TelnetClient::TelnetClient(const std::string &serverIP,
                           const std::string &serverPort, 
                           const size_t timeOut,
                           const size_t bufferSize)
    : mBufferSize(bufferSize) {
  // define our buffer
  mBuffer = static_cast<char *>(malloc(sizeof(char) * mBufferSize));

  try {
    // let's make socket
    mSockFd = makeConnection(serverIP, serverPort);
  } catch (const std::runtime_error &error) {
    // throw it to the user
    throw std::runtime_error(error);
  }

  // socket related configuarations
  configureReadWriteFd();
  configureTimeout(timeOut);

  // telnet options
  const telnet_telopt_t telnetOpts[] = {
      {     TELNET_TELOPT_ECHO, TELNET_WONT,   TELNET_DO},
      {    TELNET_TELOPT_TTYPE, TELNET_WILL, TELNET_DONT},
      {TELNET_TELOPT_COMPRESS2, TELNET_WONT,   TELNET_DO},
      {     TELNET_TELOPT_MSSP, TELNET_WONT,   TELNET_DO},
      {                     -1,           0,           0}};

  // initialize telnet box
  mTelnet = telnet_init(telnetOpts, trampoline, 0, this);
}

int TelnetClient::makeConnection(const std::string &serverIP,
                                 const std::string &serverPort) {
  int retVal;
  int sockFd;
  struct addrinfo *addrInfo;
  struct addrinfo addrHints;

  // look up server host
  memset(&addrHints, 0, sizeof(addrHints));
  addrHints.ai_family = AF_INET;
  addrHints.ai_socktype = SOCK_STREAM;

  if ((retVal = getaddrinfo(serverIP.c_str(), serverPort.c_str(), &addrHints, &addrInfo)) != 0) {
    throw std::runtime_error("getaddrinfo() failed for " + serverIP + " with error: " + gai_strerror(retVal));
  }

  // create server socket
  if ((sockFd = socket(addrInfo->ai_family, addrInfo->ai_socktype, addrInfo->ai_protocol)) == -1) {
    throw std::runtime_error("socket() failed: " + std::string(strerror(errno)));
  }

  // connect
  if (connect(sockFd, addrInfo->ai_addr, addrInfo->ai_addrlen) == -1) {
    auto error = std::string(strerror(errno));
    close(sockFd);
    throw std::runtime_error("connect() failed: " + error);
  }

  // free address lookup info
  freeaddrinfo(addrInfo);

  return sockFd;
}

void TelnetClient::trampoline(telnet_t *telnet, 
                              telnet_event_t *event,
                              void *user_data) {
  (void)telnet;  // to get rid of unused parameter warning
  TelnetClient *me = static_cast<TelnetClient *>(user_data);
  me->telnetEvent(event);
}

void TelnetClient::telnetEvent(telnet_event_t *event) {
  switch (event->type) {
    // data received
    case TELNET_EV_DATA:
      mReceivedMsg = std::string(event->data.buffer, event->data.size);
      #if DEBUG_MODE
        std::cout << "response: [" << mReceivedMsg << "]" << std::endl;
      #endif
      break;
    // data must be sent
    case TELNET_EV_SEND:
      sendAll(event->data.buffer, event->data.size);
      break;
    // execute to enable local feature (or receipt)
    case TELNET_EV_DO:
      throw NotImplemented();
    // demand to disable local feature (or receipt)
    case TELNET_EV_DONT:
      throw NotImplemented();
    // respond to TTYPE commands
    case TELNET_EV_TTYPE:
      throw NotImplemented();
    // respond to particular subnegotiations
    case TELNET_EV_SUBNEGOTIATION:
      throw NotImplemented();
    // error
    case TELNET_EV_ERROR:
      throw std::runtime_error("telnet error: " + std::string(event->error.msg));
    default:
      // ignore
      break;
  }
}

int TelnetClient::sendAll(const char *buffer, size_t size) {
  int retVal = -1;  // default value

  // send data
  while (size > 0) {
    retVal = send(mSockFd, buffer, size, 0);
    if (retVal == 0 || errno == EINTR) {
      // try again
      continue;
    } else if (retVal == -1) {
      throw std::runtime_error("send() failed: " + std::string(strerror(errno)));
    }

    // update pointer and size to see if we've got more to send
    buffer += retVal;
    size -= retVal;
  }
  return retVal;
}

void TelnetClient::configureReadWriteFd() {
  // clear the set ahead of time
  FD_ZERO(&mReadFd);
  FD_ZERO(&mWriteFd);

  // add our descriptors to the set
  FD_SET(mSockFd, &mReadFd);
  FD_SET(mSockFd, &mWriteFd);
}

void TelnetClient::configureTimeout(const size_t seconds) {
  mTimeout.tv_sec = seconds;
  mTimeout.tv_usec = 0;
}

void TelnetClient::execute(const std::string &command) {
  int retVal = select(mSockFd + 1, NULL, &mWriteFd, NULL, &mTimeout);

  if (retVal == -1) {
    // error occurred in select()
    throw std::runtime_error("select() failed: " + std::string(strerror(errno)));
  } else if (retVal == 0) {
    throw std::runtime_error("timeout occurred");
  } else {
    // send the execute
    if (FD_ISSET(mSockFd, &mWriteFd)) {
      #if DEBUG_MODE
        // print only for debugging
        std::cout << "request: [" << command << "]" << std::endl;
      #endif

      telnet_send(mTelnet, command.c_str(), command.length());
    }
  }
}

std::string TelnetClient::response() {
  // erase previously received msg by setting it to an empty string
  mReceivedMsg.clear();

  int retVal = select(mSockFd + 1, &mReadFd, NULL, NULL, &mTimeout);
  if (retVal == -1) {
    // error occurred in select()
    throw std::runtime_error("select() failed: " + std::string(strerror(errno)));
  } else if (retVal == 0) {
    throw std::runtime_error("timeout occurred");
  } else {
    // response the response
    if (FD_ISSET(mSockFd, &mReadFd)) {
      if ((retVal = recv(mSockFd, mBuffer, mBufferSize, 0)) > 0) {
        telnet_recv(mTelnet, mBuffer, retVal);
      } else if (retVal == 0) {
        throw std::runtime_error("connection has been closed.");
      } else {
        throw std::runtime_error("recv() failed: " + std::string(strerror(errno)));
      }
    }
  }
  return mReceivedMsg;
}

TelnetClient::~TelnetClient() {
  if (mBuffer) {
    free(mBuffer);
    mBuffer = NULL;
  }

  // clean up
  telnet_free(mTelnet);
  close(mSockFd);
}

}  // namespace simple_telnet_client

2. simple_telnet_client.hpp

#ifndef SIMPLE_TELNET_CLIENT__TELNET_CLIENT_HPP_
#define SIMPLE_TELNET_CLIENT__TELNET_CLIENT_HPP_

#include <netdb.h>
#include <string.h>
#include <unistd.h>

#include <libtelnet.h>

#include <iostream>
#include <stdexcept>

// flag to print requests and responses in the console for debugging purposes
#define DEBUG_MODE true

namespace simple_telnet_client {
class TelnetClient {
 public:
  TelnetClient(const std::string &serverIP, 
               const std::string &serverPort,
               const size_t timeOut = 1, 
               const size_t bufferSize = 2048);

  // execute a command on the server
  void execute(const std::string &command);

  // receive the response from the server
  std::string response();

  ~TelnetClient();

 private:
  int mSockFd;
  const size_t mBufferSize;
  std::string mReceivedMsg;

  fd_set mReadFd;
  fd_set mWriteFd;
  struct timeval mTimeout;

  // todo: use std::unique_ptr?
  telnet_t *mTelnet;

  // todo: how about this?
  char *mBuffer;

  static void trampoline(telnet_t *telnet, 
                        telnet_event_t *event,
                         void *user_data);

  int makeConnection(const std::string &serverIP,
                     const std::string &serverPort);

  void telnetEvent(telnet_event_t *event);

  int sendAll(const char *mBuffer, 
              size_t size);

  void configureTimeout(const size_t seconds);

  void configureReadWriteFd();
};

}  // namespace simple_telnet_client

#endif  // SIMPLE_TELNET_CLIENT__TELNET_CLIENT_HPP_

PS: I am using C++14 and GCC 9.4 in Ubuntu 20.04 LTS.

Answer 1

Avoid using malloc() in C++

There are much better ways to allocate memory in C++ that are simpler and safer. First, in C++ you should use new if you really want to do your own memory allocation:

mBuffer = new char[mBufferSize];

But now you have to remember to delete it. Even better is to just use std::vector to manage a bunch of bytes for you:

class TelnetClient {
    ...
    std::vector<char> mBuffer;
    ...
};

TelnetClient::TelnetClient(const std::string &serverIP,
                           const std::string &serverPort, 
                           const size_t timeOut,
                           const size_t bufferSize)
        : mBuffer(bufferSize) {
    ...
}

Now you no longer need to free mBuffer yourself. To receive data into the buffer, you can write:

retVal = recv(mSockFd, mBuffer.data(), mBuffer.size(), 0);

This also avoids a memory leak in the constructor of TelnetClient, as when you throw because makeConnection() failed, the vector will be automatically cleaned up, but manually allocated memory will not.

Avoid other C-isms in C++

Since you are using socket functions, your code is going to look a bit like C anyway, but try to avoid coding like C and use more idiomatic C++ code where possible. Instead of memcpy() and memset(), consider using std::copy_n() and std::fill_n(). But if you just want to zero-initialize a struct, you can write this:

struct addrinfo addrHints = {};

Pay attention to the return type of functions

In response() you declare int retVal. This is the right type to store the return value of select(). But later on you reuse retVal to store the return value of recv(). However, recv() returns a ssize_t, not an int. Due to implicit conversion rules this is legal, but the value returned might not fit in an int, which can lead to problems. Always store return values in the correct type.

Consider not reusing variables in this case, and using auto to deduce the correct type:

auto retVal = select(...);
...
auto received = recv(...);

Overuse of exceptions

Exceptions should be used for exceptional errors. However, network errors are rather common, and particularly the remote end closing the connection is quite normal. So arguably, instead of throwing exceptions, you should return error codes in these cases. Consider using std::error_code for this.

Since C++17, you could also consider using std::optional<> to return things, and in C++23 we will get std::expected<>.

You also wrote some code that doesn't make sense at all, for example:

try {
    mSockFd = makeConnection(serverIP, serverPort);
} catch (const std::runtime_error &error) {
    throw std::runtime_error(error);
}

Here you catch an exception only to rethrow it immediately. Just don't catch it here, the whole point of exceptions is that they will propagate up the call stack until something can actually handle the error.

As Jack Aidley mentioned, it also helps if you create your own exception types that inherit from the standard exception types. This allows callers to programmatically detect exactly what kind of error happened, without them having to parse the .what() text.

Not all errors are fatal

select(), recv() and send() might return -1 also when there is a recoverable error. In particular, if errno == EINTR, you should just try again.

Write log and error messages to std::cerr

Always write log, warning and error messages to std::cerr. Consider that the regular output of a program goes to std::cout, and if you are redirecting the standard output to a file, then if warnings and errors are written to std::cout as well, both get mixed up in the file, which is undesirable.

Use '\n' instead of std::endl

Prefer using '\n' instead of std::endl; the latter is equivalent to the former but also forces the output to be flushed, which is often unnecessary and might impact performance.

Avoiding unused parameter warnings

In C++ you can omit the name of a function parameter, this will suppress unused parameter warnings:

void TelnetClient::trampoline(telnet_t *, 
                              telnet_event_t *event,
                              void *user_data) {
  TelnetClient *me = static_cast<TelnetClient *>(user_data);
  me->telnetEvent(event);
}

response() might not return the full response

response() only ever calls recv() once, so it is perfectly possible to only receive a partial response, but the name of this function suggests that it always receives a complete response.

Either this function should be written to call recv() in a loop until the whole response has been received (and deal with all the issues you might have with this, which we already discussed here), or it should just be renamed to recv() so it implies it has the same semantics as the POSIX recv() function.