Iterating a read() from a socket

Question

Is this the proper way to iterate over a read on a socket? I am having a hard time getting this to work properly. data.size is an unsigned int that is populated from the socket as well. It is correct. data.data is an unsigned char *.

if ( data.size > 0 ) {
    data.data = (unsigned char*)malloc(data.size);
    memset(&data.data, 0, data.size);
    int remainingSize = data.size;
    unsigned char *iter = data.data;
    int count = 0;
    do {
        count = read(connect_fd, iter, remainingSize);
        iter += count;
        remainingSize -= count;
    } while (count > 0 && remainingSize > 0);
}
else {
    data.data = 0;
}

Thanks in advance.

Answer 1

You need to check the return value from read before you start adding it to other values.

You'll get a zero when the socket reports EOF, and -1 on error. Keep in mind that for a socket EOF is not the same as closed.

Answer 2

Low level socket programming is very tedious and error prone. If you use C++ you should try to use higher level libraries like Boost or ACE.

I would also suggest to read C++ Network Programming: Mastering Complexity Using ACE and Patterns and C++ Network Programming: Systematic Reuse with ACE and Frameworks

Answer 3

Put the read as part of the while condition.

while((remainingSize > 0) && (count = read(connect_fd, iter, remainingSize)) > 0)
{
    iter += count;
    remainingSize -= count;
}

This way if it fails you immediately stop the loop.
It is very common pattern to use the read as part of the loop condition otherwise you need to check the state inside the loop which makes the code uglier.

Personally:
I would move the whole above test into a separate function for readability but your milage may very.

Also using malloc (and company) is going to lead to a whole boat of memory management issues. I would use a std::vector. This also future proofs the code when you modify it to start throwing exceptions, now it will also be exception safe.

So assuming you change data.data to have a type of std::vector<unsigned char> then

if ( data.size > 0 )
{
    std::vector<unsigned char>   buffer(data.size);

    unsigned char *iter = &buffer[0];
    while(...  read(connect_fd, iter, remainingSize) )
    {
        .....
    }

    ... handle error as required

    buffer.resize(buffer.size() - remainingSize);
    data.data.swap(buffer);
}

Answer 4

Keep in mind that read() calls are system calls and thus a source of possible blocking, and even if you use non-blocking I/O, are inherently heavyweight. I would recommend minimising them.

A good way to go that has always served me well in over a decade of BSD socket programming in C is to use non-blocking I/O and issue a FIONREAD ioctl() to get the total amount of data waiting at a given polling interval (assuming you're using some sort of synchronous I/O mux like select()) and then just read() that amount as many times as necessary to capture all of it, and then return the function for the moment until the next timer tick.