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.

• anon: Disagree on the overuse of exceptions. This is a perfectly good use of exceptions. However, if I was calling this class I'd prefer to get custom exceptions that allow different causes to be distinguished (since many can be solved by retrying with a fresh connection) rather than just a std::runtime_exception with a string.
• code_dredd: I'd add that cerr should be used for the program to print all its own logging, including progress and/or status reports, whereas cout should be used for the actual output the program is meant to produce. For example, a program add 2 3 would print 5 to stdout but might print a log message such as adding 2 + 3 ... to stderr.
• ravi: Thanks for providing many great comments. I have following 3 points: 1 Regarding the recoverable error, I plan to use do { select or recv or send ( ... ); } whie (errno == EINTR); statement. Correct me, if I am on the wrong track, please. 2 Renaming response() to recv() is making the compiler unhappy. It is getting confused with recv() from the socket library. 3 The telnet_t *mTelnet pointer is still bothering me. I thought of using std::unique_ptr to replace it, but it has to incorporate telnet_free(mTelnet) as well. I have a feeling the custom deleter in C++ can help, though.
• G. Sliepen: 1 That might work, but note that errno doesn't reset itself automatically. 2 You have to write ::recv() to get the recv() function from the global namespace then. 3 You could consider creating a wrapper class for telnet_t.
• Martin Sand: G. Sliepen: The new vs. vector for allocation of memory on the heap was totally new to me! Thank you very much for this!