Re: [PATCH] read_in_full: always report errors

From: Jeff King <peff@peff.net>
Date: May 26, 2011, 21:04 UTC
Message-ID: <20110526210424.GD31340@sigill.intra.peff.net>
In-Reply-To: <7vlixtw5e2.fsf@alter.siamese.dyndns.org>

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

Show 9 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> 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?

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.

Show 14 quoted lines

> diff --git a/combine-diff.c b/combine-diff.c
> index be67cfc..176231e 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -845,11 +845,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
>  			result = xmalloc(len + 1);
>  
>  			done = read_in_full(fd, result, len);
> -			if (done < 0)
> +			if (done != len)
>  				die_errno("read error '%s'", elem->path);
> -			else if (done < len)
> -				die("early EOF '%s'", elem->path);
> -

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

-Peff