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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 21, 2015, 23:55 UTC
Message-ID: <xmqq37y78gzt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442875159-13027-3-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 5 quoted lines

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

> +				if (i < 0) {
> +					if (errno == EINTR || errno == ENOMEM)
> +						continue;

I can sort of see EINTR but why is ENOMEM any special than other errors?

Show 8 quoted lines

> +					else
> +						die_errno("poll");
> +				}
> +			}
> +		}
>  		return nr;
>  	}
>  }