C - Client Server application

Question

I must create a client-server application. This application must calculate the average and variance of a sample. The client sent the number in this format: 2 3 4, where the first number mean how many numbers I can insert. If I insert more or less number than I have to enter The server signals the error. For ending the inserting of the number I can insert the number 0. If the data inserted are right the server sent to the client how many numbers he read. If the user insert only one data the server signals that is not possible calculating the variance.

server.c

#include <sys/socket.h>
#include <sys/types.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <stdio.h>
#include <netdb.h>
#include <ctype.h>

#define MAX_DIM 256

float average(int *numbers, int dim) {
    int i;
    float cnt=0;
    for(i=0; i<dim; i++)
        cnt+=numbers[i];
    return cnt/(float)dim;
}

float variance(int *numbers, int dim, float average) {
    int i;
    float cnt=0;
    for(i=0; i<dim; i++)
        cnt+=numbers[i]*numbers[i];
    return (1/(float)(dim-1))*(cnt-((float)dim*(average*average)));
}

int main(int argc, char const *argv[]) {

    const char MESSAGE[] = "Welcome, send me your data\n";
    char buffer[MAX_DIM];
    char *tok;
    int simpleSocket = 0, simplePort = 0, returnStatus = 0, q, qtot, *numbers, j, i, tmpval, *tmp, tmpvett[6], flag, connection;
    float av, var;
    struct sockaddr_in server;

    /* make sure we have a port number */
    if (argc != 2) {
        fprintf(stderr, "Use: %s <port number> \n", argv[0]);
        exit(EXIT_FAILURE);
    }


    /* create a streaming socket */
    simpleSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

    if(simpleSocket == -1) {
        fprintf(stderr, "Could not create a socket!\n");
        exit(EXIT_FAILURE);
    }
    else
        printf("Socked created!\n");


    /* retrieve the port number to listening */
    simplePort = atoi(argv[1]);


    /* set up the address structure */
    memset(&server, '\0', sizeof(server));
    server.sin_family = AF_INET;
    server.sin_addr.s_addr = htonl(INADDR_ANY); /* bind to local addresses */
    server.sin_port = htons(simplePort);


    /* bind to the address and port with our socket */
    returnStatus = bind(simpleSocket, (struct sockaddr *)&server, sizeof(server));

    if(returnStatus == 0)
        fprintf(stderr, "Bind completed!\n");
    else {
        fprintf(stderr, "Could not bind to address!\n");
        close(simpleSocket);
        exit(EXIT_FAILURE);
    }


    /* lets listen on the socket for connections */
    returnStatus = listen(simpleSocket, 5);

    if(returnStatus == -1) {
        fprintf(stderr, "Cannot listen on socket!\n");
        close(simpleSocket);
        exit(EXIT_FAILURE);
    }

    while(1) {
        /* set up variables to handle client connections */
        struct sockaddr_in clientName = {0};
        int simpleChildSocket = 0;
        unsigned int clientNameLength = sizeof(clientName);

        /* block on accept function call */
        simpleChildSocket = accept(simpleSocket, (struct sockaddr *)&clientName, &clientNameLength);

        if(simpleChildSocket == -1) {
            fprintf(stderr, "Cannot accept connections!\n");
            close(simpleSocket);
            exit(EXIT_FAILURE);
        }

        /* server operations */
        memset(buffer, '\0', sizeof(buffer));
        snprintf(buffer, sizeof(buffer), "OK %s", MESSAGE);
        write(simpleChildSocket, buffer, strlen(buffer));

        i=0, j=0, qtot=0, numbers=NULL, connection=1;
        do {
            do {
                flag=1;
                memset(buffer, '\0', sizeof(buffer));
                returnStatus = read(simpleChildSocket, buffer, sizeof(buffer));
                if(returnStatus<=0) {
                    fprintf(stderr, "Return status = %d\n", returnStatus);
                    connection=0;
                }

                for(i = 0; i < (int)strlen(buffer) && flag!=0 && connection==1; i++){
                    if((isdigit(buffer[i])==0 && isspace(buffer[i])==0) || isspace(buffer[0])!=0) {
                        flag=0;
                        memset(buffer, '\0', sizeof(buffer));
                        snprintf(buffer, sizeof(buffer), "ER Formato stringa non corretto\n");
                        write(simpleChildSocket, buffer, strlen(buffer));
                    }
                }
            } while(flag==0 && connection==1);
            if(connection==1) {
                tok=strtok(buffer, " ");
                q=atoi(tok);
                if(q>0 && q<7 && isalpha(*tok)==0) {
                    printf("Elaborazione degli elementi inseriti in corso...\n");
                    for(i=0; tok!=NULL && isalpha(*tok)==0;) {
                        tok=strtok(NULL, " ");
                        if(tok!=NULL && isalpha(*tok)==0) {
                            tmpval=atoi(tok);
                            if(tmpval>=0) {
                                if(i<q) {
                                    printf("Token[%d]: %d\n", i, tmpval);
                                    tmpvett[i]=tmpval;
                                }
                                i++;
                            }
                        }
                    }
                    memset(buffer, '\0', sizeof(buffer));
                    if((i<q || i>q) && tok==NULL) {
                        printf("ER Numero di campioni diverso da quelli indicati. L'ultima stringa inviata verrà scartata\n");
                        snprintf(buffer, sizeof(buffer), "ER Numero di campioni elaborati diverso da quelli indicati. L'ultima stringa inviata verrà scartatati\n");
                    }
                    else {
                        qtot+=q;
                        tmp=realloc(numbers, qtot*sizeof(int));
                        numbers=tmp;
                        for(i=0; i<q; i++, j++)
                            numbers[j]=tmpvett[i];
                        snprintf(buffer, sizeof(buffer), "DT %d\n", i);
                        printf("Invio stringa al client: %s", buffer);
                    }
                    write(simpleChildSocket, buffer, strlen(buffer));
                }
                else if((q<0 || q>6) && isalpha(*tok)==0) {
                    memset(buffer, '\0', sizeof(buffer));
                    snprintf(buffer, sizeof(buffer), "ER Quantità elementi inserita non valida\n");
                    write(simpleChildSocket, buffer, strlen(buffer));
                }
            }
        } while(q!=0 && connection==1);

        if(connection==1) {
            memset(buffer, '\0', sizeof(buffer));
            if(numbers==NULL) {
                snprintf(buffer, sizeof(buffer), "ER Non ci sono campioni su cui calcolare media e varianza\n");
                write(simpleChildSocket, buffer, strlen(buffer));
                close(simpleChildSocket);
            }
            else if(qtot>1) {
                for(i=0; i<qtot; i++)
                    printf("%d: %d\n", i, numbers[i]);
                printf("Calcolo media e varianza in corso...\n");
                av=average(numbers, qtot);
                var=variance(numbers, qtot, average(numbers, qtot));
                snprintf(buffer, sizeof(buffer), "MV %d %.2f %.2f\n", j, av, var);
                printf("Invio stringa al client: %s", buffer);
                printf("%s", buffer);
            }
            else {
                printf("Impossibile calcolare la varianza\n");
                snprintf(buffer, sizeof(buffer), "ER Non posso calcolare la varianza di %d campione\n", qtot);
            }
            write(simpleChildSocket, buffer, strlen(buffer));
        }
        if(numbers!=NULL) {
            free(numbers);
        }
        close(simpleChildSocket);
    }

    close(simpleSocket);

    return 0;

}

