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

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