9
\$\begingroup\$

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.

\$\endgroup\$

1 Answer 1

14
\$\begingroup\$

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.

\$\endgroup\$
5
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ Aug 6, 2022 at 16:16
  • 2
    \$\begingroup\$ 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. \$\endgroup\$
    – code_dredd
    Aug 6, 2022 at 19:37
  • \$\begingroup\$ Thanks for providing many great comments. I have following 3 points: ① 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. ② Renaming response() to recv() is making the compiler unhappy. It is getting confused with recv() from the socket library. ③ 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. \$\endgroup\$
    – ravi
    Aug 7, 2022 at 6:26
  • \$\begingroup\$ ① That might work, but note that errno doesn't reset itself automatically. ② You have to write ::recv() to get the recv() function from the global namespace then. ③ You could consider creating a wrapper class for telnet_t. \$\endgroup\$
    – G. Sliepen
    Aug 7, 2022 at 8:14
  • \$\begingroup\$ G. Sliepen: The new vs. vector for allocation of memory on the heap was totally new to me! Thank you very much for this! \$\endgroup\$ Aug 7, 2022 at 18:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.