Re: [PATCH v4 1/4] transport-helper: use xread instead of read

From: Jeff King <peff@peff.net>
Date: Jan 3, 2019, 07:16 UTC
Message-ID: <20190103071632.GB24149@sigill.intra.peff.net>
In-Reply-To: <xmqqbm4ykcdk.fsf@gitster-ct.c.googlers.com>

On Wed, Jan 02, 2019 at 12:55:51PM -0800, Junio C Hamano wrote:

Show 22 quoted lines

> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> >  transport-helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/transport-helper.c b/transport-helper.c
> > index bf225c698f..a290695a12 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -1225,7 +1225,7 @@ static int udt_do_read(struct unidirectional_transfer *t)
> >  		return 0;	/* No space for more. */
> >  
> >  	transfer_debug("%s is readable", t->src_name);
> > -	bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> > +	bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> > - 	if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> > - 		errno != EINTR) {
> > + 	if (bytes < 0 && errno != EINTR) {
> >  		error_errno(_("read(%s) failed"), t->src_name);
> 
> Can't we also lose EINTR check, though?  When read() returns
> negative, we check errno and if it is EINTR, continue the loop.

Yes.

I also wondered if this caller might actually be relying on the current non-looping behavior, but it looks like I already traced through and determined this was OK:

  https://public-inbox.org/git/20180111063110.GB31213@sigill.intra.peff.net/

(the cleanup is correct either way, but that is what makes the conversion to xread() OK).

We may want to just take the xread() conversion and then do the patch that I linked above on top, since it also cleans up the xwrite() spot, too.

-Peff