Re: [PATCH] remove unnecessary test and dead diagnostic

From: Jeff King <peff@peff.net>
Date: May 26, 2011, 14:11 UTC
Message-ID: <20110526141130.GB18520@sigill.intra.peff.net>
In-Reply-To: <87tych5zrh.fsf@rho.meyering.net>

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

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

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