Basic socket library in C

I am trying to learn C and have written a very basic socket library. I would be interested in any general design and coding comments.

Note that at this stage I am not bothered about implementing the most efficient networking model. I just want to get a simple synchronous model working then I can progress from there.

This is a client library only and the most pressing problem is how to retrieve data. I have implemented a function for the caller to pass in a function pointer for a callback function for received data. But how do I retrieve the recv data in the library? Do I poll on a timer? First phase is Windows winsock. but want to extend to simple Unix based synchronous when windows code working.

basic_socket.h

#ifndef __BASIC_SOCKET_H__
#define __BASIC_SOCKET_H__

/* startup/shutdown */
int init();
void libcleanup();

int tcp_connect(const char* pcHost, int nPort);

int senddata(const char* data, size_t len);
void getdata();

/* setup client callback function for retrieved data */
void data_recv_callback(void (*mycallback_function)(const char*));

#endif /*__BASIC_SOCKET_H__ */

basic_socket.c

```
#include
#include
/ Not doing anything with linux yet /
#ifdef __linux
#include
#include
#include
#include
#elif WIN32
#include
#endif

SOCKET sockfd;
char buffer[4096];

typedef void (cbfunc) (const char s);

cbfunc g_cbfunc = 0;

/ helper function to connect to server /
SOCKET establish_connection(u_long nRemoteAddr, u_short nPort)
{
/ Create a stream socket /
struct sockaddr_in sinRemote;

sockfd = socket(AF_INET, SOCK_STREAM, 0);
if (sockfd != INVALID_SOCKET) {
sinRemote.sin_family = AF_INET;
sinRemote.sin_addr.s_addr = nRemoteAddr;
sinRemote.sin_port = nPort;
if (connect(sockfd, (struct sockaddr*)&sinRemote, sizeof(struct sockaddr_in)) == SOCKET_ERROR) {
sockfd = INVALID_SOCKET;
}
}

return sockfd;
}

int init() {

#ifdef WIN32
WORD ver = MAKEWORD( 1, 1 );
WSAD

If you have not done anything with linux then don't even include the header files.

This gives a user a false sense that this is correct which is unknowable until you test it.

#if WIN32
#include 
#else
/* Not doing anything with linux yet */
#error "Untested in this environment"
#endif

The trouble with the socketaddr_in type is that a lot of the data is unspecified. So you need to make sure you manually zero the whole structure before you use it:

struct sockaddr_in sinRemote;
bzero((char *) &sinRemote, sizeof(sinRemote));

I don't think the sockaddr_in is structured like you think. I don;t believe (I could be wrong on this) you can assume it is an integer. The on;y way I know of setting the destination host is

sinRemote.sin_addr.s_addr = nRemoteAddr;

// don't believe your is portable.
// This is the way I am used to doing it.

struct hostent *server = gethostbyname(nRemoteAddr);
bcopy((char *)server->h_addr, 
     (char *)&sinRemote.sin_addr.s_addr,
     server->h_length);

The port number is supposed to be in network byte order.

sinRemote.sin_port = nPort;

   // So you should write your code to make sure this always happens.

   sinRemote.sin_port = htons(nPort);

One assumes that SOCKET_ERROR is -1.

Personally I don't see a point in hiding this value inside a variable it makes the code harder to read. The documentation says connect returns -1 on an error.

if (connect(sockfd, (struct sockaddr*)&sinRemote, sizeof(struct sockaddr_in)) == SOCKET_ERROR) {

This is definitely not an assumption I would make:

u_long nRemoteAddr = *((u_long*)pHE->h_addr_list[0]);

What happens if it returns an IPV6 address? Will it fit? What happens in the future and our alien overlords require IPV12. Will you destroy the last hope of humanity because your code crashes while trying to infect the aliens spaceship with your windows machine.

This is undefined behavior if the amount of data read is 4096!

while((n = recv(sockfd, buffer, sizeof(buffer), 0)) > 0) {

// Buffer overflow if n is the size of the buffer.
  buffer[n]= 0;

This is not threaded code (as there are no locks). So there seems little point in making a copy of the data here.

rec = malloc(n+1);
  strncpy(rec, buffer, n+1);

  if(g_cbfunc)
    g_cbfunc(rec);

  free(rec);

Pass a pointer to the buffer to g_cbfunc(). If it needs a copy then it can make a copy itself. Otherwise you are wasting resources making a copy.

Since you have the length locally why not pass that information to g_cbfun() as a parameter. Otherwise it needs to scan the buffer looking for the zero byte (which if the data is binary and not text may be a problem).

Rather than using a NULL as the default pointer use a function that does nothing. That way there is no need to test for NULL.

typedef void (*cbfunc) (const char* s, size_t count);
 void doNothing(const char* s, size_t count) {}

 // Just make sure if sombody sets a NULL callback then you set doNothing.
 cbfunc g_cbfunc = &doNothing;

 while((n = recv(sockfd, buffer, sizeof(buffer), 0)) > 0)
 {
     g_cbfunc(buffer, n);
 }

StackExchange Code Review Q#14340, answer score: 4