Common client server implementation

Question

I have created a common C file for a basic client server. It works fine on my Linux setup. Please provide your comments/suggestions on how the code can be optimized/enhanced.

After compiling we need to give parameters "s" or "c" to executable from the command line to run it as a server or client respectively. The client connects to the server at the default IP of 192.168.7.2.

#include"stdio.h"  
#include"stdlib.h"  
#include"sys/types.h"  
#include"sys/socket.h"  
#include"string.h"  
#include"netinet/in.h"  
#include"pthread.h"  
#include"netdb.h"
#include"errno.h"
#include"signal.h"

#define PORT 4444  
#define BUF_SIZE 2000  
#define CLADDR_LEN 100
#define MAX_ALLOWED_CONNECTIONS 5
char * whoAmI;
int sockfd, len, ret, newsockfd;

void * receiveMessage(void * socket) {  
 int sockfd, ret;  
 char buffer[BUF_SIZE];   
 sockfd = (int) socket;  
 memset(buffer, 0, BUF_SIZE);    
 for (;;) {  
  memset(buffer, 0, BUF_SIZE);
  ret = recvfrom(sockfd, buffer, BUF_SIZE, 0, NULL, NULL);    
  if (ret < 0) {    
   printf("Error receiving data!\n");      
  } else {
   if(*whoAmI == "c") {
        printf("server: ");
    }
    else {
        printf("client: ");
    }
   fputs(buffer, stdout);  
   //printf("\n");
  }    
 }  
}  

void  INThandler(int sig)
{
    printf("Closig all sockets");
    close(newsockfd);  
    close(sockfd);  
    exit(0);
} 
void main(int argc, char**argv) {  
 struct sockaddr_in addr, cl_addr;  

 char buffer[BUF_SIZE];  
 pid_t childpid;  
 char clientAddr[CLADDR_LEN];  
 pthread_t rThread;  

 /* install signal handler to close sockets on ctrl+c from user */
 signal(SIGINT, INThandler);

if (argc < 2) {  
  printf("Needs arguments: 0==Server; 1==client\n");  
  exit(1);    
}  

whoAmI = argv[1];

printf("whoami = %s \n", whoAmI);
 sockfd = socket(AF_INET, SOCK_STREAM, 0);  
 if (sockfd < 0) {  
  printf("Error creating socket!\n");  
  exit(1);  
 }  
 printf("Socket created...\n");  

 memset(&addr, 0, sizeof(addr));  

// start of server   
if ((strcmp(whoAmI, "s") == 0) ) {
 addr.sin_family = AF_INET;  
 addr.sin_addr.s_addr = INADDR_ANY;  
 addr.sin_port = PORT;

/* Naming the socket */
 //strcpy(addr.sun_path,"server_socket");

     ret = bind(sockfd, (struct sockaddr *) &addr, sizeof(addr));  
     if (ret < 0) { 
      printf("%s",strerror(errno));
      printf("Error binding!\n");  
      exit(1);  
     }  
     printf("Binding done...\n");  

     printf("Waiting for a connection...\n");  
     listen(sockfd, MAX_ALLOWED_CONNECTIONS);  

     len = sizeof(cl_addr);  
     newsockfd = accept(sockfd, (struct sockaddr *) &cl_addr, &len);  
     if (newsockfd < 0) {  
      printf("Error accepting connection!\n");  
      exit(1);  
     }   

     inet_ntop(AF_INET, &(cl_addr.sin_addr), clientAddr, CLADDR_LEN);  
     printf("Connection accepted from %s...\n", clientAddr);   
    }//end of server
    else {
//start of client
    addr.sin_family = AF_INET;    
     addr.sin_addr.s_addr = inet_addr("192.168.7.2");
     addr.sin_port = PORT;
           ret = connect(sockfd, (struct sockaddr *) &addr, sizeof(addr));    
         if (ret < 0) {    
          printf("Error connecting to the server!\n");    
          exit(1);    
         }    
         printf("Connected to the server...\n");
    //very important step
    newsockfd = sockfd;    
// end of client
    }

 memset(buffer, 0, BUF_SIZE);  
 printf("Enter your messages one by one and press return key!\n");  

 //creating a new thread for receiving messages from the client  
 ret = pthread_create(&rThread, NULL, receiveMessage, (void *) newsockfd);  
 if (ret) {  
  printf("ERROR: Return Code from pthread_create() is %d\n", ret);  
  exit(1);
 }  

 while (fgets(buffer, BUF_SIZE, stdin) != NULL) {  
  ret = sendto(newsockfd, buffer, BUF_SIZE, 0, (struct sockaddr *) &cl_addr, len);    
  if (ret < 0) {    
   printf("Error sending data!\n");    
   exit(1);  
  }  
 }     

 close(newsockfd);  
 close(sockfd);  

 pthread_exit(NULL);  
 return;  
}

