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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 17, 2015, 17:54 UTC
Message-ID: <xmqqoah1hqxs.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20150917171308.GA28046@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> 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.

Show 9 quoted lines

> 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;
}