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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 18, 2021, 22:19 UTC
Message-ID: <YR2HVU4KBCihqoYZ@google.com>
In-Reply-To: <20210817161419.3606302-1-jonathantanmy@google.com>

Hi,

Jonathan Tan wrote:

> Han-Wen Nienhuys wrote:

Show 17 quoted lines

>> 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