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

[Jovi Zhang]

On Wed, Dec 7, 2011 at 12:57 AM, Michael Santos
<michael.santos@REDACTED> wrote:
> 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.

You are right, we can just return check_error(retval, errInfo)

>
> 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.

I'm not sure, I don't familiar with DARWIN and freebsd system, it's
possible that len will be assign
to 0 when sendfile return other error code(like bad file EBDF), so
driver will not go into a loop.

>
> For FreeBSD:
>
> The code is similar to Darwin, so the same changes need to be applied.

I don't know what's meaning about sendfile EAGAIN/EINTR in DARWIN and
freebsd system, by
mostly I agree with you comments, it's not reasonable move to next
trunk if sendfile return EAGAIN.

But it seems that non-blocking io and header/tailer work is just preliminary,
so there might have more work and patch on that part.

See below new patch, Thanks for you comments!

>From 44b0b734128f9ffb61f9d66b2d44cc2c972a46fc Mon Sep 17 00:00:00 2001
From: Jovi Zhang <bookjovi@REDACTED>
Date: Tue, 29 Nov 2011 22:22:30 +0800
Subject: [PATCH] erts: remove useless check in efile_sendfile

remove useless check for "retval != -1".

Signed-off-by: Michael Santos <michael.santos@REDACTED>
---
erts/emulator/drivers/unix/unix_efile.c |    4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/erts/emulator/drivers/unix/unix_efile.c
b/erts/emulator/drivers/unix/unix_efile.c
index eb2c5f5..3704eca 100644
--- a/erts/emulator/drivers/unix/unix_efile.c
+++ b/erts/emulator/drivers/unix/unix_efile.c
@@ -1500,9 +1500,9 @@ efile_sendfile(Efile_error* errInfo, int in_fd,
int out_fd,
       written += retval;
       *nbytes -= retval;
      }
-    } while (retval != -1 && retval == SENDFILE_CHUNK_SIZE);
+    } while (retval == SENDFILE_CHUNK_SIZE);
    *nbytes = written;
-    return check_error(retval == -1 ? -1 : 0, errInfo);
+    return check_error(retval, errInfo);
#elif defined(DARWIN)
    int retval;
    off_t len;
--
1.7.2.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-erts-remove-useless-check-in-efile_sendfile.patch
Type: application/octet-stream
Size: 1067 bytes
Desc: not available
URL: <http://erlang.org/pipermail/erlang-patches/attachments/20111207/6097f971/attachment.obj>