Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking

From: Stefan Beller <sbeller@google.com>
Date: Dec 15, 2015, 00:25 UTC
Message-ID: <CAGZ79kbLHNtxcwhZz=tHpJB2XnxMeuEJBG=PmoAbcVF4Wzno2g@mail.gmail.com>
In-Reply-To: <20151215001642.GA26409@sigill.intra.peff.net>

On Mon, Dec 14, 2015 at 4:16 PM, Jeff King <peff@peff.net> wrote:

Show 21 quoted lines

> On Mon, Dec 14, 2015 at 04:09:01PM -0800, Stefan Beller wrote:
>
>> > Are we trying to protect ourselves against somebody _else_ giving us a
>> > non-blocking descriptor? In that case we'll quietly spin and waste CPU.
>> > Which isn't great, but perhaps better than returning an error.
>>
>> Yes.
>> This sounds like a good reasoning for 2/8 (add in the poll, so we are
>> more polite), though.
>>
>> This patch is a prerequisite for 4/8, which explicitly doesn't want to loop
>> but a quick return. Maybe we could even drop this patch and just use
>> `read` as is in 4/8. Looking from a higher level perspective, we don't care
>> about strbuf_read_nonblocking to return after a signal without retry.
>
> I was actually thinking about simply teaching xread() not to worry about
> EAGAIN, but that would probably be a regression in the "whoops, somebody
> gave us a non-blocking stdin!" case.
>
> But yeah, I think simply using xread() as-is in strbuf_read_once (or
> whatever it ends up being called) is OK.

I was actually thinking about using {without-x}read, just the plain system call.
Do we have any issues with that for wrapping purposes for Windows?
There is no technical reason to prefer xread over read in strbuf_read_once as
* we are not nonblocking (so the EAGAIN|| EWOULDBLOCK doesn't apply)
* we don't care about EINTR and retrying upon that signal
* we would not care about MAX_IO_SIZE most likely (that's actually one
of the reasons I could technically think of to prefer xread)

> I think all of the
> _intentionally_ non-blocking descriptors are gone in this iteration,
> right?

I think we don't even have unintentional non blocking fds here now as we create all the fds ourselves and never set the NOBLOCK flag.

> So the caller of strbuf_read_once expects to have to call poll()
> or to block. And that's what xread() does.

ok, I'll drop this patch and use xread there.

>
> -Peff