Re: [PATCH v3 0/5] refs: cleanup errno sideband ref related functions

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jul 6, 2021, 18:36 UTC
Message-ID: <CAFQ2z_PXjiUNxgC5W=+BB4yRanHtNO+AtPq4u0k6S+t4zcj5FQ@mail.gmail.com>
In-Reply-To: <87sg0r5x0m.fsf@evledraar.gmail.com>

On Tue, Jul 6, 2021 at 4:37 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 7 quoted lines

> > I've added a failure_errno argument parse_loose_ref_contents now.
>
> Great, it's not clear if you picked up the full diff-on-top but I think
> consistently using *failure_errno in files_read_raw_ref() instead of
> "errno" and the "*failure_errno = errno" at the end also makes the code
> much more readable (and allows for the removal of the "saved_errno" we
> don't want).

It's hard to get right: there are many system calls, and in most cases EISDIR/ENOTDIR have to be propagated to the caller. I tried doing this, and promptly got a failing test related to dir/file conflicts. By doing

  *failure_errno = errno

near the exit of files_read_raw_ref(), we can be sure there are no changes in behavior. If anyone wants to understand the files backend in more depth, they're welcome to disentangle this in a follow-up patch.

Show 5 quoted lines

> By using "errno" itself for the body of the function it needs to be
> really carefully read to assure oneself that one of the functions it
> calls doesn't make a syscall, and even if we're assured of that now if
> one of them has a new syscall added in the future it might be clobbered
> at a distance.

Yes, anyone adding a syscall to files_read_raw_ref has to be very careful.

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