Answer 1

Who am I?

When skimming the code, it's not obvious to a reader what the significance of checking whether a string starts with the letter "c".

if(*whoAmI == "c") {
    ...
} else {
    ...
}

In addition, this construct introduces bugs – what if the user were to type something other than "c" or "s"?

I think you should instead parse the argument once and define an enum to be tested against.

enum running_mode { SERVER, CLIENT };
enum running_mode running_mode;

// ...

if (0 == strcmp(argv[1], "c") {
    running_mode = CLIENT;
} else if (0 == strcmp(argv[1], "s") {
    running_mode = SERVER;
} else {
    printf("Argument must be 'c' or 's'\n");
    exit(1);
}

// ...

if (running_mode == CLIENT) {
    // handle client case
} else {
    // handle server case
}

You should also provide a more helpful message if there are not 2 arguments to the program. "0==Server; 1==client" seems a bit misleading.

Commented out code

I consider leaving commented out lines in your code without explanation a bad practise. Let me quote @nhgrif in this answer:

Source control should help you keep track of code that used to be there, so there's not a real good excuse to leave it there for any historical reason.

Arguably, you might want to leave it in if it's something you're frequently uncommenting for some sort of testing purposes, but if that's the case, perhaps leave a comment above the line, something to the effect of:

// Uncomment the following line to ... 

Because you use source control, right?

Indentation

The indentation in the posted code is a mess. I don't know if that's a consequence of copy-pasting the code or if it actually looks that way on your machine. To get any kind of grip on your code I had to run it through an auto-indent program first.

If you don't know the common practices, here is a general rule of thumb: whenever there is a {, increase indentation by one, and whenever there is a }, decrease indentation again. Other lines in the code should use the same level of indentation as the previous line.

Complex main

The main function is quite long and complex. You have acknowledged this yourself by leaving the comments "start of server", "end of server" etc in the code. Instead of this, I recommend refactoring the server and client specific code into separate functions. Any common data could then be passed to these functions by main.

Void main

You declare main as returning void, and then use exit(1) to signal an unsuccessful result to the OS. I think you should instead declare int main and use a return statement instead of exit.

Signal handler

The behavior of INThandler is undefined, because only very specific standard library functions can be called from signal handlers (Cppreference).

If the signal handler is called NOT as a result of abort or raise (in other words, the signal handler is asynchronous), the behavior is undefined if the signal handler calls any function within the standard library, except

• abort
• _Exit
• quick_exit
• signal with the first argument being the number of the signal currently handled (async handler can re-register itself, but not other signals).
• atomic functions from stdatomic.h if the atomic arguments are lock-free
• atomic_is_lock_free (with any kind of atomic arguments)

The program might still work on your machine, but is not guaranteed to do so on any box.

On the other hand, you don't need this signal handler at all. When a program terminates on modern operating systems, resources like open files are automatically released by the OS. The only purpose I see the handler serving is printing out "Closing all sockets", which seems a bit useless to me.

Implicit delcarations

You've forgot to include some header files.

• unistd.h containing close.
• arpa/inet.h conataing inet_ntop and inet_addr.

Global variables

Use of global variables is generally frowned upon. They make programs much harder to read and reason about. Of course they are sometimes required, but in your case they could just as easily be local variables passed around as function parameters.

Type correctness

The type of len should be socklen_t, not int. If not that, at least an unsigned integer type.

Buffer size

You define a macro BUF_SIZE with an arbitrary value (I assume). There's no need to do this as stdio.h already has a macro specifically for this purpose called BUFSIZ. The value of the macro "is chosen on each system so as to make stream I/O efficient" (GNU Manual).

Conclusion

I think this is a great beginner project! Writing network code is always interesting. To summarize most of my points above, code is read more times than written, so think about the reader!