Read function that properly handles interrupts

Question

This function is meant to be used for reading files. It returns all bytes asked unless EOF is reached. It handles interrupts and returns -1 on errors.

//safe function to read all bytes asked, only returns less bytes if EOF
static ssize_t read_all(int fdes, void *buffer, size_t size)
{
 ssize_t ret;
 ssize_t ret2;
 ret = read(fdes, buffer, size);
 if(ret == -1){
 if(errno == EINTR)
 return read_all(fdes, buffer, size);
 return -1;
 }
 if(ret && ret != size){
 ret2 = read_all(fdes, buffer + ret, size - ret);
 if(ret2 == -1)
 return -1;
 return ret + ret2;
 }
 return ret;
}

• ChrisWue: It is recommended to state what specifically you want to get reviewed in your question.

Answer 1

My main problem is the recursion. I don't think its major but I personally would use a loop. Given the current layout I can't quite convince myself that it works in all situations (especially since there are two alternative recursive calls).

Declare variables close to the usage point (rather than everything at the top).

 ssize_t ret;
 ssize_t ret2;

C has been updated so that you can declare variables at any point in a function. This helps in readability as you don't need to scroll far to find the variable declaration. Also if you declare in the most restrictive scope possible it helps to prevent data leaking (out of a scope).

see: https://stackoverflow.com/a/8474123/14065

Try this:

static ssize_t read_all(int fdes, void* buffer, ssize_t size)
{
 ssize_t totalRead = 0;
 while(totalRead != size)
 {
 ssize_t thisRead = read(fdes, buffer + totalRead, size - totalRead);
 if ((thisRead == -1) && (errno == EINTR)) {
 continue;
 }
 // Note: There are other errors that may not be erros.
 // EAGAIN or EWOULDBLOCK spring to mind but may need special handling
 // as immediately calling read may just cause a busy wait you may
 // want to suspend the thread by calling sleep..
 if (thisRead == -1) {
 return -1;
 }
 if (thisRead == 0) {
 break;
 }
 totalRead += thisRead;
 }
 return totalRead;
}

• dmuir: Typo? thisRead should be a ssize_t, yes?