Client server application for file transfer

Question

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 fileWrite functions:

void loader(int val, int max, int size, int row){
 static int lastPrinted = 0;
 int percent = (100.*val)/max;
 int count = ((float)percent * size) / 100;
 if(count == lastPrinted)
 return;
 if(row > 0){
 printf("033円[s"); //save current position
 printf("033円[%dB\n", row);
 }
 lastPrinted = count;
 printf("\r[");
 int i;
 for(i = 0; i < count; i++)
 printf("#");
 for(; i < size; i++)
 printf(".");
 printf("] %d%%", percent);
 if(row > 0)
 printf("033円[u");
 fflush(stdout);
}
static pthread_mutex_t STDOUT_mutex = PTHREAD_MUTEX_INITIALIZER;
void* fileWrite(void *fd){
 struct fileWriteArgs *p = (struct fileWriteArgs*)fd;
 int sockfd = p->sockfd;
 int childNumber = p->childNumber;
 char sz[32];
 read(sockfd, sz, 32);
 int fileSize = atoi(sz);
 char fileNameSize;
 int readed = read(sockfd, &fileNameSize, 1);
 if(readed < 0)
 errorReadingFromSocket();
 else if(fileNameSize == 0)
 errorBadFileName();
 char filename[256];
 readed = read(sockfd, filename, fileNameSize);
 if(readed < 0)
 errorReadingFromSocket();
 FILE *f = fopen(filename, "wb");
 if(f == NULL){
 errorOpeningFile();
 }
 char buffer[BUFFER_SIZE];
 int n = 0;
 while(n < fileSize){
 readed = read(sockfd, buffer, BUFFER_SIZE);
 if(readed < 0)
 errorReadingFromSocket();
 write(fileno(f), buffer, readed);
 n += readed;
 pthread_mutex_lock(&STDOUT_mutex);
 loader(n, fileSize, LOADER_LENGTH, childNumber);
 pthread_mutex_unlock(&STDOUT_mutex);
 }
 fclose(f);
 return NULL;
}

This is the client function:

int clientFileTransfer(char *ip, int port, char* filename){
 int sockfd = socket(AF_INET, SOCK_STREAM, 0);
 if(sockfd < 0)
 errorOpeningSocket();
 struct hostent* server = gethostbyname(ip);
 if(server == NULL)
 noServerFound();
 struct sockaddr_in serv_addr;
 memset((char*)&serv_addr, 0, sizeof(serv_addr));
 serv_addr.sin_family = AF_INET;
 serv_addr.sin_port = htons(port);
 bcopy((char*) server -> h_addr, (char*) &serv_addr.sin_addr.s_addr, server -> h_length);
 if(connect(sockfd, (struct sockaddr*)&serv_addr, sizeof(serv_addr)) < 0)
 errorConnecting();
 fileRead(filename, sockfd);
 return 0;
}

And the fileRead function:

void fileRead(char *filename, int sockfd){
 FILE *f = fopen(filename, "rb");
 if(f == NULL){
 errorOpeningFile();
 return;
 }
 fseek(f, 0, SEEK_END);
 int fileSize = ftell(f);
 fseek(f, 0, SEEK_SET);
 char size[32];
 sprintf(size, "%d", fileSize);
 write(sockfd, size, 32);
 char *name = strrchr(filename, '/');
 if(name == NULL)
 name = filename;
 else
 name++;
 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);
 int n = 0; //total bytes read/written
 char buffer[BUFFER_SIZE];
 while(!feof(f)){
 int read = fread(buffer, 1, BUFFER_SIZE, f);
 if(read < 0)
 errorReadingFromFile();
 write(sockfd, buffer, read);
 n+=read;
 loader(n, fileSize, LOADER_LENGTH, 0);
 }
 fclose(f);
}

The clientFileTransfer and fileRead functions are used by the client in a similar manner to serverFileTransfer and fileWrite, that are used by server.

As the code is pretty large, I would like some parts to be reviewed especially, as I feel there is something wrong there. My primary concern is the loader function. It has a static variable that keeps the value that was written last time when the function was called (to avoid printing the same thing all over again). I do not know if this is a good thing. Also this is my first multithreaded application, so I would like some remarks about the way that multithreading is implemented here.

Of course, I would really appreciate if I get also a review of the other parts of this application and remarks about it (what I have done right and where something is wrong).

• Paul92: I know that and I specified the parts that I am interested in, but I tought it will be nice if I show the whole thing, maybe I will get a superficial remark at least.

Answer 1

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.

• Paul92: You have no reason to apologize. I appreciate your answer and it is really helpful. You said that you would use SIZE_BUFFER_SIZE only once, and then use sizeof operator. Why?
• William Morris: Just that the size of sz is guaranteed to be given by sizeof sz, whereas it is not guaranteed to be SIZE_BUFFER_SIZE unless you really did dimension sz with that constant. In this case the two expressions are adjacent and it is clear, but in general they are not and it is easy for the definition and use to drift apart. BTW, SIZE_BUFFER_SIZE was just the first constant name that came to mind - you can probably improve it :-)