Send and receive functions for telnet client

Question

I am using libtelnet to design a telnet client by sending text commands to a telnet server and receiving text responses. I am utilizing telnet-client.c. For simplicity, I made send and receive functions. The complete code is shown 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

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 void _input(const char *buffer, int size) {
  /* printing here only for debugging */
  char msg[size + 1];
  strncpy(msg, buffer, size);
  msg[size] = '\0';
  printf("request: [%s]", msg);

  static char crlf[] = {'\r', '\n'};
  int i;

  for (i = 0; i != size; ++i) {
    if (buffer[i] == '\r' || buffer[i] == '\n') {
      telnet_send(telnet, crlf, 2);
    } else {
      telnet_send(telnet, buffer + i, 1);
    }
  }
  fflush(stdout);
}

static void _send(int sock, const char *buffer, size_t size) {
  int rv;

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

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

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

  switch (ev->type) {
    /* data received */
    case TELNET_EV_DATA:
      strncpy(msg, ev->data.buffer, ev->data.size);
      msg[ev->data.size] = '\0';
      /* printing here only for debugging */
      printf("response: [%s]", msg);
      break;
    /* data must be sent */
    case TELNET_EV_SEND:
      _send(sock, ev->data.buffer, ev->data.size);
      break;
    default:
      /* ignore */
      break;
  }
}

static void try_send(int sock, const char *cmd, size_t cmdlen) {
  fd_set writefd;

  // timeout 1 second
  struct timeval tv;
  tv.tv_sec = 1;
  tv.tv_usec = 0;

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

  // add our descriptors to the set
  FD_SET(sock, &writefd);

  int rv = select(sock + 1, NULL, &writefd, NULL, &tv);

  if (rv == -1) {
    perror("select");  // error occurred in select()
  } else if (rv == 0) {
    printf("timeout occurred!\n");
  } else {
    /* send the request */
    if (FD_ISSET(sock, &writefd)) {
      _input(cmd, cmdlen);
    }
  }
}

