Re: [PATCH] fixup! propagate errno from failing read

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jonathan Tan wrote:
> Han-Wen Nienhuys wrote:

>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 0d96eeba61b..f546cc3cc3d 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -454,6 +454,7 @@ stat_ref:
>>  	}
>>  	strbuf_reset(&sb_contents);
>>  	if (strbuf_read(&sb_contents, fd, 256) < 0) {
>> +		myerr = errno;
>>  		close(fd);
>>  		goto out;
>>  	}
>
> Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
>
> Thanks - a straightforward fixup. (I don't think we need the errno from
> close() in this case.)

This looks good as far as it goes, but how do we know this has covered
all the code paths?  Let's see.

The only nonobvious paths are

 stat_ref:
	/*
	 * We might have to loop back here to avoid a race
	 * condition: first we lstat() the file, then we try
	 * to read it as a link or as a file.  But if somebody
	 * changes the type of the file (file <-> directory
	 * <-> symlink) between the lstat() and reading, then
	 * we don't want to report that as an error but rather
	 * try again starting with the lstat().
	 *
	 * We'll keep a count of the retries, though, just to avoid
	 * any confusing situation sending us into an infinite loop.
	 */

	if (remaining_retries-- <= 0)
		goto out;

and

	ret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);

 out:
	if (ret && !myerr)
		BUG("returning non-zero %d, should have set myerr!", ret);

For the 'stat_ref' case, we have to check that whenever we 'goto
stat_ref', we set myerr moments before.  Fortunately, that is the
case.

For the 'fall through into out' case, we have the check the
parse_loose_ref_contents always sets *failure_errno on error.  That is
also the case.

So this indeed covers all our cases, and the BUG now correctly
reflects an invariant we can count on.  Thanks for the fix, and thanks
for looking it over.

Sincerely,
Jonathan