Re: proposed tweaks for openat in coreutils

[Jim Meyering]

Paul Eggert <address@hidden> wrote:
> I stared at the openat code in coreutils for a while and came up with
> the following proposed cleanups of several small issues.  Perhaps the
> largest one is that I guess mkdirat uses openat_save_fail even when it
> is a no-op macro, which doesn't sound right.

Indeed.

> On the plus side, the proposed change makes the code 80 lines shorter....
>
> I have some minor qualms about this part of the patch in remove.c:
>
> -     {
> -       bool t_cwd_restore_failed = false;
> -       status = remove_dir (fd_cwd, ds, filename, x, &t_cwd_restore_failed);
> -       *cwd_restore_failed |= t_cwd_restore_failed;
> -     }
> +     status = remove_dir (fd_cwd, ds, filename, x, cwd_errno);

That is a fine change.

> I can't figure out why the old code had that local boolean variable,
> rather than just passing cwd_restore_failed to remove_dir.  Perhaps
> this was to help with debugging with GDB?

I wrote it that way to work with an early version of openat_permissive
that would set *cwd_restore_failed to false, too, not just to `true'.

> 2005-12-16  Paul Eggert  <address@hidden>
>
>       * lib/openat.c: Don't include <stdlib.h>, <unistd.h>, <fcntl.h>,
...
>       * lib/openat.h: Revamp code so that function macros depend on
...
>       * src/remove.c (OPENAT_CWD_RESTORE__REQUIRE): Remove.
>       (OPENAT_CWD_RESTORE__ALLOW_FAILURE): Likewise.
>       (fd_to_subdirp): Remove openat_cwd_restore_allow_failure arg; its
>       value is now signified by whether cwd_errno is null.
>       (fd_to_subdirp, remove_dir, rm_1); Change cwd failure indicator from
>       pointer-to-bool to pointer-to-errno-value.  All callers changed.
>       (rm_1): Don't bother setting a local cwd failure flag and then
>       ORing it into the caller's.  Just set the caller's.
>       (rm): Use cwd failure errno value to print a slightly-better
>       diagnostic.

That looks fine, too.
Thanks for all the clean-up!

I was going to request a test that would demonstrate the use of the
slightly-better diagnostic, but realized it could be exercised only
on systems (not Solaris, and not Linux-with-PROC_FS) for which the
gnulib openat emulation resorts to using save_cwd and restore_cwd.
With such limitations, I won't require it, but please do at least
simulate it once, e.g., on Linux by tweaking lib/openat.c so that its
open always fails and it falls through to the save_cwd/restore_cwd code.
Then, run rm from a write-only directory (forcing save_cwd to use getcwd)
with two relative-named non-empty hierarchies.  When rm is in the first
openat call, and has changed to the top of the first hierarchy, rename
the initial working directory so that the first restore_cwd call fails.
That should provoke the new diagnostic.