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

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

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

This fix was needed on HPE NonStop NSE and NSX where SSIZE_MAX is less than
BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
was the only place outside of wrapper.c where it is used instead of xread.

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);
-- 
2.17.0.10.gb132f7033

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

[Junio C Hamano]

randall.s.becker@rogers.com writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> This fix was needed on HPE NonStop NSE and NSX where SSIZE_MAX is less than
> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
> was the only place outside of wrapper.c where it is used instead of xread.

Thanks.

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

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

[Jeff King]

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 02, 2019 at 12:55:51PM -0800, Junio C Hamano wrote:
>
>> > 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.

OK.  That does sound cleaner.