patterncMinor
Client server application for file transfer
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:
Here is the server function (the main function just parses arguments and calls this):
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
Here are the
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 `fileWSolution
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
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.
for example in the
In
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
and then do a
when in fact it is sending!
Some details I noticed are that
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.
In the
last
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 (
(
A minor point is that the 32 used as the file-size buffer
should really be a constant defined in a header shared between client
and server. And I would use it just once:
In
would expect to see the check for the accepted socket being invalid
immediately after the
(not here and not in the
In
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 methodof 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.
newsockfdfor example in the
accept loop could be just fd with no loss. In
fileRead and fileWriteThese 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 socketand then do a
fileRead implies to me that it is receiving a filewhen in fact it is sending!
Some details I noticed are that
fileNameSize should be unsigned ifyou 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 thelast
write call rather than repeating the strlen call. And yourread and write calls here and elsewhere should really all beerror-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. Iwould 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.