6
\$\begingroup\$

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;
}
\$\endgroup\$
8
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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