Re: [PATCH v2 2/3] refs/files-backend: stop setting errno from lock_ref_oid_basic

From: Junio C Hamano <gitster@pobox.com>
To: Han-Wen Nienhuys <hanwen@google.com>
Cc: Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>,
	git <git@vger.kernel.org>, Han-Wen Nienhuys <hanwenn@gmail.com>
Subject: Re: [PATCH v2 2/3] refs/files-backend: stop setting errno from lock_ref_oid_basic
Date: Thu, 29 Apr 2021 10:55:08 +0900	[thread overview]
Message-ID: <xmqqa6pham83.fsf@gitster.g> (raw)
In-Reply-To: <CAFQ2z_O=4sUjh1wk6nRijp9Gz2eeqX4=EY+Q-OTi9ppb9ikg3g@mail.gmail.com> (Han-Wen Nienhuys's message of "Wed, 28 Apr 2021 12:55:07 +0200")

Han-Wen Nienhuys <hanwen@google.com> writes:

> On Wed, Apr 28, 2021 at 6:20 AM Junio C Hamano <gitster@pobox.com> wrote:
>> > These calls do I/O and therefore clobber errno. They are not inspecting the
>> > incoming errno.
>>
>> Hmph, are you saying that these calls do I/O and always the I/O
>> would fail?  A system call that is successfull don't touch errno;
>> only the calls that resulted in failure do.
>
> I'm saying that callers cannot reliably observe the errno result of
> lock_ref_oid_basic, because it might be clobbered by a failing
> follow-up call.

Sorry, I still do not quite get it.  For example, you cite that a
call to lock_ref_oid_basic() in files_create_symref() is followed by
create_symref_locked() that may clobber errno when the latter fails.

But a failing lock_ref_oid_basic() would yield NULL and causes the
caller to leave, before calling create_symref_locked() and letting
it clobber errno, and the caller of files_create_symref() can
observe, when it returns -1 to signal an error, the errno left by
lock_ref_oid_basic(), no?  I would understand it if no caller of
files_create_symref() cares what is in errno when it receives
negative return to signal a failure, though.

And when lock_ref_oid_basic() did not fail, create_symref_locked()
calls helpers that can fail (e.g. fdopen_lock_file()) and result in
errno getting updated to record how it failed (this is also reported
to the user via "error(... strerror(errno))").

So a caller of files_create_symref() may not be able to tell between
lock_ref_oid_basic() and create_symref_locked() which one caused the
files_create_symref() call to fail, but in either case it should be
able to inspect errno to learn what kind of error we got from the
underlying system, no?