Re: [PATCH 2/8] xread: poll on non blocking fds
On Mon, Dec 14, 2015 at 3:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Eric Sunshine <ericsunshine@gmail.com> writes:
>
>> This comment tells us what the code itself already says, but not why
>> the value is being ignored. The reader still has to consult the commit
>> message to learn that detail, which makes the value of the comment
>> questionable.
>
> Let's do this for now, then.
That looks good to me. I'll pick it up for the resend.
Show 69 quoted lines
>
> -- >8 --
> From: Stefan Beller <sbeller@google.com>
> Date: Mon, 14 Dec 2015 11:37:12 -0800
> Subject: [PATCH] xread: poll on non blocking fds
>
> The man page of read(2) says:
>
> EAGAIN The file descriptor fd refers to a file other than a socket
> and has been marked nonblocking (O_NONBLOCK), and the read
> would block.
>
> EAGAIN or EWOULDBLOCK
> The file descriptor fd refers to a socket and has been marked
> nonblocking (O_NONBLOCK), and the read would block. POSIX.1-2001
> allows either error to be returned for this case, and does not
> require these constants to have the same value, so a portable
> application should check for both possibilities.
>
> If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.
> As the intent of xread is to read as much as possible either until the
> fd is EOF or an actual error occurs, we can ease the feeder of the fd
> by not spinning the whole time, but rather wait for it politely by not
> busy waiting.
>
> We should not care if the call to poll failed, as we're in an infinite
> loop and can only get out with the correct read().
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Acked-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> wrapper.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index 6fcaa4d..1770efa 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -236,8 +236,24 @@ ssize_t xread(int fd, void *buf, size_t len)
> len = MAX_IO_SIZE;
> while (1) {
> nr = read(fd, buf, len);
> - if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
> - continue;
> + if (nr < 0) {
> + if (errno == EINTR)
> + continue;
> + if (errno == EAGAIN || errno == EWOULDBLOCK) {
> + struct pollfd pfd;
> + pfd.events = POLLIN;
> + pfd.fd = fd;
> + /*
> + * it is OK if this poll() failed; we
> + * want to leave this infinite loop
> + * only when read() returns with
> + * success, or an expected failure,
> + * which would be checked by the next
> + * call to read(2).
> + */
> + poll(&pfd, 1, -1);
> + }
> + }
> return nr;
> }
> }
> --
> 2.7.0-rc0-109-gb762328
>Stefan Beller <sbeller@google.com>