patchremove unnecessary test and dead diagnostic

* sha1_file.c (index_stream): Don't check for size_t < 0.
read_in_full does not return an indication of failure.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 sha1_file.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

On Thu, May 26, 2011 at 03:59:14PM +0200, Jim Meyering wrote:

Are you sure about that?

  $ sed -n '/read_in_full/,/^}/p' wrapper.c
  ssize_t read_in_full(int fd, void *buf, size_t count)
  {
          char *p = buf;
          ssize_t total = 0;

          while (count > 0) {
                  ssize_t loaded = xread(fd, p, count);
                  if (loaded <= 0)
                          return total ? total : loaded;
                  count -= loaded;
                  p += loaded;
                  total += loaded;
          }

          return total;
  }

It looks like if we get -1 on the _first_ read, we will then return -1. Subsequent errors are then ignored, and we return the (possibly truncated) result.

Which, to be honest, seems kind of insane to me. I'd think:

  while (count > 0) {
          ssize_t loaded = xread(fd, p, count);
          if (loaded < 0)
                  return loaded;
          if (loaded == 0)
                  return total;
          ...
  }

would be much more sensible semantics.

-Peff

Jeff King wrote:

Argh. I went in with blinders on, thinking that the caller was right in using a type of size_t, and then read this "xread" name and assumed that it would exit upon failure.

Thanks for catching that. Here's a better patch:

-- >8 --
Subject: [PATCH] use the correct type (ssize_t, not size_t) for read-style function

* sha1_file.c (index_stream): Using an unsigned type,
we would fail to detect a read error and then proceed to
try to write (size_t)-1 bytes.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 sha1_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Thu, May 26, 2011 at 04:34:20PM +0200, Jim Meyering wrote:

I've made the same mistake, as many of our x* functions are designed to die on error.

This version looks right to me.

There's another one, too:

-- >8 --
Subject: [PATCH] read_gitfile_gently: use ssize_t to hold read result

Otherwise, a negative error return becomes a very large read value. We catch this in practice because we compare the expected and actual numbers of bytes (and you are not likely to be reading (size_t)-1 bytes), but this makes the correctness a little more obvious.

Signed-off-by: Jeff King <peff@peff.net>
---
 setup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Jeff King wrote: ...

That looks better to me, too.

On Thu, May 26, 2011 at 04:37:10PM +0200, Jim Meyering wrote:

I was worried that some caller might care about the truncated output, but after looking through the code, that is not the case. So I think we should do this.

-- >8 --
Subject: [PATCH] read_in_full: always report errors

The read_in_full function repeatedly calls read() to fill a buffer. If the first read() returns an error, we notify the caller by returning the error. However, if we read some data and then get an error on a subsequent read, we simply return the amount of data that we did read, and the caller is unaware of the error.

This makes the tradeoff that seeing the partial data is more important than the fact that an error occurred. In practice, this is generally not the case; we care more if an error occurred, and should throw away any partial data.

I audited the current callers. In most cases, this will make no difference at all, as they do:

  if (read_in_full(fd, buf, size) != size)
	  error("short read");

However, it will help in a few cases:

  1. In sha1_file.c:index_stream, we would fail to notice
     errors in the incoming stream.

  2. When reading symbolic refs in resolve_ref, we would
     fail to notice errors and potentially use a truncated
     ref name.

  3. In various places, we will get much better error
     messages. For example, callers of safe_read would
     erroneously print "the remote end hung up unexpectedly"
     instead of showing the read error.

Signed-off-by: Jeff King <peff@peff.net>
---
 wrapper.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Jeff King <peff@peff.net> writes:

Is the caller unaware?  While it won't hurt the callers who do:

   if (expect != read_in_full(fd, buf, expect))
      die(...)

I think this change hurts the one that you mentioned in your analysis.

The caller in index_stream() reads what it could, writes what it read, and comes back and makes another call to read_in_full(), at which point either it gets an error and the whole thing would error out (i.e. no difference from before), or if it was an transient error that interrupted the previous read_in_full(), it can keep reading (with this patch it will not have a chance to do so).

Not really. I think we care about both, and I think that is what the current code tries to do.

On Thu, May 26, 2011 at 11:35:51AM -0700, Junio C Hamano wrote:

The problem is that most callers are not careful enough to repeatedly call read_in_full and find out that there might have been an error in the previous result. They see a read shorter than what they asked, and assume it was EOF.

But even if we assume all callers are careful and want to handle these transient errors, then:

  1. What sort of transient errors are we talking about? We already
     handle retrying after EAGAIN and EINTR via xread.

  2. If we get a non-transient error, are we guaranteed to get the same
     error if we make some other syscalls and then call read() again?
     Otherwise we are masking it.

But really, it just seems like a non-intuitive interface to me (as evidenced by the number of callers who _didn't_ get it right). If a caller like index_stream is really interested in reading and processing some data up to a certain size, shouldn't it just be using xread directly?

-Peff

Jeff King <peff@peff.net> writes:

I can buy that argument, but then shouldn't we change the "careful" callers to treat any short-read from read_in_full() as an error?

After this patch, which you convinced me is a good thing to do overall, they are no longer careful but are merely misguided that they can catch and tell two kinds of errors apart.

Perhaps like this. I notice that overall they are good changes, but the one in pkt-line.c does not look very good.

 combine-diff.c |    5 +----
 csum-file.c    |    2 --
 pkt-line.c     |    6 ++----
 sha1_file.c    |    2 +-
 4 files changed, 4 insertions(+), 11 deletions(-)

On Thu, May 26, 2011 at 01:53:09PM -0700, Junio C Hamano wrote:

I don't think so. A short-read could still be EOF, and you can distinguish between the two. Before, if you asked for n bytes, you would get back an 'r' that was one of:

  r < 0: error on first read
  r < n: short read via EOF, or error on subsequent read
  r == n: OK, got n bytes

With my patch, you get:

  r < 0: error on any read
  r < n: short read via EOF
  r == n: OK, got n bytes

So any negative return is an error, and less than n now _always_ means a short read. So your "careful" callers will now get the error automatically. If you want to update any callers, it would be ones like:

  if (read_in_full(fd, buf, len) != len))
          die("unable to read %d bytes", len);

which are not _wrong_, but could be more specific in doing:

  ssize_t r = read_in_full(fd, buf, len);
  if (r < 0)
          die_errno("unable to read");
  else if (r < len)
          die("short read");

But that is just a quality-of-error-message issue, not a correctness issue.

This is backwards. We now _can_ tell the two apart, so more callers could be like this.

-Peff