Network Interface Object

Question

I wrote a Network... thing (not really telnet); and it's pretty simple. No options, just straight I/O. It seems programs get so bloated easily.

#include <arpa/inet.h>
#include <fcntl.h>
#include <netdb.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <unistd.h>

#define BUFLEN  1024

sig_atomic_t run = 1;
sig_atomic_t sd = 1;

char e_socket_msg[]     = "socket creation failed\n";
char e_sockopt_msg[]    = "set socket non-block failed\n";
char e_parse_msg[]      = "address parsing failed\n";
char e_timeout_msg[]    = "connection attempt timed out\n";
char e_io_msg[]         = "i/o error\n";
char e_generic_msg[]    = "unknown or unexpected error\n";
char e_resolve_msg[]    = "unable to resolve address\n";

typedef enum {
    e_resolve   = -1,
    e_socket    = -2,
    e_sockopt   = -3,
    e_parse     = -4,
    e_timeout   = -5,
    e_io        = -6
} Error;

void input(char *input, char *output, int len);
int resolve(char *host);
void sig_handler(int sig);
int connect_to(char *host, int port);
int transfer(int fd_in, char *buf, int buf_len, int fd_out);
int print_error(Error e);

int main(void) {
    fd_set fds;
    struct timeval tv;
    int rv;
    char buffer[BUFLEN];
    char host[64], port[16];
    char host_msg[] = "host:\t";
    char port_msg[] = "port:\t";

    input(host_msg, host, 64);
    input(port_msg, port, 16);

    sd = connect_to(host, atoi(port));
    if (sd < 0) {
        rv = resolve(host);
        if (rv < 0) return print_error(rv);
        sd = connect_to(host, atoi(port));
        if (sd < 0) return print_error(sd);
    }

    signal(SIGINT, sig_handler);
    signal(SIGPIPE, sig_handler);

    FD_ZERO(&fds);
    tv.tv_sec = 0;
    tv.tv_usec = 300000;
    while (run) {
        FD_SET(sd, &fds);
        FD_SET(STDIN_FILENO, &fds);
        rv = select(sd + 1, &fds, NULL, NULL, &tv);
        if (FD_ISSET(STDIN_FILENO, &fds))
            rv = transfer(STDIN_FILENO, buffer, BUFLEN, sd);
        else if (FD_ISSET(sd, &fds))
            rv = transfer(sd, buffer, BUFLEN, STDOUT_FILENO);
        if (rv != 0) {
            run = 0;
            if (rv > 0) print_error(e_io);
        }
    }
    close(sd);
    return 0;
}

void input(char *input, char *output, int len) {
    int rv;

    (void) write(STDOUT_FILENO, input, strlen(input));
    rv = read(STDIN_FILENO, output, len - 1);
    output[rv - 1] = '\0';
}

void sig_handler(int sig) {
    run = 0;
    close(sd);
}

int resolve(char *host) {
    struct addrinfo hints, *servinfo;
    struct in_addr addr;
    char *addr_tmp;
    int rv = 0;

    memset(&hints, 0, sizeof hints);
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_family = AF_INET;
    rv = getaddrinfo(host, NULL, &hints, &servinfo);
    if (rv) return print_error(e_resolve);
    addr.s_addr = ((struct sockaddr_in*)servinfo->ai_addr)->sin_addr.s_addr;
    addr_tmp = inet_ntoa(addr);
    memset(host, 0, 64);
    memcpy(host, addr_tmp, strlen(addr_tmp));
    freeaddrinfo(servinfo);
    return rv;
}

int connect_to(char *host, int port) {
    int sd;
    struct sockaddr_in addr;
    fd_set sfds;
    struct timeval tv;

    sd = socket(AF_INET, SOCK_STREAM, 0);
    if (sd == -1) return e_socket;
    if (fcntl(sd, F_SETFL, O_NONBLOCK) == -1) return e_sockopt;
    memset(&addr, 0, sizeof (addr));
    addr.sin_family = AF_INET;
    if (inet_pton(AF_INET, host, &addr.sin_addr) != 1)
        return e_parse;
    addr.sin_port = htons(port);
    connect(sd, (struct sockaddr *) &addr, sizeof (addr));
    FD_ZERO(&sfds);
    FD_SET(sd, &sfds);
    tv.tv_sec = 4;
    tv.tv_usec = 0;
    if (select(sd + 1, NULL, &sfds, NULL, &tv)) return sd;
    return e_timeout;
}

int transfer(int fd_in, char *buf, int buf_len, int fd_out) {
    int len = read(fd_in, buf, buf_len);
    return len > 0? len - write(fd_out, buf, len) : -1;
}

int print_error(Error e) {
    char *msg;

    switch (e) {
        case e_socket:
            msg = e_socket_msg;
            break;
        case e_sockopt:
            msg = e_sockopt_msg;
            break;
        case e_parse:
            msg = e_parse_msg;
            break;
        case e_timeout:
            msg = e_timeout_msg;
            break;
        case e_io:
            msg = e_io_msg;
            break;
        case e_resolve:
            msg = e_resolve_msg;
            break;
        default:
            msg = e_generic_msg;
            break;
    }
    (void) write(STDERR_FILENO, msg, strlen(msg));
    return -e;
}

Answer 1

• There is no guarantee that write writes out the entire buffer. This means that

  if (FD_ISSET(STDIN_FILENO, &fds))
      rv = transfer(STDIN_FILENO, buffer, BUFLEN, sd);
  

  may lose data. Consider looping until all data gone out.

• As long as the portability is concerned, the timeout parameter of select needs attention. The BSD implementation doesn't change it, Linux does (POSIX considers both compliant), WSA says nothing. It is prudent to re-initialize tv inside the loop.

• Getting host and port from stdin seriously restricts the utility of the program. It cannot be used as a filter. Consider passing them via command line arguments.

• Make up your mind on who is responsible for error reporting. For example, resolve does call print_error, and so does main on resolve's failure. The error message gots printed twice.

• geraddrinfo returns some valuable information in servinfo, which you effectively throw away - what was the reason to call it?. Still you should freeaddrinfo afterwards.

Answer 2

the posted code causes the compiler to output several warning messages.

When compiling, always enable the warnings, then fix those warnings.

Here is the compile statement and the resulting warning messages.

gcc -Wall -Wextra -Wconversion -pedantic -std=gnu11  -ggdb -c "untitled2.c" 


untitled2.c: In function ‘input’:
untitled2.c:86:37: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 rv = read(STDIN_FILENO, output, len - 1);
                                 ^~~

untitled2.c:86:10: warning: conversion to ‘int’ from ‘ssize_t {aka long int}’ may alter its value [-Wconversion]
 rv = read(STDIN_FILENO, output, len - 1);
      ^~~~

untitled2.c: In function ‘sig_handler’:
untitled2.c:90:22: warning: unused parameter ‘sig’ [-Wunused-parameter]
 void sig_handler(int sig) {
                  ^~~

untitled2.c: In function ‘connect_to’:
untitled2.c:127:27: warning: conversion to ‘uint16_t {aka short unsigned int}’ from ‘int’ may alter its value [-Wconversion]
 addr.sin_port = htons(port);
                       ^~~~

untitled2.c: In function ‘transfer’:
untitled2.c:138:32: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 int len = read(fd_in, buf, buf_len);
                            ^~~~~~~

untitled2.c:138:15: warning: conversion to ‘int’ from ‘ssize_t {aka long int}’ may alter its value [-Wconversion]
 int len = read(fd_in, buf, buf_len);
           ^~~~

untitled2.c:139:46: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 return len > 0? len - write(fd_out, buf, len) : -1;
                                          ^~~

untitled2.c:139:51: warning: conversion to ‘int’ from ‘ssize_t {aka long int}’ may alter its value [-Wconversion]
 return len > 0? len - write(fd_out, buf, len) : -1;