2
\$\begingroup\$

This is a follow up question to Send and receive functions for telnet client.

I am designing a simple wrapper around the telnet client using libtelnet for text-based communication to a telnet server. Please see my implementation below:

#include <arpa/inet.h>
#include <ctype.h>
#include <errno.h>
#include <netdb.h>
#include <netinet/in.h>
#include <poll.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/time.h>
#include <sys/types.h>
#include <termios.h>
#include <unistd.h>

#ifdef HAVE_ZLIB
#include "zlib.h"
#endif

#include "libtelnet.h"

#define BUFFER_SIZE 512

// following typedefs should be a part of header file
typedef struct addrinfo Addrinfo;
typedef struct sockaddr_in Sockaddr;
typedef struct timeval Timeval;

static telnet_t *telnet;

static const telnet_telopt_t telopts[] = {
    {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}};

static int send_all(int sock_fd, char *buffer, size_t size) {
  int ret_val = -1;  // default value

  // send data
  while (size > 0) {
    if ((ret_val = send(sock_fd, buffer, size, 0)) == -1) {
      fprintf(stderr, "send() failed: %s\n", strerror(errno));
      exit(-1);
    } else if (ret_val == 0) {
      fprintf(stderr, "send() unexpectedly returned 0\n");
      exit(-1);
    }

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

static void event_handler(telnet_t *telnet, telnet_event_t *event,
                          void *user_data) {
  int sock_fd = *(int *)user_data;
  char msg[event->data.size + 1];

  switch (event->type) {
    // data received
    case TELNET_EV_DATA:
      strncpy(msg, event->data.buffer, event->data.size);
      msg[event->data.size] = '\0';
      printf("Response: [%s]\n", msg);
      break;
    // data must be sent
    case TELNET_EV_SEND:
      send_all(sock_fd, event->data.buffer, event->data.size);
      break;
    // request to enable local feature (or receipt)
    case TELNET_EV_DO:
      break;
    // demand to disable local feature (or receipt)
    case TELNET_EV_DONT:
      break;
    // respond to TTYPE commands
    case TELNET_EV_TTYPE:
      // respond with our terminal type, if requested
      if (event->ttype.cmd == TELNET_TTYPE_SEND) {
        telnet_ttype_is(telnet, getenv("TERM"));
      }
      break;
    // respond to particular subnegotiations
    case TELNET_EV_SUBNEGOTIATION:
      break;
    // error
    case TELNET_EV_ERROR:
      fprintf(stderr, "ERROR: %s\n", event->error.msg);
      exit(EXIT_FAILURE);
    default:
      // ignore
      break;
  }
}

static Timeval configure_timeout() {
  // timeout 1 sec
  Timeval time;
  time.tv_sec = 1;
  time.tv_usec = 0;
  return time;
}

static void try_send(int sock_fd, char *cmd, size_t cmd_len) {
  fd_set write_fd;

  // clear the set ahead of time
  FD_ZERO(&write_fd);

  // add our descriptors to the set
  FD_SET(sock_fd, &write_fd);

  Timeval tv = configure_timeout();

  int ret_val = select(sock_fd + 1, NULL, &write_fd, NULL, &tv);

  if (ret_val == -1) {
    // error occurred in select()
    perror("select");
  } else if (ret_val == 0) {
    fprintf(stderr, "Timeout occurred!\n");
  } else {
    // send the request
    if (FD_ISSET(sock_fd, &write_fd)) {
      // print only for debugging
      char msg[cmd_len + 1];
      strncpy(msg, cmd, cmd_len);
      msg[cmd_len] = '\0';
      printf("Request: [%s]\n", msg);
      telnet_send(telnet, cmd, cmd_len);
    }
  }
}

static int try_recv(int sock_fd, char *buffer) {
  int ret_val = -1;  // default value
  fd_set read_fd;

  // clear the set ahead of time
  FD_ZERO(&read_fd);

  // add our descriptors to the set
  FD_SET(sock_fd, &read_fd);

  Timeval tv = configure_timeout();

  ret_val = select(sock_fd + 1, &read_fd, NULL, NULL, &tv);

  if (ret_val == -1) {
    // error occurred in select()
    perror("select");
  } else if (ret_val == 0) {
    fprintf(stderr, "Timeout occurred!\n");
  } else {
    // receive the response
    if (FD_ISSET(sock_fd, &read_fd)) {
      if ((ret_val = recv(sock_fd, buffer, BUFFER_SIZE, 0)) > 0) {
        telnet_recv(telnet, buffer, ret_val);
      } else if (ret_val == 0) {
        fprintf(stderr, "connection has been closed.\n");
        exit(-1);
      } else {
        fprintf(stderr, "recv() failed: %s\n", strerror(errno));
        exit(-1);
      }
    }
  }
  return ret_val;
}

static int make_connection() {
  int ret_val;
  int sock_fd;
  Sockaddr socket_addr;
  Addrinfo *addr_info;
  Addrinfo addr_hints;

  const char *servname = "50000";
  const char *hostname = "192.168.102.85";

  // look up server host
  memset(&addr_hints, 0, sizeof(addr_hints));
  addr_hints.ai_family = AF_UNSPEC;
  addr_hints.ai_socktype = SOCK_STREAM;
  if ((ret_val = getaddrinfo(hostname, servname, &addr_hints, &addr_info)) != 0) {
    fprintf(stderr, "getaddrinfo() failed for %s: %s\n", hostname, gai_strerror(ret_val));
    return -1;
  }

  // create server socket
  if ((sock_fd = socket(AF_INET, SOCK_STREAM, 0)) == -1) {
    fprintf(stderr, "socket() failed: %s\n", strerror(errno));
    return -1;
  }

  // bind server socket
  memset(&socket_addr, 0, sizeof(socket_addr));
  socket_addr.sin_family = AF_INET;
  if (bind(sock_fd, (struct sockaddr *)&socket_addr, sizeof(socket_addr)) == -1) {
    fprintf(stderr, "bind() failed: %s\n", strerror(errno));
    close(sock_fd);
    return -1;
  }

  // connect
  if (connect(sock_fd, addr_info->ai_addr, addr_info->ai_addrlen) == -1) {
    fprintf(stderr, "connect() failed: %s\n", strerror(errno));
    close(sock_fd);
    return -1;
  }

  // free address lookup info
  freeaddrinfo(addr_info);

  return sock_fd;
}

int main() {
  int num_bytes;
  char buffer[BUFFER_SIZE];
  int sock_fd = make_connection();

  if (sock_fd < 0) {
    return EXIT_FAILURE;
  }

  // initialize telnet box
  telnet = telnet_init(telopts, event_handler, 0, &sock_fd);

  // our server returns "Welcome\r\n" upon a successful connection
  num_bytes = try_recv(sock_fd, buffer);
  printf("Bytes: [%d]\n", num_bytes);  // print bytes only for debugging
  
  // the second recv is needed
  num_bytes = try_recv(sock_fd, buffer);
  printf("Bytes: [%d]\n", num_bytes);  // print bytes only for debugging

  // define our commands
  const char *commands[] = {"INIT\r\n", 
                            "READ PT3\r\n", 
                            "READ PT4\r\n", 
                            "REM PT3\r\n"};

  size_t commands_count = sizeof(commands) / sizeof(*commands);
  size_t i;
  for (i = 0; i < commands_count; i++) {
    try_send(sock_fd, commands[i], strlen(commands[i]));
    num_bytes = try_recv(sock_fd, buffer);
    printf("Bytes: [%d]\n", num_bytes);  // print bytes only for debugging
  }

  // clean up
  telnet_free(telnet);
  close(sock_fd);

  return EXIT_SUCCESS;
}

Output

I added a few print statements for debugging purposes. Please see below the complete log displayed on the terminal.

ravi@dell:~/telnet$ ./my_client
Response: [Welcome
]
Bytes: [8]
Response: [
]
Bytes: [2]
Request: [INIT
]
Response: [OK
]
Bytes: [4]
Request: [READ PT3
]
Response: [OK
]
Bytes: [4]
Request: [READ PT4
]
Response: [OK
]
Bytes: [4]
Request: [REM PT3
]
Response: [OK
]
Bytes: [4]
ravi@dell:~/telnet$

Problems

Most of the issues are related to how the response is received. Below are the problems I am facing:

  1. The server returns "Welcome\r\n" upon a successful connection. Unfortunately, to grab the welcome message, the receive function, i.e., try_recv, is called twice, which does not look so nice. I think that the receive function should be able to collect the complete response in one call.
  2. After every send command, the receive function is called. If I do not call receive function, the program prints "connection has been closed.". In this situation, I have to restart the telnet server forcefully. Can a client ignore the response from a telnet server, or is it compulsory to receive? If it is mandatory, then I think that send function should call receive.
  3. The receive function prints the response on the terminal. Instead, I would like to return it as a string.
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

Error handling

You are already doing a lot of error handling, which is good, but you need to pay a little bit more attention to what kind of errors functions can return, and how to respond to them.

For example, send() can return -1 in situations that are recoverable. You should check errno, and if that is set to EINTR, you should retry sending the same data. Also, if send() returns zero, that is technically not an error, and you should just treat that as a short send, see also this StackOverflow question.

Also note that for many socket functions, only -1 is an error, and other negative numbers might be valid. So in main(), check for sock_fd == -1 instead of sock_fd < 0.

Exit codes

The proper exit code to signal an error is usually 1, not -1. Even better is to use EXIT_SUCCESS and EXIT_FAILURE instead of numeric codes.

Consider removing the typedefs

A typedef can be nice to reduce the amount of typing you have to do. If you use it for your own structs, it is usually fine. But consider not making typedefs for types from the system headers. The reason is that if someone else reads your code, and sees Timeval, they will wonder: "is this some custom made type, or just a typedef for struct timeval?" And then they have to search through the code to get an answer. If you just keep using struct timeval consistently, it's one less thing to think about.

Also, what is Sockaddr? Given the other two typedefs, one might assume it is equal to struct sockaddr, but in fact it's struct sockaddr_in. It would have been better to name it Sockaddr_in, but not making the typedef would have avoided this issue.

Call recv() in a loop

Just like you had to use a loop in send_all() because send() might not send all data in one go, recv() might not receive everything the peer sent in one go, thus you must implement a loop and keep receiving until you got all the data you want.

You might think, if the server calls send() once, shouldn't the client be able to call recv() once and receive everything that was sent? The issue though is that whatever you send() has to go on the network in packets. If you use a SOCK_STREAM socket with an IPv4 or IPv6 address, the connection will use the TCP protocol. This will treat all data that is being sent as one contiguous stream, and it will chop that stream up in to packets in a way that makes it efficient to traverse the network, but it does not have to correspond to how much you send() at a time. Furthermore, for IPv4 even routers between the sender and receiver can chop up the packets even more, called packet fragmentation. On the receiving side, there is also no guarantee how much you will recv() in one go; maybe it is however much there was in one network packet, but the kernel can buffer more or less than one packet.

So you have to treat the incoming data as a stream of bytes, and be able to handle recv() returning a random amount of bytes from that stream at a time. The only guarantee you have is that the bytes are received in the same order as they were sent.

Thus, if you want to receive a complete line, you need to write a loop that will call recv() and each time checks if you received a newline character. But you also be prepared to receive more than a single line.

Instead of doing all this yourself, you can use the POSIX function fdopen() to create a FILE * from a file descriptor, and then use fgets() or getline() to read a line at a time. The drawback is that those functions don't handle timeouts.

Unfortunately, to grab the welcome message, the receive function, i.e., try_recv, is called twice, which does not look so nice. I think that the receive function should be able to collect the complete response in one call.

The problem is how to determine that you got the "complete response". If you know that the initial response is two lines, you can just try receiving until you have gotten at least two newline characters.

Don't assume getaddrinfo() will return an IPv4 address

If you call getaddrinfo() with AF_UNSPEC, it can return IPv6 addresses. Don't make assumptions about the address family, and instead create the socket with the right address family like so:

if ((sock_fd = socket(addr_info->ai_family, addr_info->ai_socktype, addr_info->ai_protocol) == -1) {
    ...
}

Don't bind() outgoing sockets

You don't need to bind() outgoing sockets. If you call connect() and the local end is not bound yet, the kernel will do it for you automatically. You then also don't have to worry about providing a socket_addr.

\$\endgroup\$
7
  • \$\begingroup\$ Thank you very much for providing a complete review of the code. I have the following 2 points to say. 1) May I request you to please elaborate on the "Call recv() in a loop" section? 2) I am getting confused about "how do we realize that all data is read?" By saying all data, I mean the response for one command. For example, the complete welcome message has 2 lines. Furthermore, commenting any try_recv function shows "connection has been closed" that ultimately leads me restarting the server forcefully. It seems that the client must know when and how many times recv should be called. \$\endgroup\$
    – ravi
    Aug 3, 2022 at 8:20
  • 1
    \$\begingroup\$ I updated the answer, I hope with enough information to explain the problem. The gist is that the client cannot know how many times recv() should be called, as you cannot know how much data it will return each time it is called. If you know the response is two lines, you have to recv() until you've seen at two newline characters in the received data. \$\endgroup\$
    – G. Sliepen
    Aug 3, 2022 at 8:44
  • \$\begingroup\$ Thanks again for elaborating on the requested section. The fdopen(), fgets(), and getline() are new for me, so I am reading man pages shared by you. By the way, let me reply to your comments to ensure we are on the same page. 1) "the client cannot know how many times recv() should be called, as you cannot know how much data it will return each time it is called." I agree with you. 2) "If you know, the response is two lines..." No, I don't know. Assuming I don't know that response contains two lines, I wish to modify the "try_recv" function so that the welcome message requires only one call. \$\endgroup\$
    – ravi
    Aug 3, 2022 at 10:12
  • \$\begingroup\$ Is there anything else that indicates the end of the welcome message? Maybe it's an empty line at the end? If there is nothing to indicate the end of the message, then I suggest not waiting for the welcome message, but immediately send the first command, and then read until you get the response for the command (assuming it can easily be distinguished, for example because it is always "OK" or "ERROR", or something like that). \$\endgroup\$
    – G. Sliepen
    Aug 3, 2022 at 10:35
  • \$\begingroup\$ "Maybe it's an empty line at the end?" Yes, there is an empty line at the end. Please see the output section in the question above. BTW, assume this client connects to another telnet server. Let's say the welcome message is only 1 line. In this situation, my code may wait forever because the recv function waits for a message to arrive when no messages are available at the socket. \$\endgroup\$
    – ravi
    Aug 3, 2022 at 11:37

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.