On Tue, Jul 6, 2021 at 4:37 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > 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. > 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
