| Message ID | 20181228233556.5704-1-randall.s.becker@rogers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/4] transport-helper: use xread instead of read | expand |
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.
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
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.
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