static void try_recv(int sock, char *buffer) {
  fd_set readfd;

  struct timeval tv;
  tv.tv_sec = 1;
  tv.tv_usec = 0;

  FD_ZERO(&readfd);
  FD_SET(sock, &readfd);

  int rv = select(sock + 1, &readfd, NULL, NULL, &tv);

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

int main() {
  char buffer[BUFFER_SIZE];
  int rv;
  int sock;
  struct sockaddr_in addr;
  struct addrinfo *ai;
  struct addrinfo hints;

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

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

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

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

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

  /* free address lookup info */
  freeaddrinfo(ai);

  /* initialize telnet box */
  telnet = telnet_init(telopts, _event_handler, 0, &sock);

  /* our server returns "welcome\r\n" upon a successful connection */
  try_recv(sock, buffer);
  try_recv(sock, buffer); /* the second recv is needed */


  /* define our commands and length */
  const char *cmd[] = {"INIT\n", "READ PT3\n", "READ PT4\n", "REM PT3\n"};
  const int len[] = {5, 9, 9, 8};

  int i;

  i = 0;
  try_send(sock, cmd[i], len[i]);
  try_recv(sock, buffer);

  i = 1;
  try_send(sock, cmd[i], len[i]);
  try_recv(sock, buffer);

  i = 2;
  try_send(sock, cmd[i], len[i]);
  try_recv(sock, buffer);

  i = 3;
  try_send(sock, cmd[i], len[i]);
  try_recv(sock, buffer);

  /* clean up */
  telnet_free(telnet);
  close(sock);

  return 0;
}

Run

The server returns "welcome\r\n" upon a successful connection. Please see below the complete log displayed on the terminal.

ravi@dell:~/telnet$ ./my_client 
response: [Welcome
]response: [
]request: [INIT
]response: [OK
]request: [READ PT3
]response: [OK
]request: [READ PT4
]response: [OK
]request: [REM PT3
]response: [OK
]ravi@dell:~/telnet$

Problems

I plan to make a wrapper of this code which will later be called from another (main) file. Most of my issues are related to how the response is received. Nevertheless, I feel that the design/code style should be improved. To this end, below are the problems I am facing:

1. The call to receive function (i.e., try_recv(sock, buffer)) is done twice in order to grab the welcome message. However, I expect the receive function to receive all the data in one call. Calling the receive function twice does not look nice to me.
2. Removing the second call or adding an extra receive call makes the program stop working next time. From next time, the program prints "connection has been closed.". In this situation, I have to restart the telnet server forcefully.
3. 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.
4. The receive function prints the response on the terminal. Instead, I would like to return it as a string.

Answer 1

General Observations

The code could be made more maintainable (easier to read, write, and debug) through reduction of redundant code and simplification.

There is a possible bug in static void try_recv(int sock, char* buffer), the variable rv is defined twice in 2 separate scopes, it would be better to only declare it once in the function.

Maintainability

It would be easier to add or delete commands in the cdm array if each command was on a separate line:

    const char* cmd[] = {
        "INIT\n",
        "READ PT3\n",
        "READ PT4\n",
        "REM PT3\n"
    };

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code multiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

It isn't clear why you aren't using a loop of some kind in main() for this code:

    const char* cmd[] = { "INIT\n", "READ PT3\n", "READ PT4\n", "REM PT3\n" };
    const int len[] = { 5, 9, 9, 8 };

    int i;

    i = 0;
    try_send(sock, cmd[i], len[i]);
    try_recv(sock, buffer);

    i = 1;
    try_send(sock, cmd[i], len[i]);
    try_recv(sock, buffer);

    i = 2;
    try_send(sock, cmd[i], len[i]);
    try_recv(sock, buffer);

    i = 3;
    try_send(sock, cmd[i], len[i]);
    try_recv(sock, buffer);

It also isn't clear why the second array, len exists, since you can simply use the strlen() function to get the information, let the computer do as much work as it can. The above code can be simplified to:

    const char* cmd[] = { "INIT\n", "READ PT3\n", "READ PT4\n", "REM PT3\n" };

    size_t cmd_count = sizeof(cmd) / sizeof(*cmd);
    int i;
    for (i = 0; i < cmd_count; i++)
    {
        try_send(sock, cmd[i], strlen(cmd[i]));
        try_recv(sock, buffer);

    }

Starting a Function Name with Underscore _

Function names that start with underscore are reserved, it would be much better if the functions _input(), _send() and _event_handler() did not start with underscores even though they are static and therefore not global functions. This stack overflow question also discusses the issue.

Use More Descriptive Variable Name

The variable names cmd, ai, rv, tv and others could be more descriptive. I understand that addrinfo is already the name of a struct, it would be much better if the header file included a typedef that capitalized the A and I so that you could use addrinfo as a variabble name as well.

Default Action in _event_handler()

The switch statement in _event_handler() doesn't handle possible errors, it might be better to list each possible case with a common break; statement and let default: handle the case where an unknown type is returned as an error.

Return Possible Errors

Both the try_send() function and the try_recv() function should return a status on success or failure, the initial code in main() could then loop until try_recv() completed successfully (returned a positive value).

Returning Status From main()

There are 2 documented system defined macros to return status from main, these are EXIT_SUCCESS and EXIT_FAILURE. These macros are defined in stdlib.h. They are also used in C++ programming. They help provide self documenting code.

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

Suggested Additional Function and Modifications to main()

static int make_connections()
{
    int rv;
    int sock;
    struct sockaddr_in addr;
    struct addrinfo* ai;
    struct addrinfo hints;

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

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

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

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

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

    /* free address lookup info */
    freeaddrinfo(ai);

    return sock;
}

int main() {
    char buffer[BUFFER_SIZE];
    int sock = make_connections();
    
    if (sock < 0)
    {
        return EXIT_FAILURE;
    }

    /* initialize telnet box */
    telnet = telnet_init(telopts, _event_handler, 0, &sock);

    /* our server returns "welcome\r\n" upon a successful connection */
    try_recv(sock, buffer);
    try_recv(sock, buffer); /* the second recv is needed */


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

    size_t cmd_count = sizeof(cmd) / sizeof(*cmd);
    int i;
    for (i = 0; i < cmd_count; i++)
    {
        try_send(sock, cmd[i], strlen(cmd[i]));
        try_recv(sock, buffer);

    }

    /* clean up */
    telnet_free(telnet);
    close(sock);

    return EXIT_SUCCESS;
}