{"thread":{"id":"50125","subject":"[PATCH v4 1/4] transport-helper: use xread instead of read","startedAt":"2018-12-28T23:36:14Z","lastAt":"2019-01-03T19:31:40Z","messageCount":4,"participants":["randall.s.becker@rogers.com","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"365936","messageId":"20181228233556.5704-1-randall.s.becker@rogers.com","threadId":"50125","inReplyTo":null,"subject":"[PATCH v4 1/4] transport-helper: use xread instead of read","fromName":"","fromEmail":"randall.s.becker@rogers.com","sentAt":"2018-12-28T23:35:56Z","receivedAt":"2018-12-28T23:36:14Z","isPatch":true,"sender":{"key":"randall.s.becker@rogers.com","avatar":null},"body":"From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n\nThis fix was needed on HPE NonStop NSE and NSX where SSIZE_MAX is less than\nBUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c\nwas the only place outside of wrapper.c where it is used instead of xread.\n\nSigned-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n---\n transport-helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex bf225c698f..a290695a12 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1225,7 +1225,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n \t\treturn 0;\t/* No space for more. */\n \n \ttransfer_debug(\"%s is readable\", t->src_name);\n-\tbytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n+\tbytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n- \tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n- \t\terrno != EINTR) {\n+ \tif (bytes < 0 && errno != EINTR) {\n \t\terror_errno(_(\"read(%s) failed\"), t->src_name);\n-- \n2.17.0.10.gb132f7033\n\n"},{"id":"366054","messageId":"xmqqbm4ykcdk.fsf@gitster-ct.c.googlers.com","threadId":"50125","inReplyTo":"20181228233556.5704-1-randall.s.becker@rogers.com","subject":"Re: [PATCH v4 1/4] transport-helper: use xread instead of read","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-02T20:55:51Z","receivedAt":"2019-01-02T20:55:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"randall.s.becker@rogers.com writes:\n\n> From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n>\n> This fix was needed on HPE NonStop NSE and NSX where SSIZE_MAX is less than\n> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c\n> was the only place outside of wrapper.c where it is used instead of xread.\n\nThanks.\n\n> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n> ---\n>  transport-helper.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index bf225c698f..a290695a12 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -1225,7 +1225,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n>  \t\treturn 0;\t/* No space for more. */\n>  \n>  \ttransfer_debug(\"%s is readable\", t->src_name);\n> -\tbytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n> +\tbytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n> - \tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n> - \t\terrno != EINTR) {\n> + \tif (bytes < 0 && errno != EINTR) {\n>  \t\terror_errno(_(\"read(%s) failed\"), t->src_name);\n\nCan't we also lose EINTR check, though?  When read() returns\nnegative, we check errno and if it is EINTR, continue the loop.\n"},{"id":"366072","messageId":"20190103071632.GB24149@sigill.intra.peff.net","threadId":"50125","inReplyTo":"xmqqbm4ykcdk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 1/4] transport-helper: use xread instead of read","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-03T07:16:33Z","receivedAt":"2019-01-03T07:16:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 02, 2019 at 12:55:51PM -0800, Junio C Hamano wrote:\n\n> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n> > ---\n> >  transport-helper.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > index bf225c698f..a290695a12 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -1225,7 +1225,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n> >  \t\treturn 0;\t/* No space for more. */\n> >  \n> >  \ttransfer_debug(\"%s is readable\", t->src_name);\n> > -\tbytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n> > +\tbytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n> > - \tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n> > - \t\terrno != EINTR) {\n> > + \tif (bytes < 0 && errno != EINTR) {\n> >  \t\terror_errno(_(\"read(%s) failed\"), t->src_name);\n> \n> Can't we also lose EINTR check, though?  When read() returns\n> negative, we check errno and if it is EINTR, continue the loop.\n\nYes.\n\nI also wondered if this caller might actually be relying on the current\nnon-looping behavior, but it looks like I already traced through and\ndetermined this was OK:\n\n  https://public-inbox.org/git/20180111063110.GB31213@sigill.intra.peff.net/\n\n(the cleanup is correct either way, but that is what makes the\nconversion to xread() OK).\n\nWe may want to just take the xread() conversion and then do the patch\nthat I linked above on top, since it also cleans up the xwrite() spot,\ntoo.\n\n-Peff\n"},{"id":"366098","messageId":"xmqqpntdh71l.fsf@gitster-ct.c.googlers.com","threadId":"50125","inReplyTo":"20190103071632.GB24149@sigill.intra.peff.net","subject":"Re: [PATCH v4 1/4] transport-helper: use xread instead of read","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-03T19:31:34Z","receivedAt":"2019-01-03T19:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 02, 2019 at 12:55:51PM -0800, Junio C Hamano wrote:\n>\n>> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n>> > ---\n>> >  transport-helper.c | 2 +-\n>> >  1 file changed, 1 insertion(+), 1 deletion(-)\n>> >\n>> > diff --git a/transport-helper.c b/transport-helper.c\n>> > index bf225c698f..a290695a12 100644\n>> > --- a/transport-helper.c\n>> > +++ b/transport-helper.c\n>> > @@ -1225,7 +1225,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n>> >  \t\treturn 0;\t/* No space for more. */\n>> >  \n>> >  \ttransfer_debug(\"%s is readable\", t->src_name);\n>> > -\tbytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n>> > +\tbytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n>> > - \tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n>> > - \t\terrno != EINTR) {\n>> > + \tif (bytes < 0 && errno != EINTR) {\n>> >  \t\terror_errno(_(\"read(%s) failed\"), t->src_name);\n>> \n>> Can't we also lose EINTR check, though?  When read() returns\n>> negative, we check errno and if it is EINTR, continue the loop.\n>\n> Yes.\n>\n> I also wondered if this caller might actually be relying on the current\n> non-looping behavior, but it looks like I already traced through and\n> determined this was OK:\n>\n>   https://public-inbox.org/git/20180111063110.GB31213@sigill.intra.peff.net/\n>\n> (the cleanup is correct either way, but that is what makes the\n> conversion to xread() OK).\n>\n> We may want to just take the xread() conversion and then do the patch\n> that I linked above on top, since it also cleans up the xwrite() spot,\n> too.\n\nOK.  That does sound cleaner.\n"}]}