HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Review internet connection pinging method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pingingmethodinternetreviewconnection

Problem

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_buffe

Solution

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 

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.

Code Snippets

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;
}
#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
}
debug("Bytes recieved: %ld\n%s\nReceiving complete. Closing socket...\n",
      bytes_recieved,
      incomming_data_buffer);
buffer[bytes_recieved - 2] = '\0';
char buffer[1000];
ssize_t bytes_recieved = recv(socketfd, buffer, sizeof buffer - 1, 0);
...
if (bytes_recieved > 0) {
    buffer[bytes_recieved] = '\0';
}

Context

StackExchange Code Review Q#29805, answer score: 8

Revisions (0)

No revisions yet.