Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote this code to send any binary file from server to client (in our example, I am sending sample_file.txt); the client should recreate the file locally. Code works fine (I tested with one or two files). Data is sent in chunks of 256 bytes.

I would appreciate remarks and critique, however, not that much from programming style point of view, rather if there are some major flaws in the networking part/logic or file I/O.

Client:

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

int main(void)
{
    int sockfd = 0;
    int bytesReceived = 0;
    char recvBuff[256];
    memset(recvBuff, '0', sizeof(recvBuff));
    struct sockaddr_in serv_addr;

    /* Create a socket first */
    if((sockfd = socket(AF_INET, SOCK_STREAM, 0))< 0)
    {
        printf("\n Error : Could not create socket \n");
        return 1;
    }

    /* Initialize sockaddr_in data structure */
    serv_addr.sin_family = AF_INET;
    serv_addr.sin_port = htons(5000); // port
    serv_addr.sin_addr.s_addr = inet_addr("127.0.0.1");

    /* Attempt a connection */
    if(connect(sockfd, (struct sockaddr *)&serv_addr, sizeof(serv_addr))<0)
    {
        printf("\n Error : Connect Failed \n");
        return 1;
    }

    /* Create file where data will be stored */
    FILE *fp;
    fp = fopen("sample_file.txt", "ab"); 
    if(NULL == fp)
    {
        printf("Error opening file");
        return 1;
    }

    /* Receive data in chunks of 256 bytes */
    while((bytesReceived = read(sockfd, recvBuff, 256)) > 0)
    {
        printf("Bytes received %d\n",bytesReceived);    
        // recvBuff[n] = 0;
        fwrite(recvBuff, 1,bytesReceived,fp);
        // printf("%s \n", recvBuff);
    }

    if(bytesReceived < 0)
    {
        printf("\n Read Error \n");
    }


    return 0;
}

Server:

#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <sys/types.h>

int main(void)
{
    int listenfd = 0;
    int connfd = 0;
    struct sockaddr_in serv_addr;
    char sendBuff[1025];
    int numrv;

    listenfd = socket(AF_INET, SOCK_STREAM, 0);

    printf("Socket retrieve success\n");

    memset(&serv_addr, '0', sizeof(serv_addr));
    memset(sendBuff, '0', sizeof(sendBuff));

    serv_addr.sin_family = AF_INET;
    serv_addr.sin_addr.s_addr = htonl(INADDR_ANY);
    serv_addr.sin_port = htons(5000);

    bind(listenfd, (struct sockaddr*)&serv_addr,sizeof(serv_addr));

    if(listen(listenfd, 10) == -1)
    {
        printf("Failed to listen\n");
        return -1;
    }


    while(1)
    {
        connfd = accept(listenfd, (struct sockaddr*)NULL ,NULL);

        /* Open the file that we wish to transfer */
        FILE *fp = fopen("sample_file.txt","rb");
        if(fp==NULL)
        {
            printf("File opern error");
            return 1;   
        }   

        /* Read data from file and send it */
        while(1)
        {
            /* First read file in chunks of 256 bytes */
            unsigned char buff[256]={0};
            int nread = fread(buff,1,256,fp);
            printf("Bytes read %d \n", nread);        

            /* If read was success, send data. */
            if(nread > 0)
            {
                printf("Sending \n");
                write(connfd, buff, nread);
            }

            /*
             * There is something tricky going on with read .. 
             * Either there was error, or we reached end of file.
             */
            if (nread < 256)
            {
                if (feof(fp))
                    printf("End of file\n");
                if (ferror(fp))
                    printf("Error reading\n");
                break;
            }


        }

        close(connfd);
        sleep(1);
    }


    return 0;
}
share|improve this question

3 Answers 3

ALWAYS check the return values of C functions and take appropriate action.

Example:

::read()  returns the number of characters read.
          Just because you write in chunks of 256 does not mean they
          will arrive in chunks of.

::write() Same thing goes here.
          You may try and write in chunks of 256 does not mean
          that all 256 will be written.

Note. When they return -1 (error) the actual error is in the variable errno and not all errors are unrecoverable (see [EINTR]).

How I would use write

std::size_t    bytesWritten = 0;
std::size_t    bytesToWrite = <Some Size>;

while (bytesWritten != bytesToWrite)
{
    std::size_t    writtenThisTime;

    do
    {
         writtenThisTime = ::write(fd, buf + bytesWritten, (bytesToWrite - bytesWritten));
    }
    while((writtenThisTime == -1) && (errno == EINTR));

    if (writtenThisTime == -1)
    {
        /* Real error. Do something appropriate. */
        return;
    }
    bytesWritten += writtenThisTime;
}

How I would read:

std::size_t    bytesRead = 0;
std::size_t    bytesToRead = <Some Size>;

while (bytesToRead != bytesRead)
{
    std::size_t    readThisTime;

    do
    {
         readThisTime = ::read(fd, buf + bytesRead, (bytesToRead - bytesRead));
    }
    while((readThisTime == -1) && (errno == EINTR));

    if (readThisTime == -1)
    {
        /* Real error. Do something appropriate. */
        return;
    }
    bytesRead += readThisTime;
}

other unchecked calls:

fwrite(recvBuff, 1,bytesReceived,fp);
listenfd = socket(AF_INET, SOCK_STREAM, 0);
bind(listenfd, (struct sockaddr*)&serv_addr,sizeof(serv_addr));
connfd = accept(listenfd, (struct sockaddr*)NULL ,NULL);
int nread = fread(buff,1,256,fp);             // You may not get all 256 items.
share|improve this answer
    
Thanks. I thought everywhere I use read, I check return values, isn't it the case? For write, I use it once in server, and once in client - on both places I shall compare if number of bytes written is equal to the number of bytes I requested to write, and if not - abort program or smth, that is your suggestion right? –  user38434 Mar 10 at 11:33
    
@If the number of bytes written does not equal the number of bytes you requested just try again (starting from the first byte not written). Yes you do check the read count but you don't compensate for errors that are not errors. updated with how I would read/write. –  Loki Astari Mar 11 at 1:21
    
I will give more thought to your suggestions. But why is your write better than one I had, or just doing one write and checking error code? Basically, one could wrap your suggestion into a WRITE(buffer,size) function also, and use it instead of original write command too right? –  user38434 Mar 11 at 7:29
    
Because write does not guarantee to write all the bytes requested. The FD may be busy. Checking the error code is not enough you need to check how much was written. Yes you can wrap that in a function (and I would encourage that). Also see the list of functions that you do not correctly validate the return code on. –  Loki Astari Mar 11 at 9:10

I am not an expert in network so I will comment on the part that doesn't interest you that much. On top of that, your code is pretty clean, well documented and seemed to work properly when I tried it locally so there is not too much to say.

You should try to avoid magic numbers to keep things maintainable and any other kind of hardcoded data in the middle of the code.

You could put server and client in the same file in different methods.

You have unused variable. An easy way to notice it is to try to put declaration as late as possible, in the smallest possible scope and as close to their first use as possible. Also, compiler warnings might help you.

I am not sure you are cleaning ressources (files for instance) properly. This needs a lot of re-writing to do this properly so I'll leave this for you.

After a bit of tweaking here and there, here is the code I have on my side :

// For both
#include <sys/socket.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <sys/types.h>
#include <unistd.h>
#include <netinet/in.h>
#include <arpa/inet.h>

// For server
#include <netdb.h>

// For client


#define PORT 5000
#define BUF_SIZE 256

int client(const char* filename)
{
    /* Create file where data will be stored */
    FILE *fp = fopen(filename, "ab");
    if(NULL == fp)
    {
        printf("Error opening file");
        return 1;
    }

    /* Create a socket first */
    int sockfd = 0;
    if((sockfd = socket(AF_INET, SOCK_STREAM, 0))< 0)
    {
        printf("\n Error : Could not create socket \n");
        return 1;
    }

    /* Initialize sockaddr_in data structure */
    struct sockaddr_in serv_addr;
    serv_addr.sin_family = AF_INET;
    serv_addr.sin_port = htons(PORT); // port
    serv_addr.sin_addr.s_addr = inet_addr("127.0.0.1");

    /* Attempt a connection */
    if(connect(sockfd, (struct sockaddr *)&serv_addr, sizeof(serv_addr))<0)
    {
        printf("\n Error : Connect Failed \n");
        return 1;
    }

    /* Receive data in chunks of BUF_SIZE bytes */
    int bytesReceived = 0;
    char buff[BUF_SIZE];
    memset(buff, '0', sizeof(buff));
    while((bytesReceived = read(sockfd, buff, BUF_SIZE)) > 0)
    {
        printf("Bytes received %d\n",bytesReceived);
        fwrite(buff, 1,bytesReceived,fp);
    }

    if(bytesReceived < 0)
    {
        printf("\n Read Error \n");
    }

    return 0;
}


