refs: cleanup errno sideband ref related functions.

[hanwen]

v2:

• peff's feedback. For now, leave refs.c alone; instead cleanups in files-backend and the ref backend API.

cc: Han-Wen Nienhuys hanwen@google.com

[hanwen]

/submit

[gitgitgadget-git]

Submitted as pull.1011.git.git.1619173446857.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-git-1011/hanwen/einval-sideband-v1

To fetch this version to local tag pr-git-1011/hanwen/einval-sideband-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-1011/hanwen/einval-sideband-v1

[gitgitgadget-git]

On the Git mailing list, Jeff King wrote (reply to this):

On Fri, Apr 23, 2021 at 10:24:06AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

> Subject: refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn

The subject says "read_raw_ref_fn", but the patch is touching
refs_resolve_ref_unsafe(). The former is an abstract type, and I didn't
dig to see the relationships, but I'll focus on the code change in the
patch.

> A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> references.

I don't think that's sufficient, for two reasons:

  - in general we try to be careful about forks and topics in flight,
    which might end up with semantic conflicts. So we don't necessarily
    assume that we can see all code, and prefer if any subtle changes
    like this at least result in a compile failure (e.g., changing
    function name or signature). In practice, this is balanced with how
    likely such code is, how bad the breakage would be, what we're
    gaining, etc.

  - just because they are not looking for EINVAL specifically doesn't
    mean they are not looking at errno at all (e.g., after calling
    refs_resolve_ref_unsafe(), lock_ref_oid_basic() does so). So we have
    to set errno to _something_ after the error. After your patch, we
    don't set it at all for these error returns, and so we'll be left
    with whatever junk was in errno from a previous unrelated syscall,
    which could be very misleading. Since we have to set it to
    something, EINVAL seems like a reasonable value.

I certainly buy the argument that errno is a pretty lousy channel for
passing back error data, for a number of reasons.  If we were going all
the way towards getting rid of errno in this function (and replacing it
with something better, as we must, since some callers _do_ care about
more detailed information), I could see the value. But this patch
doesn't get us anywhere useful and risks regressions in the meantime.

-Peff

[gitgitgadget-git]

User Jeff King <peff@peff.net> has been added to the cc: list.

[gitgitgadget-git]

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Fri, Apr 23, 2021 at 2:57 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Apr 23, 2021 at 10:24:06AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
>
> > Subject: refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
>
> The subject says "read_raw_ref_fn", but the patch is touching
> refs_resolve_ref_unsafe(). The former is an abstract type, and I didn't
> dig to see the relationships, but I'll focus on the code change in the
> patch.

Well spotted. I reverted this part (I did glance over existing
callers, and couldn't find anyone inspecting errno)

> > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> > references.
>
> I don't think that's sufficient, for two reasons:
>
>   - in general we try to be careful about forks and topics in flight,
>     which might end up with semantic conflicts. So we don't necessarily
>     assume that we can see all code, and prefer if any subtle changes
>     like this at least result in a compile failure (e.g., changing
>     function name or signature). In practice, this is balanced with how
>     likely such code is, how bad the breakage would be, what we're
>     gaining, etc.

would you say this is warranted here? refs.h doesn't mention the word
errno, so this behavior isn't documented at all. I also looked over
the current callers of read_raw_ref, and outside of refs/*.c none seem
to inspect errno.

>   - just because they are not looking for EINVAL specifically doesn't
>     mean they are not looking at errno at all (e.g., after calling
>     refs_resolve_ref_unsafe(), lock_ref_oid_basic() does so). So we have
>     to set errno to _something_ after the error. After your patch, we
>     don't set it at all for these error returns, and so we'll be left
>     with whatever junk was in errno from a previous unrelated syscall,
>     which could be very misleading. Since we have to set it to
>     something, EINVAL seems like a reasonable value.

The function has several exit paths that don't set errno at all, so
the result is kind of random anyway, but I can't see the code I don't
have. I've updated the series, with some real progress to stamping out
errno. Hope this pleases you better.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[gitgitgadget-git]

User Han-Wen Nienhuys <hanwen@google.com> has been added to the cc: list.

[hanwen]

/submit

[gitgitgadget-git]

Submitted as pull.1011.v2.git.git.1619191907.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-git-1011/hanwen/einval-sideband-v2

To fetch this version to local tag pr-git-1011/hanwen/einval-sideband-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-1011/hanwen/einval-sideband-v2

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Errno is a global variable written by almost all system calls, and therefore it
> is hard to reason about its state.
>
> This is a functional noop, because calls to lock_ref_oid_basic() in this file
> are followed by:
>
> * lock_ref_oid_basic (copy/rename rollback error path)
>
> * write_ref_to_lockfile (both in the rollback path and the success path of
>   copy/rename)
>
> * create_symref_locked (files_create_symref)
>
> * refs_reflog_exists (reflog expiry)
>
> 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.

[gitgitgadget-git]

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

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.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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?

[gitgitgadget-git]

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Thu, Apr 29, 2021 at 3:55 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

You're right; I didn't look carefully enough.  I did a grep over the
source code for create_symref() now, and couldn't find callers that
inspect errno; the same for reflog_expire().

I'll update the commit message to reflect this.

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

I disagree.  create_symref in the refs API gets an error strbuf_t. If
the function wants to say something to the user, it should use that
mechanism. If other operations are meant to provide reasonable error
messages, they should also get an error strbuf.

The files backend touches many files as part of its operation. If the
error is something like EPERM, errno reporting leaves no channel to
describe which file and which syscall is the offending one (is it
packed-refs.lock, refs/heads/branch.lock, refs/heads/ ; is it the
creat/rename/unlink syscall?). It's not a realistic mechanism to use
for errors that are meant to be understandable for users.

The errno mechanism is also poorly adjusted for alternate backends. If
there is corrupted data in a reftable file, the library returns
REFTABLE_FORMAT_ERROR, but what errno would correspond to that?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado