Re: [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn - Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>,
	Han-Wen Nienhuys <hanwenn@gmail.com>
Subject: Re: [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn
Date: Mon, 5 Jul 2021 16:34:36 +0200	[thread overview]
Message-ID: <CAFQ2z_M5bbDgdmVmn-esxg5kAoT9tGS4azvvB5LGMb0UZg9QwA@mail.gmail.com> (raw)
In-Reply-To: <87h7hedz9z.fsf@evledraar.gmail.com>

On Thu, Jul 1, 2021 at 1:55 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > diff --git a/refs.c b/refs.c
> > index 8c9490235ea6..bebe3f584da7 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1675,13 +1675,17 @@ int refs_read_raw_ref(struct ref_store *ref_store,
> >                     const char *refname, struct object_id *oid,
> >                     struct strbuf *referent, unsigned int *type)
> >  {
> > +     int result, failure;
>
> Style nit: Different variables should be on different lines, except
> where they're the same, so "int i, j", not "int ret, i". Perhaps "ret"
> and "errno" are similar enough, but I'd split them, just my 0.02.
>
> Also nit: s/failure/failure_errno/, just like the function signature.

done.

> >       if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
> >               return refs_read_special_head(ref_store, refname, oid, referent,
> >                                             type);
> >       }
> >
> > -     return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> > -                                        type);
> > +     failure = 0;
> > +     result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> > +                                          type, &failure);
> > +     errno = failure;
> > +     return result;
> >  }
>
> Is there some subtlety here where this isn't equivalent to the more
> simple/straightforward:
>
>         diff --git a/refs.c b/refs.c
>         index bebe3f584da..49ab7555de9 100644
>         --- a/refs.c
>         +++ b/refs.c
>         @@ -1675,17 +1675,14 @@ int refs_read_raw_ref(struct ref_store *ref_store,
>                               const char *refname, struct object_id *oid,
>                               struct strbuf *referent, unsigned int *type)
>          {
>         -       int result, failure;
>                 if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
>                         return refs_read_special_head(ref_store, refname, oid, referent,
>                                                       type);
>                 }
>
>         -       failure = 0;
>         -       result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
>         -                                            type, &failure);
>         -       errno = failure;
>         -       return result;
>         +       errno = 0;
>         +       return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
>         +                                            type, &errno);

There is a big difference: in this case, in read_raw_ref, the
failure_errno pointer is aliased to errno. Which means that things
like

   *failure_errno = 0
   some_syscall();

might mysteriously clobber *failure_errno again, which is the kind of
effect-at-a-distance we're trying to get rid of.

> I can only imagine a case where we think files_read_raw_ref() would
> encounter a new errno after it assigned to *failure_errno, which is just
> a couple of strbuf_release() calls.
>
> if that is a case we're worried about then like in my comment on 2/8
> shouldn't we be explicitly checking for such a lost/different errno?
> I.e. something like (should probably be less fatal than BUG(...):
>
>         diff --git a/refs.c b/refs.c
>         index bebe3f584da..9584ddae392 100644
>         --- a/refs.c
>         +++ b/refs.c
>         @@ -1675,16 +1675,22 @@ int refs_read_raw_ref(struct ref_store *ref_store,
>                               const char *refname, struct object_id *oid,
>                               struct strbuf *referent, unsigned int *type)
>          {
>         -       int result, failure;
>         +       int result, failure_errno = 0;
>         +
>                 if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
>                         return refs_read_special_head(ref_store, refname, oid, referent,
>                                                       type);
>                 }
>
>         -       failure = 0;
>         +       errno = 0;
>                 result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
>         -                                            type, &failure);
>         -       errno = failure;
>         +                                            type, &failure_errno);
>         +       if (errno)
>         +               BUG("Got another errno from read_raw_ref()?: %s, failure errno: %s",
>         +                   strerror(errno),
>         +                   strerror(failure_errno));

The whole idea is to make sure that important (ie. errno values that
influence downstream logic) are separate from unimportant (ie. used
for error messages). By adding a BUG() here, you're making a drastic
downstream logic change depending on errno,  so your example code is
going precisely in the opposite direction of where I want to go, and
forces implementers of read_raw_ref to walk on eggshells again wrt.
errno handling to avoid triggering error messages.

> > + * Return 0 on success. If the ref doesn't exist, set failure_errno to ENOENT
> > + * and return -1. If the ref exists but is neither a symbolic ref nor an object
> > + * ID, it is broken; set REF_ISBROKEN in type, and return -1 (failure_errno
> > + * should not be ENOENT). The files backend may return EISDIR (if the ref name
> > + * is a directory) and ENOTDIR (if a ref prefix is not a directory). If there is
> > + * another error reading the ref, set failure_errno appropriately and return -1.
> >   *
>
> This documentation is a bit confusing in light of the above comments. I
> assume that "set failure_errno appropriately" here means that it will do
> it, but it's really worded like it's suggesting that you should do it,
> and does the "another error" suggest that a caller may need to deal with
> an arbitrary errno, not just the ENOENT|EISDIR|ENOTDIR?

Tried to clarify this.

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