int server(const char * filename)
{
    int listenfd = socket(AF_INET, SOCK_STREAM, 0);

    printf("Socket retrieve success\n");

    struct sockaddr_in serv_addr;
    memset(&serv_addr, '0', sizeof(serv_addr));
    serv_addr.sin_family = AF_INET;
    serv_addr.sin_addr.s_addr = htonl(INADDR_ANY);
    serv_addr.sin_port = htons(PORT);

    bind(listenfd, (struct sockaddr*)&serv_addr,sizeof(serv_addr));

    if(listen(listenfd, 10) == -1)
    {
        printf("Failed to listen\n");
        return -1;
    }

    for (;;)
    {
        int connfd = accept(listenfd, (struct sockaddr*)NULL ,NULL);

        /* Open the file that we wish to transfer */
        FILE *fp = fopen(filename,"rb");
        if(fp==NULL)
        {
            printf("File opern error");
            return 1;
        }

        /* Read data from file and send it */
        for (;;)
        {
            /* First read file in chunks of BUF_SIZE bytes */
            unsigned char buff[BUF_SIZE]={0};
            int nread = fread(buff,1,BUF_SIZE,fp);
            printf("Bytes read %d \n", nread);

            /* If read was success, send data. */
            if(nread > 0)
            {
                printf("Sending \n");
                write(connfd, buff, nread);
            }

            /*
             * There is something tricky going on with read ..
             * Either there was error, or we reached end of file.
             */
            if (nread < BUF_SIZE)
            {
                if (feof(fp))
                    printf("End of file\n");
                if (ferror(fp))
                    printf("Error reading\n");
                break;
            }
        }
        close(connfd);
        sleep(1);
    }

    return 0;
}

int main(int argc, char** argv)
{
    if (argc == 3)
    {
        const char* mode = argv[1];
        const char* filename = argv[2];
        if (strcmp(mode, "client") == 0)
            return client(filename);
        else if (strcmp(mode, "server") == 0)
            return server(filename);
        else
            printf("Invalid mode %s - should be 'client' or 'server'\n",mode);
    }
    else
    {
        printf("Invalid number of argument, usage is %s [MODE] [FILENAME]\n",argv[0]);
    }
    return 1; // Something went wrong
}
share|improve this answer

If I had to pick one word about networking it would be "unreliable". Networks are not bullet-proof, not even in lab scenarios; things can fail: whole machines, network adapters, cables, routers, switches or the application at the "other" end. Your code does little to ensure that errors are handled properly and only relies on the (limited) guarantees that TCP provides you with.

What happens if the server crashes in the middle of a transfer? How do you know that the entire contents of the file has been transferred?

I'd start by designing a 'mini protocol' for the app: maybe the client should be able to ask for a specific file. How would you change the request (non-existent right now) and response? I'd also add a header to inform the client of how much data I'm going to send or maybe some sort of file CRC.

Regarding your choice of buffer size. TCP packets are usually much larger than 256 bytes - while not a big issue in your case, you may want to choose a better value MTU.

sleep - some call it evil. Why did you use it?

share|improve this answer
    
why would server send how many bytes it is going to send - to let the client verify that data transfer was success?? So you recommend sending CRC to let the client make sure it has successfully received file? Is that it? About sleep - some little part of this code, like basic client/server connection setup I found on internet, so it seems to be a relic from that ... –  user38434 Mar 10 at 12:07
    
@dmcr_code knowing the file size beforehand can be useful for several things: 1. client knows when to stop/how much to read with the added benefit of being able to detect (more reliably) if something went wrong (maybe cleanup incomplete files). 2. it could allow you to 'pre-allocate' the space for the file(s) instead of trashing your hdd with 256byte long writes (not even a full old-style 512 byte disk sector) - torrent clients, for instance, usually do that to avoid being limited by HDD write speed. –  bkdc Mar 10 at 13:52
    
@dmcr_code CRC doesn't replace file size though, in some way, it could be used like that. The main benefit CRCs provide is that they allow the client to verify that the data it received has not been corrupted or tampered with. –  bkdc Mar 10 at 13:57
    
@dmcr_code you should fclose the files once you're done with them –  bkdc Mar 10 at 14:01
    
Yes fclose I added. I agree CRC can increase reliability because I can check if file receive was success. On sending file size not convinced yet, but thanks for your feedback. –  user38434 Mar 10 at 14:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.