[erlang-patches] [PATCH] erts: fix error logic in efile_sendfile

[Michael Santos]

On Tue, Dec 06, 2011 at 05:49:51PM +0800, Jovi Zhang wrote:
> On Tue, Dec 6, 2011 at 4:51 PM, Henrik Nord <henrik@REDACTED> wrote:
> > Hello!
> >
> > We can not get the tests to pass with this patch.
> > can you rerun the test at your side and verify that they all pass?
> >
> > Thank you
> >
> 
> Sorry, I missed that "*nbytes == 0" have special meaning used by upper layer.
> I will take more carefully in next time.
> 
> I have removed "*nbytes == 0" part, please try below new patch.
> 
> 
> From cbfce8104b708e7df287b88783bf7bdc21dc1035 Mon Sep 17 00:00:00 2001
> From: Jovi Zhang <bookjovi@REDACTED>
> Date: Tue, 29 Nov 2011 05:23:56 +0800
> Subject: [PATCH] erts: minor fix for unnecessary condition
> 
> In "while (retval != -1 && retval == SENDFILE_CHUNK_SIZE)", "retval !=
> -1" is pointless.

-    } while (retval != -1 && retval == SENDFILE_CHUNK_SIZE);                                                                          
+    } while (retval == SENDFILE_CHUNK_SIZE);              

Looks ok, but the surrounding code looks sketchy.

For Linux:

erts/emulator/drivers/unix/unix_efile.c:
    do {
      // check if *nbytes is 0 or greater than the largest size_t
      if (*nbytes == 0 || *nbytes > SENDFILE_CHUNK_SIZE)
    retval = sendfile(out_fd, in_fd, offset, SENDFILE_CHUNK_SIZE);
      else
    retval = sendfile(out_fd, in_fd, offset, *nbytes);
      if (retval > 0) {
    written += retval;
    *nbytes -= retval;
      }
    } while (retval != -1 && retval == SENDFILE_CHUNK_SIZE);
    *nbytes = written;
    return check_error(retval == -1 ? -1 : 0, errInfo);

Note the ternary check in check_error() is not required either:

    return check_error(retval, errInfo);

On error, sendfile returns -1 and sets errno. So the function will
return on error, including EAGAIN and EINTR.

The calling function, invoke_sendfile(), ignores EAGAIN and EINTR:

erts/emulator/drivers/common/efile_drv.c:
    } else if (result == 0 && (d->errInfo.posix_errno == EAGAIN
        || d->errInfo.posix_errno == EINTR)) {
        d->result_ok = 1;
    } 

For Darwin:

erts/emulator/drivers/unix/unix_efile.c:
    int retval;
    off_t len;
    do {
      // check if *nbytes is 0 or greater than the largest off_t
      if(*nbytes > SENDFILE_CHUNK_SIZE)
    len = SENDFILE_CHUNK_SIZE;
      else
    len = *nbytes;
      retval = sendfile(in_fd, out_fd, *offset, &len, NULL, 0);
      if (retval != -1 || errno == EAGAIN || errno == EINTR) {
        *offset += len;
    *nbytes -= len;
    written += len;
      }
    } while (len == SENDFILE_CHUNK_SIZE);
    *nbytes = written;
    return check_error(retval, errInfo);

If retval returns an error and errno is either EAGAIN or EINTR, the
chunk is discarded and the offset moved to the next chunk. Probably the
check should be:

      if (retval != -1) {

In the case where len is set to SENDFILE_CHUNK_SIZE and sendfile returns
an error other than EAGAIN/EINTR (for example, the fd is bad, etc),
the driver will go into a loop.

For FreeBSD:

The code is similar to Darwin, so the same changes need to be applied.