Re: [PATCHv3 02/13] xread: poll on non blocking fds

From: Torsten Bögershausen <tboegi@web.de>
Date: Sep 22, 2015, 04:55 UTC
Message-ID: <5600DF2D.9010202@web.de>
In-Reply-To: <xmqq37y78gzt.fsf@gitster.mtv.corp.google.com>

On 09/22/2015 01:55 AM, Junio C Hamano wrote:

Show 17 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> So if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.
>> As the intend of xread is to read as much as possible either until the
>> fd is EOF or an actual error occurs, we can ease the feeder of the fd
>> by not spinning the whole time, but rather wait for it politely by not
>> busy waiting.
> As you said in the cover letter, this does look questionable.  It is
> sweeping the problem under the rug (the hard-coded 100ms is a good
> clue to tell that).  If the caller does want us to read thru to the
> end, then we would need to make it easier for such a caller to stop
> marking the file descriptor to be non-blocking, but this does not do
> anything to help that.  An alternative might be to automatically
> turn nonblocking off temporarily once we get EAGAIN (and turn it on
> again before leaving); that would be an approach to make it
> unnecessary to fix the caller (which has its own set of problems,
> though).

Wouldn' that function be somewhat mis-named and/or mis-behaved? read_in_full_with_hard_coded_timeout_and_fix_O_NONBLOCK() could make sure that the user of this function knows what's going on under the hood.

More seriously, if someone calls xread() with a non-blocking socket, the caller wants to return and does want to his own timeout handling.

If we want to have a timeouted read(), we can call it

xread_timout(int fd, voxread(int fd, void *buf, size_t len, int timeout)

(Or something similar) to make clear that there is an underlying timeout handling. Another option could be to name the function

read_in_full_timeout().

But in any case I suggest to xread() as it is, and not to change the functionality behind the back of the users.