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

Client/server implementation in C (sending data/files)

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

Problem

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 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

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("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
#include
#include
#include
#include
#include
#include
#include
#include

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==NU

Solution

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 = ;

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 = ;

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.

Code Snippets

::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.
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;
}
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;
}
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.

Context

StackExchange Code Review Q#43914, answer score: 8

Revisions (0)

No revisions yet.