Re: [PATCHv3 04/13] strbuf: add strbuf_read_once to read without blocking

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2015, 00:17 UTC
Message-ID: <xmqqoagv71es.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442875159-13027-5-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> The new call will read a fd into a strbuf once. The underlying call

"read from a fd"

> xread_nonblock is meant to execute non blockingly if the fd is set to
> O_NONBLOCK.

The latter sentence adds more questions than it answers. If the file descriptor is not set to non-blocking, what happens? Is it a bug in the caller, and if so do we give help to diagnose such a bug?

Show 23 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  strbuf.c | 11 +++++++++++
>  strbuf.h |  6 ++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index cce5eed..35e71b8 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -384,6 +384,17 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
>  	return sb->len - oldlen;
>  }
>  
> +ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
> +{
> +	ssize_t cnt;
> +
> +	strbuf_grow(sb, hint ? hint : 8192);
> +	cnt = xread_nonblock(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
> +	if (cnt > 0)
> +		strbuf_setlen(sb, sb->len + cnt);
> +	return cnt;

OK. So the caller that receives a negative value can check errno to see if we got EAGAIN. How would the caller tell when it got an EOF?

>  /**
> + * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.

I do not think you want to say "same as" for this one. strbuf_read() is about reading thru to the end, but this is about making some progress without blocking.

    Read from a file descriptor that is marked as O_NONBLOCK without
    blocking.  Returns the number of new bytes appended to the sb.
    Negative return value signals there was an error returned from
    underlying read(2), in which case the caller should check errno.
    e.g. errno == EAGAIN when the read may have blocked.

or something?  Again, how would a caller tell when it got an EOF?

Show 8 quoted lines

> + * The fd must have set O_NONBLOCK.
> + */
> +extern ssize_t strbuf_read_once(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.
>   */