patterncMinor
Review internet connection pinging method
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
```
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:
All that DEBUG stuff is distracting. Maybe it is temporary, but if you
wanted to leave it in, I suggest extracting it:
and calling it:
If DEBUG is undefined, the inline
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.
The
specify a smaller buffer:
Note the use of
And some other things...
-
The man-page for
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
note the
-
perhaps use
-
camelCase function name but separate_word names elsewhere.
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 willbe 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 tospecify 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 1000And some other things...
-
The man-page for
getaddrinfo suggests looping through the list ofaddresses 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.