{"thread":{"id":"27467","subject":"[PATCH] remove unnecessary test and dead diagnostic","startedAt":"2011-05-26T13:59:14Z","lastAt":"2011-05-26T21:04:24Z","messageCount":10,"participants":["Jim Meyering","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"168765","messageId":"87tych5zrh.fsf@rho.meyering.net","threadId":"27467","inReplyTo":null,"subject":"[PATCH] remove unnecessary test and dead diagnostic","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-05-26T13:59:14Z","receivedAt":"2011-05-26T13:59:14Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\n* sha1_file.c (index_stream): Don't check for size_t < 0.\nread_in_full does not return an indication of failure.\n\nSigned-off-by: Jim Meyering <meyering@redhat.com>\n---\n sha1_file.c |    2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5fc877f..ea4549c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2736,8 +2736,6 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,\n \t\tsize_t actual;\n\n \t\tactual = read_in_full(fd, buf, sz);\n-\t\tif (actual < 0)\n-\t\t\tdie_errno(\"index-stream: reading input\");\n \t\tif (write_in_full(fast_import.in, buf, actual) != actual)\n \t\t\tdie_errno(\"index-stream: feeding fast-import\");\n \t\tsize -= actual;\n--\n1.7.5.2.660.g9f46c\n"},{"id":"168767","messageId":"20110526141130.GB18520@sigill.intra.peff.net","threadId":"27467","inReplyTo":"87tych5zrh.fsf@rho.meyering.net","subject":"Re: [PATCH] remove unnecessary test and dead diagnostic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T14:11:30Z","receivedAt":"2011-05-26T14:11:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2011 at 03:59:14PM +0200, Jim Meyering wrote:\n\n> * sha1_file.c (index_stream): Don't check for size_t < 0.\n> read_in_full does not return an indication of failure.\n\nAre you sure about that?\n\n  $ sed -n '/read_in_full/,/^}/p' wrapper.c\n  ssize_t read_in_full(int fd, void *buf, size_t count)\n  {\n          char *p = buf;\n          ssize_t total = 0;\n\n          while (count > 0) {\n                  ssize_t loaded = xread(fd, p, count);\n                  if (loaded <= 0)\n                          return total ? total : loaded;\n                  count -= loaded;\n                  p += loaded;\n                  total += loaded;\n          }\n\n          return total;\n  }\n\nIt looks like if we get -1 on the _first_ read, we will then return -1.\nSubsequent errors are then ignored, and we return the (possibly\ntruncated) result.\n\nWhich, to be honest, seems kind of insane to me. I'd think:\n\n  while (count > 0) {\n          ssize_t loaded = xread(fd, p, count);\n          if (loaded < 0)\n                  return loaded;\n          if (loaded == 0)\n                  return total;\n          ...\n  }\n\nwould be much more sensible semantics.\n\n-Peff\n"},{"id":"168769","messageId":"87mxi95y4z.fsf@rho.meyering.net","threadId":"27467","inReplyTo":"20110526141130.GB18520@sigill.intra.peff.net","subject":"Re: [PATCH] remove unnecessary test and dead diagnostic","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-05-26T14:34:20Z","receivedAt":"2011-05-26T14:34:20Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Jeff King wrote:\n> On Thu, May 26, 2011 at 03:59:14PM +0200, Jim Meyering wrote:\n>\n>> * sha1_file.c (index_stream): Don't check for size_t < 0.\n>> read_in_full does not return an indication of failure.\n>\n> Are you sure about that?\n>\n>   $ sed -n '/read_in_full/,/^}/p' wrapper.c\n>   ssize_t read_in_full(int fd, void *buf, size_t count)\n>   {\n>           char *p = buf;\n>           ssize_t total = 0;\n>\n>           while (count > 0) {\n>                   ssize_t loaded = xread(fd, p, count);\n\nArgh.  I went in with blinders on, thinking that the caller was\nright in using a type of size_t, and then read this \"xread\" name and\nassumed that it would exit upon failure.\n\nThanks for catching that.\nHere's a better patch:\n\n-- >8 --\nSubject: [PATCH] use the correct type (ssize_t, not size_t) for read-style function\n\n* sha1_file.c (index_stream): Using an unsigned type,\nwe would fail to detect a read error and then proceed to\ntry to write (size_t)-1 bytes.\n\nSigned-off-by: Jim Meyering <meyering@redhat.com>\n---\n sha1_file.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5fc877f..8a85217 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2731,11 +2731,11 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,\n \twrite_or_whine(fast_import.in, fast_import_cmd, len,\n \t\t       \"index-stream: feeding fast-import\");\n \twhile (size) {\n \t\tchar buf[10240];\n \t\tsize_t sz = size < sizeof(buf) ? size : sizeof(buf);\n-\t\tsize_t actual;\n+\t\tssize_t actual;\n\n \t\tactual = read_in_full(fd, buf, sz);\n \t\tif (actual < 0)\n \t\t\tdie_errno(\"index-stream: reading input\");\n \t\tif (write_in_full(fast_import.in, buf, actual) != actual)\n--\n1.7.5.2.660.g9f46c\n"},{"id":"168770","messageId":"87hb8h5y09.fsf@rho.meyering.net","threadId":"27467","inReplyTo":"20110526141130.GB18520@sigill.intra.peff.net","subject":"Re: [PATCH] remove unnecessary test and dead diagnostic","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-05-26T14:37:10Z","receivedAt":"2011-05-26T14:37:10Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Jeff King wrote:\n...\n>   $ sed -n '/read_in_full/,/^}/p' wrapper.c\n>   ssize_t read_in_full(int fd, void *buf, size_t count)\n>   {\n>           char *p = buf;\n>           ssize_t total = 0;\n>\n>           while (count > 0) {\n>                   ssize_t loaded = xread(fd, p, count);\n>                   if (loaded <= 0)\n>                           return total ? total : loaded;\n>                   count -= loaded;\n>                   p += loaded;\n>                   total += loaded;\n>           }\n>\n>           return total;\n>   }\n>\n> It looks like if we get -1 on the _first_ read, we will then return -1.\n> Subsequent errors are then ignored, and we return the (possibly\n> truncated) result.\n>\n> Which, to be honest, seems kind of insane to me. I'd think:\n>\n>   while (count > 0) {\n>           ssize_t loaded = xread(fd, p, count);\n>           if (loaded < 0)\n>                   return loaded;\n>           if (loaded == 0)\n>                   return total;\n>           ...\n>   }\n>\n> would be much more sensible semantics.\n\nThat looks better to me, too.\n"},{"id":"168796","messageId":"20110526162844.GB4049@sigill.intra.peff.net","threadId":"27467","inReplyTo":"87mxi95y4z.fsf@rho.meyering.net","subject":"Re: [PATCH] remove unnecessary test and dead diagnostic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T16:28:44Z","receivedAt":"2011-05-26T16:28:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2011 at 04:34:20PM +0200, Jim Meyering wrote:\n\n> Argh.  I went in with blinders on, thinking that the caller was\n> right in using a type of size_t, and then read this \"xread\" name and\n> assumed that it would exit upon failure.\n\nI've made the same mistake, as many of our x* functions are designed to\ndie on error.\n\n> Subject: [PATCH] use the correct type (ssize_t, not size_t) for read-style function\n> \n> * sha1_file.c (index_stream): Using an unsigned type,\n> we would fail to detect a read error and then proceed to\n> try to write (size_t)-1 bytes.\n\nThis version looks right to me.\n\nThere's another one, too:\n\n-- >8 --\nSubject: [PATCH] read_gitfile_gently: use ssize_t to hold read result\n\nOtherwise, a negative error return becomes a very large read\nvalue. We catch this in practice because we compare the\nexpected and actual numbers of bytes (and you are not likely\nto be reading (size_t)-1 bytes), but this makes the\ncorrectness a little more obvious.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n setup.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 013ad11..ce87900 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -382,7 +382,7 @@ const char *read_gitfile_gently(const char *path)\n \tconst char *slash;\n \tstruct stat st;\n \tint fd;\n-\tsize_t len;\n+\tssize_t len;\n \n \tif (stat(path, &st))\n \t\treturn NULL;\n-- \n1.7.4.5.13.gd3ff5\n"},{"id":"168798","messageId":"20110526163027.GC4049@sigill.intra.peff.net","threadId":"27467","inReplyTo":"87hb8h5y09.fsf@rho.meyering.net","subject":"[PATCH] read_in_full: always report errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T16:30:27Z","receivedAt":"2011-05-26T16:30:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2011 at 04:37:10PM +0200, Jim Meyering wrote:\n\n> > It looks like if we get -1 on the _first_ read, we will then return -1.\n> > Subsequent errors are then ignored, and we return the (possibly\n> > truncated) result.\n> >\n> > Which, to be honest, seems kind of insane to me. I'd think:\n> >\n> >   while (count > 0) {\n> >           ssize_t loaded = xread(fd, p, count);\n> >           if (loaded < 0)\n> >                   return loaded;\n> >           if (loaded == 0)\n> >                   return total;\n> >           ...\n> >   }\n> >\n> > would be much more sensible semantics.\n> \n> That looks better to me, too.\n\nI was worried that some caller might care about the truncated output,\nbut after looking through the code, that is not the case. So I think we\nshould do this.\n\n-- >8 --\nSubject: [PATCH] read_in_full: always report errors\n\nThe read_in_full function repeatedly calls read() to fill a\nbuffer. If the first read() returns an error, we notify the\ncaller by returning the error. However, if we read some data\nand then get an error on a subsequent read, we simply return\nthe amount of data that we did read, and the caller is\nunaware of the error.\n\nThis makes the tradeoff that seeing the partial data is more\nimportant than the fact that an error occurred. In practice,\nthis is generally not the case; we care more if an error\noccurred, and should throw away any partial data.\n\nI audited the current callers. In most cases, this will make\nno difference at all, as they do:\n\n  if (read_in_full(fd, buf, size) != size)\n\t  error(\"short read\");\n\nHowever, it will help in a few cases:\n\n  1. In sha1_file.c:index_stream, we would fail to notice\n     errors in the incoming stream.\n\n  2. When reading symbolic refs in resolve_ref, we would\n     fail to notice errors and potentially use a truncated\n     ref name.\n\n  3. In various places, we will get much better error\n     messages. For example, callers of safe_read would\n     erroneously print \"the remote end hung up unexpectedly\"\n     instead of showing the read error.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n wrapper.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 2829000..85f09df 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -148,8 +148,10 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n \n \twhile (count > 0) {\n \t\tssize_t loaded = xread(fd, p, count);\n-\t\tif (loaded <= 0)\n-\t\t\treturn total ? total : loaded;\n+\t\tif (loaded < 0)\n+\t\t\treturn -1;\n+\t\tif (loaded == 0)\n+\t\t\treturn total;\n \t\tcount -= loaded;\n \t\tp += loaded;\n \t\ttotal += loaded;\n-- \n1.7.4.5.13.gd3ff5\n"},{"id":"168812","messageId":"7vy61twbqw.fsf@alter.siamese.dyndns.org","threadId":"27467","inReplyTo":"20110526163027.GC4049@sigill.intra.peff.net","subject":"Re: [PATCH] read_in_full: always report errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T18:35:51Z","receivedAt":"2011-05-26T18:35:51Z","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> Subject: [PATCH] read_in_full: always report errors\n>\n> The read_in_full function repeatedly calls read() to fill a\n> buffer. If the first read() returns an error, we notify the\n> caller by returning the error. However, if we read some data\n> and then get an error on a subsequent read, we simply return\n> the amount of data that we did read, and the caller is\n> unaware of the error.\n\nIs the caller unaware?  While it won't hurt the callers who do:\n\n   if (expect != read_in_full(fd, buf, expect))\n      die(...)\n\nI think this change hurts the one that you mentioned in your analysis.\n\nThe caller in index_stream() reads what it could, writes what it read, and\ncomes back and makes another call to read_in_full(), at which point either\nit gets an error and the whole thing would error out (i.e. no difference\nfrom before), or if it was an transient error that interrupted the\nprevious read_in_full(), it can keep reading (with this patch it will not\nhave a chance to do so).\n\n> This makes the tradeoff that seeing the partial data is more\n> important than the fact that an error occurred. In practice,\n> this is generally not the case; we care more if an error\n> occurred, and should throw away any partial data.\n\nNot really. I think we care about both, and I think that is what the\ncurrent code tries to do.\n"},{"id":"168813","messageId":"20110526184839.GA6910@sigill.intra.peff.net","threadId":"27467","inReplyTo":"7vy61twbqw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] read_in_full: always report errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T18:48:40Z","receivedAt":"2011-05-26T18:48:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2011 at 11:35:51AM -0700, Junio C Hamano wrote:\n\n> The caller in index_stream() reads what it could, writes what it read, and\n> comes back and makes another call to read_in_full(), at which point either\n> it gets an error and the whole thing would error out (i.e. no difference\n> from before), or if it was an transient error that interrupted the\n> previous read_in_full(), it can keep reading (with this patch it will not\n> have a chance to do so).\n\nThe problem is that most callers are not careful enough to repeatedly\ncall read_in_full and find out that there might have been an error in\nthe previous result. They see a read shorter than what they asked, and\nassume it was EOF.\n\nBut even if we assume all callers are careful and want to handle these\ntransient errors, then:\n\n  1. What sort of transient errors are we talking about? We already\n     handle retrying after EAGAIN and EINTR via xread.\n\n  2. If we get a non-transient error, are we guaranteed to get the same\n     error if we make some other syscalls and then call read() again?\n     Otherwise we are masking it.\n\nBut really, it just seems like a non-intuitive interface to me (as\nevidenced by the number of callers who _didn't_ get it right). If a\ncaller like index_stream is really interested in reading and processing\nsome data up to a certain size, shouldn't it just be using xread\ndirectly?\n\n-Peff\n"},{"id":"168817","messageId":"7vlixtw5e2.fsf@alter.siamese.dyndns.org","threadId":"27467","inReplyTo":"20110526184839.GA6910@sigill.intra.peff.net","subject":"Re: [PATCH] read_in_full: always report errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T20:53:09Z","receivedAt":"2011-05-26T20:53:09Z","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> The problem is that most callers are not careful enough to repeatedly\n> call read_in_full and find out that there might have been an error in\n> the previous result. They see a read shorter than what they asked, and\n> assume it was EOF.\n\nI can buy that argument, but then shouldn't we change the \"careful\"\ncallers to treat any short-read from read_in_full() as an error?\n\nAfter this patch, which you convinced me is a good thing to do overall,\nthey are no longer careful but are merely misguided that they can catch\nand tell two kinds of errors apart.\n\nPerhaps like this.  I notice that overall they are good changes, but the\none in pkt-line.c does not look very good.\n\n combine-diff.c |    5 +----\n csum-file.c    |    2 --\n pkt-line.c     |    6 ++----\n sha1_file.c    |    2 +-\n 4 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex be67cfc..176231e 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -845,11 +845,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tresult = xmalloc(len + 1);\n \n \t\t\tdone = read_in_full(fd, result, len);\n-\t\t\tif (done < 0)\n+\t\t\tif (done != len)\n \t\t\t\tdie_errno(\"read error '%s'\", elem->path);\n-\t\t\telse if (done < len)\n-\t\t\t\tdie(\"early EOF '%s'\", elem->path);\n-\n \t\t\tresult[len] = 0;\n \n \t\t\t/* If not a fake symlink, apply filters, e.g. autocrlf */\ndiff --git a/csum-file.c b/csum-file.c\nindex fc97d6e..f5ac31f 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -19,8 +19,6 @@ static void flush(struct sha1file *f, void *buf, unsigned int count)\n \n \t\tif (ret < 0)\n \t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n-\t\tif (ret < count)\n-\t\t\tdie(\"%s: sha1 file truncated\", f->name);\n \t\tif (memcmp(buf, check_buffer, count))\n \t\t\tdie(\"sha1 file '%s' validation error\", f->name);\n \t}\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 5a04984..5628801 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -138,10 +138,8 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n static void safe_read(int fd, void *buffer, unsigned size)\n {\n \tssize_t ret = read_in_full(fd, buffer, size);\n-\tif (ret < 0)\n-\t\tdie_errno(\"read error\");\n-\telse if (ret < size)\n-\t\tdie(\"The remote end hung up unexpectedly\");\n+\tif (ret != size)\n+\t\tdie_errno(\"The remote end hung up unexpectedly\");\n }\n \n static int packet_length(const char *linelen)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8a85217..d1332c4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2736,7 +2736,7 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,\n \t\tssize_t actual;\n \n \t\tactual = read_in_full(fd, buf, sz);\n-\t\tif (actual < 0)\n+\t\tif (actual != sz)\n \t\t\tdie_errno(\"index-stream: reading input\");\n \t\tif (write_in_full(fast_import.in, buf, actual) != actual)\n \t\t\tdie_errno(\"index-stream: feeding fast-import\");\n"},{"id":"168824","messageId":"20110526210424.GD31340@sigill.intra.peff.net","threadId":"27467","inReplyTo":"7vlixtw5e2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] read_in_full: always report errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T21:04:24Z","receivedAt":"2011-05-26T21:04:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2011 at 01:53:09PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The problem is that most callers are not careful enough to repeatedly\n> > call read_in_full and find out that there might have been an error in\n> > the previous result. They see a read shorter than what they asked, and\n> > assume it was EOF.\n> \n> I can buy that argument, but then shouldn't we change the \"careful\"\n> callers to treat any short-read from read_in_full() as an error?\n\nI don't think so. A short-read could still be EOF, and you can\ndistinguish between the two. Before, if you asked for n bytes, you would\nget back an 'r' that was one of:\n\n  r < 0: error on first read\n  r < n: short read via EOF, or error on subsequent read\n  r == n: OK, got n bytes\n\nWith my patch, you get:\n\n  r < 0: error on any read\n  r < n: short read via EOF\n  r == n: OK, got n bytes\n\nSo any negative return is an error, and less than n now _always_ means a\nshort read. So your \"careful\" callers will now get the error\nautomatically. If you want to update any callers, it would be ones like:\n\n  if (read_in_full(fd, buf, len) != len))\n          die(\"unable to read %d bytes\", len);\n\nwhich are not _wrong_, but could be more specific in doing:\n\n  ssize_t r = read_in_full(fd, buf, len);\n  if (r < 0)\n          die_errno(\"unable to read\");\n  else if (r < len)\n          die(\"short read\");\n\nBut that is just a quality-of-error-message issue, not a correctness\nissue.\n\n> diff --git a/combine-diff.c b/combine-diff.c\n> index be67cfc..176231e 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -845,11 +845,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n>  \t\t\tresult = xmalloc(len + 1);\n>  \n>  \t\t\tdone = read_in_full(fd, result, len);\n> -\t\t\tif (done < 0)\n> +\t\t\tif (done != len)\n>  \t\t\t\tdie_errno(\"read error '%s'\", elem->path);\n> -\t\t\telse if (done < len)\n> -\t\t\t\tdie(\"early EOF '%s'\", elem->path);\n> -\n\nThis is backwards. We now _can_ tell the two apart, so more callers\ncould be like this.\n\n-Peff\n"}]}