Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> Subject: Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock

s/Add/add/;

> We need to read from pipes without blocking in a later patch.

I am hoping that you are at least not spinning---i.e. do a poll 
first to make sure there is at least some progress to be made
before calling this.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  strbuf.c | 25 +++++++++++++++++++++++--
>  strbuf.h |  6 ++++++
>  2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index cce5eed..4130ee2 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -357,7 +357,10 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
>  	return res;
>  }
>  
> -ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
> +#define IGNORE_EAGAIN (1)
> +
> +static ssize_t strbuf_read_internal(struct strbuf *sb, int fd,
> +				    size_t hint, int flags)
>  {
>  	size_t oldlen = sb->len;
>  	size_t oldalloc = sb->alloc;
> @@ -366,8 +369,16 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
>  	for (;;) {
>  		ssize_t cnt;
>  
> -		cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
> +		cnt = read(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
>  		if (cnt < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			if (errno == EAGAIN) {
> +				if (flags & IGNORE_EAGAIN)
> +					break;
> +				else
> +					continue;
> +			}

In order to ensure that this is not breaking the normal case, I had
to look at the implementation of xread() to see it behaves identically
when the flags is not passed.  That one-time review burden implies
that this is adding a maintenance burden to keep this copied function
in sync.

We should also handle EWOULDBLOCK not just EAGAIN.

More importantly, I am not sure if this helper is even necessary.

Looking at xread (reproduced in its full glory):

/*
 * xread() is the same a read(), but it automatically restarts read()
 * operations with a recoverable error (EAGAIN and EINTR). xread()
 * DOES NOT GUARANTEE that "len" bytes is read even if the data is available.
 */
ssize_t xread(int fd, void *buf, size_t len)
{
	ssize_t nr;
	if (len > MAX_IO_SIZE)
	    len = MAX_IO_SIZE;
	while (1) {
		nr = read(fd, buf, len);
		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
			continue;
		return nr;
	}
}

Are we doing the right thing for EAGAIN here?

Now, man read(2) says this:

       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
                con‐ stants to have the same value, so a portable
                application should check for both possibilities.

If the fd is not marked nonblocking, then we will not get EAGAIN (or
EWOULDBLOCK).

When fd is set to nonblocking, the current xread() spins if read()
says that the operation would block.  What would we achieve by
spinning ourselves here, though?  The caller must have set the fd to
be nonblocking for a reason, and that reason cannot be "if the data
is not ready, we do not have anything better than just spinning for
new data to arrive"---if so, the caller wouldn't have set the fd to
be nonblocking in the first place.

Even if there is such a stupid caller that only wants us to loop,
because we explicitly allow xread() to return a short read, all of
our callers must call it in a loop if they know how much they want
to read.  We can just return from here and let them loop around us.

And your new caller that does O_NONBLOCK wants to do more than
looping upon EWOULDBLOCK.  It certainly would not want us to loop
here.

So I wonder if you can just O_NONBLOCK the fd and use the usual
strbuf_read(), i.e. without any change in this patch, and update
xread() to _unconditionally_ return when read(2) says EAGAIN or
EWOULDBLOCK.

What would that break?

>  			if (oldalloc == 0)
>  				strbuf_release(sb);
>  			else
> @@ -384,6 +395,16 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
>  	return sb->len - oldlen;
>  }
>  
> +ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
> +{
> +	return strbuf_read_internal(sb, fd, hint, 0);
> +}
> +
> +ssize_t strbuf_read_noblock(struct strbuf *sb, int fd, size_t hint)
> +{
> +	return strbuf_read_internal(sb, fd, hint, IGNORE_EAGAIN);
> +}
> +
>  #define STRBUF_MAXLINK (2*PATH_MAX)
>  
>  int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
> diff --git a/strbuf.h b/strbuf.h
> index aef2794..23ca7aa 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -367,6 +367,12 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
>  extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
>  
>  /**
> + * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.
> + * The fd must have set O_NONBLOCK.
> + */
> +extern ssize_t strbuf_read_noblock(struct strbuf *, int fd, size_t hint);
> +
> +/**
>   * Read the contents of a file, specified by its path. The third argument
>   * can be used to give a hint about the file size, to avoid reallocs.
>   */