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

[Jeff King <peff@peff.net>]

On Thu, Sep 17, 2015 at 10:54:39AM -0700, Junio C Hamano wrote:

> OK.  Trying to repurpose strbuf_read() for non-blocking FD was a
> silly idea to begin with, as it wants to read thru to the EOF, and
> setting FD explicitly to O_NONBLOCK is a sign that the caller wants
> to grab as much as possible and does not want to wait for the EOF.
> 
> So assuming we want strbuf_read_nonblock(), what interface do we
> want from it?  We could:
> 
>  * Have it grab as much as possible into sb as long as it does not
>    block?
> 
>  * Have it grab reasonably large amount into sb, and not blocking is
>    an absolute requirement, but the function is not required to read
>    everything that is available on the FD (i.e. the caller is
>    expected to loop)?

I think we are crossing emails, but I would definitely argue for the
latter.

> If we choose the latter, then your "EAGAIN? EOF?" becomes easier,
> no?  We only have to do a single call to xread(), and then we:
> 
>  - get EAGAIN or EWOULDBLOCK; leave sb as-is, set errno==EAGAIN and
>    return -1.
> 
>  - get something (in which case that is not an EOF yet); append to
>    sb, return the number of bytes.
> 
>  - get EOF; leave sb as-is, return 0.

Yes, exactly.

> ssize_t strbuf_read_nonblock(struct strbuf *sb, int fd, size_t hint)
> {
> 	strbuf_grow(sb, hint ? hint : 8192);
> 	ssize_t want = sb->alloc - sb->len - 1;
> 	ssize_t got = xread_nonblock(fd, sb->buf + sb->len, want);
> 	if (got < 0) {
> 		if (errno == EWOULDBLOCK)
> 			errno = EAGAIN; /* make life easier for the caller */
> 		return got;
> 	}

I like the normalizing of errno, but that should probably be part of
xread_nonblock(), I would think.

> 	sb->len += got;
> 	sb->buf[sb->len] = '\0';

Use strbuf_setlen() here?

> 	return got;

If "got == 0", we naturally do not change sb->len at all, and the strbuf
is left unchanged. But do we want to de-allocate what we allocated in
strbuf_grow() above? That is what strbuf_read() does, but I think it is
even less likely to help anybody here.  With the original strbuf_read(),
you might do:

  if (strbuf_read(&buf, fd, hint) <= 0)
	return; /* got nothing */

but because the nature of strbuf_read_nonblock() is to call it from a
loop, you'd want to strbuf_release() when you leave the loop anyway.

-Peff