client.c

#include <stdio.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <arpa/inet.h>
#include <ctype.h>

int main(int argc, char *argv[]) {

    int simpleSocket = 0, simplePort = 0, returnStatus = 0, i;
    char buffer[256], *tok, *tmp, *ptrbuffer;
    struct sockaddr_in server;

    if(argc!=3) {
        fprintf(stderr, "Usage: %s <server> <port>\n", argv[0]);
        exit(EXIT_FAILURE);
    }

    /* create a streaming socket */
    simpleSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

    if(simpleSocket == -1) {
        fprintf(stderr, "Could not create a socket!\n");
        exit(EXIT_FAILURE);
    }
    else
        printf("Socked created!\n");


    /* retrieve the port number for connecting */
    simplePort = atoi(argv[2]);


    /* set up the address structure */
    memset(&server, '\0', sizeof(server));
    server.sin_family = AF_INET;
    if(strcmp(argv[1], "localhost")==0)
        server.sin_addr.s_addr = inet_addr("127.0.0.1");
    else
        server.sin_addr.s_addr = inet_addr(argv[1]);
    server.sin_port = htons(simplePort);


    /* connect to the address and port with our socket */
    returnStatus = connect(simpleSocket, (struct sockaddr *)&server, sizeof(server));

    if (returnStatus==0)
        printf("Connect successfull!\n");
    else {
        fprintf(stderr, "Could not connect to address!\n");
        close(simpleSocket);
        exit(EXIT_FAILURE);
    }

    /* client operations */
    memset(buffer, '\0', sizeof(buffer));
    returnStatus = read(simpleSocket, buffer, sizeof(buffer));
    ptrbuffer=strdup(buffer);
    tmp = ptrbuffer;
    tok=strsep(&ptrbuffer, " ");
    if(returnStatus>0 && strcmp(tok, "OK")==0) {
        printf("%s", ptrbuffer);
    }
    else {
        fprintf(stderr, "Return status = %d\n", returnStatus);
        free(tmp);
        close(simpleSocket);
        exit(EXIT_FAILURE);
    }
    free(tmp);

    do {
        memset(buffer, '\0', sizeof(buffer));
        printf("\nInserisci la quantità di numeri da inviare al server seguita poi dai valori su cui effettuare il calcolo --> ");
        fgets(buffer, sizeof(buffer), stdin);
        write(simpleSocket, buffer, strlen(buffer));
        if(strcmp(buffer, "0\n")!=0) {
            memset(buffer, '\0', sizeof(buffer));
            returnStatus = read(simpleSocket, buffer, sizeof(buffer));
            ptrbuffer=strdup(buffer);
            tmp = ptrbuffer;
            tok=strsep(&ptrbuffer, " ");
            if(returnStatus>0 && strcmp(tok, "ER")==0) {
                printf("\n%s\n", ptrbuffer);
            }
            else if(returnStatus>0 && strcmp(tok, "DT")==0) {
                printf("\nCampioni elaborati: %s\n", ptrbuffer);
            }
            else {
                fprintf(stderr, "Return status = %d\n", returnStatus);
                free(tmp);
                close(simpleSocket);
                exit(EXIT_FAILURE);
            }
            free(tmp);
        }
    } while(strcmp(buffer, "0\n")!=0);

    if(strcmp(buffer, "0\n")==0) {
        memset(buffer, '\0', sizeof(buffer));
        returnStatus = read(simpleSocket, buffer, sizeof(buffer));
        ptrbuffer=strdup(buffer);
        tmp = ptrbuffer;
        tok=strsep(&ptrbuffer, " ");
        if(returnStatus>0 && strcmp(tok, "MV")==0) {
            for(i=0; ptrbuffer!=NULL; i++) {
                tok=strsep(&ptrbuffer, " ");
                switch(i) {
                    case 0:
                        printf("\nCampioni: %s ", tok);
                        break;
                    case 1:
                        printf("Media: %s ", tok);
                        break;
                    case 2:
                        printf("Varianza: %s\n", tok);
                        break;
                }
            }
        }
        else if(returnStatus>0 && strcmp(tok, "ER")==0) {
            printf("\n%s\n", ptrbuffer);
        }
        else {
            fprintf(stderr, "Return status = %d\n", returnStatus);
            close(simpleSocket);
            exit(EXIT_FAILURE);
        }
        free(tmp);
    }
    printf("\nChiusura client in corso...\n");
    close(simpleSocket);
    return 0;
}

Answer 1

Welcome to Code Review, a very nice first question. I commend you for the ability to be able to debug rather complex code.

Just a few observations to help out:

Bug in the Code

Since the while(1) never exits, the call to close simpleSocket is never executed, since it is outside the while(1) loop. There doesn't seem to be a signal handler that catches the kill signal and closes the socket.

Use Space to Enhance Readability

The readability of the code might be improved if there were spaces between the operators and the operands in all mathematical expressions. Vertical space can also make the code more readable and easier to maintain.
Existing Code:

float average(int *numbers, int dim) {
    int i;
    float cnt=0;
    for(i=0; i<dim; i++)
        cnt+=numbers[i];
    return cnt/(float)dim;
}

More Readable Code:

double average(int *numbers, int dim) {
    double cnt = 0.0;

    for (int i = 0; i < dim; i++)
        cnt += (double) numbers[i];

    return cnt / (double) dim;
}

Please also note that the integer loop variable i can be created in the loop, it does not need to be created at the top of the function. Limit the scope of the variables as much as possible. Since the C99 standard it is possible to declare variables where they are needed rather than at the top of a block of code, this can be quite helpful when writing or debugging code, it is more useful and clearer to everyone who may need to maintain the code when the variable is created as close to its use as possible.

The use of double over float will improve accuracy.

Prefer explicit casts from integer to float or integer to double over implicit casts, this makes it clearer to whoever needs to maintain the code. It will also occasionally reduce warning messages.

Reduce Complexity, Follow SRP

The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

Robert C. Martin expresses the principle as follows:
    `A class should have only one reason to change.`

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines in procedural languages like C as well.

There is a design methodology for procedural languages called Top Down Design. This concept is basically that the task of the program should be broken up into smaller and smaller pieces until it is very easy to implement each sub-task. Each atomic sub-task should be a function. A second reference can be found here. By break code into atomic functions the code become much more readable and bugs are much easier to find if they exist.

Another reason to break up code into small atomic functions is to reduce the scope of the variables.

If the main function in server.c had been broken up into smaller functions the bug mentioned above might have been caught earlier. It also might have been easier to write and debug this program.

The main(int argc, char *argv[]) function in almost all C programs should be a control mechanism but not the actual implementation. The main should function should make sure the program is set up properly, call the main implementation function and call any clean up functions before exiting, but that's really all main should do.

The main functions in both server.c and client.c can be broken up into multiple functions.

Sub-functions that might help the main function in server.c:

• A function to report errors and quit the program.
• A function to create the socket including the error handling.
• A function to bind the socket and perform any error handling.
• A function that contains all the functionality of the while(1) loop (which should also primarily call sub functions).
• Within the while(1) loop candidates for sub functions are:
• A function to set up the variables for connections.
• The outer do while loop should clearly be at least one function and probably three functions.
• The inner do while loop

• After the outer do while loop, the block of code that starts with

      if(connection==1) {
  

  is clearly a candidate to be at least one function and possibly two.

Inconsistent Variable Naming Conventions

Within the server.c: main() function there are the variable declarations av, var, q, qtot, tmpval, tmp and flag, but there are also much more descriptive variable names such as simpleChildSocket, simpleSocket and returnStatus. It would be better to make all of the variable names as descriptive as these last 3. I understand that there is av has a naming collision with the average function and var has a naming collision with the variance function but it would be better if they were named more clearly. I have no idea what q, qtot, tmpvett, tmpval or flag really represent. The point of descriptive variables is to make the code more readable so that comments like create a streaming socket or set up the address structure aren't necessary.