Review internet connection pinging method

Question

This is some C code I have to simply test the internet connection. Any comments/tips on efficiency and refactoring this program down would be greatly appreciated.

int testConnection(void)
{
    int status;
    struct addrinfo host_info;
    struct addrinfo *host_info_list;
    memset(&host_info, 0, sizeof host_info);
    #ifdef DEBUG
    fprintf(stdout,"Setting up the structs...");
    #endif
    host_info.ai_family = AF_UNSPEC;     // IP version not specified. Can be both.
    host_info.ai_socktype = SOCK_STREAM; // Use SOCK_STREAM for TCP or SOCK_DGRAM for UDP.

    status = getaddrinfo("www.google.com", "80", &host_info, &host_info_list);
    if (status != 0)  fprintf(stdout, "Address info error:: %s\n", gai_strerror(status));

    #ifdef DEBUG
    fprintf(stdout, "Creating a socket...\n");
    #endif
    int socketfd ;
    socketfd = socket(host_info_list->ai_family, host_info_list->ai_socktype, host_info_list->ai_protocol);
    if (socketfd == -1)  fprintf(stderr, "Socket error\n");

    #ifdef DEBUG
    fprintf(stdout, "Connecting...");
    #endif
    status = connect(socketfd, host_info_list->ai_addr, host_info_list->ai_addrlen);
    if (status < 0) fprintf(stderr, "Error while connecting.\n");

    #ifdef DEBUG
    fprintf(stdout, "Sending message...\n");
    #endif
    const char *msg = "GET / HTTP/1.1\nhost: www.google.com\n\n";
    int len = strlen(msg);
    ssize_t bytes_sent = send(socketfd, msg, len, 0);
    if (bytes_sent == 0) fprintf(stderr, "No bytes sent.\n");
    #ifdef DEBUG
    fprintf(stdout, "Bytes sent: %d\n", bytes_sent);
    fprintf(stdout, "Waiting to recieve data...\n");
    #endif
    char incomming_data_buffer[1000];
    ssize_t bytes_recieved = recv(socketfd, incomming_data_buffer,1000, 0);
    // If no data arrives, the program will just wait here until some data arrives.
    if (bytes_recieved == 0) fprintf(stderr, "Host shut down.\n");
    if (bytes_recieved == -1) fprintf(stderr, "Recieve error.\n");
    incomming_data_buffer[bytes_recieved - 2] = '\0';
    #ifdef DEBUG
    fprintf(stdout, "Bytes recieved: %d\n", bytes_recieved);
    fprintf(stdout, "%s\n", incomming_data_buffer);
    fprintf(stdout, "Receiving complete. Closing socket...\n");
    #endif
    freeaddrinfo(host_info_list);
    close(socketfd);
    #ifdef DEBUG
    fprintf(stdout, "Socket closed.\n");
    #endif
    return 0;
}

Answer 1

Some comments, mostly minor:

I would extract the server connection to a function:

static int connect_server(const struct addrinfo *host_info)
{
    struct addrinfo *host_info_list;
    int fd = -1;
    int status = getaddrinfo(...);
    while(...) {
        fd = socket(...);
        status = connect(...);
    }
    freeaddrinfo(host_info_list);
    return fd;
}

---

All that DEBUG stuff is distracting. Maybe it is temporary, but if you wanted to leave it in, I suggest extracting it:

#include <stdarg.h>

static inline void debug(const char *format, ...)
{
#ifdef DEBUG
    va_list ap;
    va_start(ap, format);
    vfprintf(stdout, format, ap);
    va_end(ap);
#endif
}

and calling it:

debug("Bytes recieved: %ld\n%s\nReceiving complete. Closing socket...\n",
      bytes_recieved,
      incomming_data_buffer);

If DEBUG is undefined, the inline debug function will be empty and will be excluded during compilation - it disappears.

---

You clearly need to loop to read the whole message... After reading you throw away the last two bytes.

buffer[bytes_recieved - 2] = '\0';

The recv call filled the buffer, so to \0 terminate properly you need to specify a smaller buffer:

char buffer[1000];
ssize_t bytes_recieved = recv(socketfd, buffer, sizeof buffer - 1, 0);
...
if (bytes_recieved > 0) {
    buffer[bytes_recieved] = '\0';
}

Note the use of sizeof instead of an explicit 1000

---

And some other things...

• The man-page for getaddrinfo suggests looping through the list of addresses returned instead of just using the first (in case the first does not work).

• When something fails you should exit rather than continuing.

• instead of using strlen on a constant string, use sizeof:

  const char msg[] = "GET / HTTP/1.1\nhost: www.google.com\n\n";
  ssize_t bytes_sent = send(socketfd, msg, sizeof msg - 1, 0);
  

  note the msg[], not *msg

• perhaps use perror or strerror on failure to read etc.

• camelCase function name but separate_word names elsewhere.