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

Jeff King [off-list ref] writes:

I think they have to loop for correctness, but they may do this:

  if (xread(fd, buf, len) < 0)
	die_errno("OMG, an error!");

which is not correct if "fd" is unknowingly non-blocking.
...
The spinning behavior is not great, but does mean that we spin and
continue rather than bailing with an error.

OK, that is a problem (I just read through "git grep xread -- \*.c".
There aren't that many codepaths that read from a fd that we didn't
open ourselves, but there indeed are some.

So it seems that we would want a xread() that spins to help such
codepaths, and xread_nonblock() that gives a short-read upon
EWOULDBLOCK.  They can share a helper function, of course.

If we reset errno to "0" at the top of the function, we could get around
one problem, but it still makes an annoying interface: the caller has to
check errno for any 0-return to figure out if it was really EOF, or just
EAGAIN.

If we return -1, though, we have a similar annoyance. If the caller
notices a -1 return value and finds EAGAIN, they still may need to check
sb->len to see if they made forward progress and have data they should
be dealing with.

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)?

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.


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;
	}
	sb->len += got;
	sb->buf[sb->len] = '\0';
	return got;
}