HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Client server application for file transfer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fileapplicationclientforservertransfer

Problem

I have an application for transferring files from clients to a server. A client opens, transfers a file to the server and closes. The server is open and may receive multiple files. Also, when transferring a file, a loader bar is created on both server (the server must have some way to display multiple loaders simultaneously) and client. Also, every file transfer is handled in a separate thread on server.
This is a structure that will be used in server function:

struct fileWriteArgs{
    int childNumber;
    int sockfd;
};


Here is the server function (the main function just parses arguments and calls this):

int serverFileTransfer(int port, char* filename){

    int sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if(sockfd<0)
        errorOpeningSocket();

    struct sockaddr_in serv_addr, cli_addr;
    memset((char*) &serv_addr, 0, sizeof(serv_addr));

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

    if(bind(sockfd, (struct sockaddr*)&serv_addr, sizeof(serv_addr)) < 0)
        errorOnBinding();

    listen(sockfd, 5);

    socklen_t cilen = sizeof(cli_addr);

    pthread_t newThread;
    pthread_attr_t attr;
    pthread_attr_init(&attr);
    pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);

    int noOfChilds = 0;

    while(1){
        int newsockfd = accept(sockfd, (struct sockaddr*)&cli_addr, &cilen);
        noOfChilds++;

        struct fileWriteArgs args;
        args.sockfd = newsockfd;
        args.childNumber = noOfChilds;

        if(newsockfd < 0)
            errorOnAccept();

        pthread_create(&newThread, &attr, &fileWrite, &args);

    }

    pthread_attr_destroy(&attr);

    return 0;
}


Basically, it opens a socket and creates threads for file accept (note that I am using a Linux operating system). Also, it calls some error functions that I think are pretty explicit (they just write a message and call exit()).

Here are the loader and `fileW

Solution

Nice code in general. Many of my comments are a bit on the
nit-picking level, so apologies in advance.

Your method of error handling seems to be just to exit, so the return
values from various functions should probably be void. This method
of error handling is certainly easy but might be considered lazy :-) I
notice that you don't check for EINTR in any failed socket calls -
should it not be handled?

Some of the variable names are too long for my taste. newsockfd
for example in the accept loop could be just fd with no loss.

In fileRead and fileWrite

These functions seem misnamed. Clearly one does read a file and one
writes but I think names containing 'send' and 'receive' would be
clearer. Having clientFileTransfer 'connect' to the server socket
and then do a fileRead implies to me that it is receiving a file
when in fact it is sending!

Some details I noticed are that fileNameSize should be unsigned if
you want to handle names up to 255 chars long. And it would be better
to reject files with names longer than 255 instead of just adding a
warning comment.

unsigned char fileNameSize = strlen(name)+1;
write(sockfd, &fileNameSize, 1);  //WARNING! it works only for files
                                  //with at most 255 characters

write(sockfd, name, strlen(name)+1);


In the fileRead code above you could re-use fileNameSize in the
last write call rather than repeating the strlen call. And your
read and write calls here and elsewhere should really all be
error-checked (some are, some are not).

When writing the file to disk, I don't see a good reason for opening
the file buffered (fopen from stdio) but writing un-buffered
(write) - I would use fwrite.

A minor point is that the 32 used as the file-size buffer

char sz[32];
read(sockfd, sz, 32);
int fileSize = atoi(sz);


should really be a constant defined in a header shared between client
and server. And I would use it just once:

char sz[SIZE_BUFFER_SIZE];
read(sockfd, sz, sizeof sz);  // use sizeof
int fileSize = atoi(sz);


In serverFileTransfer the filename parameter is not used. I
would expect to see the check for the accepted socket being invalid
immediately after the accept call. Also this socket is never closed
(not here and not in the fileWrite thread.

In clientFileTransfer the socket is not closed.

Code Snippets

unsigned char fileNameSize = strlen(name)+1;
write(sockfd, &fileNameSize, 1);  //WARNING! it works only for files
                                  //with at most 255 characters

write(sockfd, name, strlen(name)+1);
char sz[32];
read(sockfd, sz, 32);
int fileSize = atoi(sz);
char sz[SIZE_BUFFER_SIZE];
read(sockfd, sz, sizeof sz);  // use sizeof
int fileSize = atoi(sz);

Context

StackExchange Code Review Q#38071, answer score: 2

Revisions (0)

No revisions yet.