proposed tweaks for openat in coreutils

[Paul Eggert]

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.

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

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?


2005-12-16  Paul Eggert  <address@hidden>

        * lib/openat.c: Don't include <stdlib.h>, <unistd.h>, <fcntl.h>,
        "gettext.h"; either no longer needed or are guaranteed by openat.h.
        (_): Remove; no longer needed.
        (openat): Renamed from rpl_openat; no need for rpl_openat
        since openat.h renames openat for us.
        Replace most of the body with a call to openat_permissive,
        to avoid duplicate code.
        Port to (probably hypothetical) environments were mode_t is
        wider than int.
        (openat_permissive): Require mode arg, so that we can check
        types better.  Put it just after flags.  Change cwd failure
        indicator from pointer-to-bool to pointer-to-errno-value.
        All callers changed.
        Invoke openat_save_fail and/or openat_restore_fail if
        cwd_errno is null, so that openat can call us.
        (openat_permissive, fdopendir, fstatat, unlinkat):
        Simplify errno handling to avoid some duplicate code,
        as it's OK to set errno on success.
        * lib/openat.h: Revamp code so that function macros depend on
        __OPENAT_PREFIX only, not also on AT_FDCWD.
        (openat_ro): Remove.  Caller changed to use openat_permissive.
        (openat_permissive): Now a macro, if not a function.
        (openat_restore_fail, openat_save_fail): Now always functions,
        since mkdirat needs them even if __OPENAT_PREFIX is defined.
        * 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.

Index: lib/openat.c
===================================================================
RCS file: /fetish/cu/lib/openat.c,v
retrieving revision 1.21
diff -p -u -r1.21 openat.c
--- lib/openat.c        30 Nov 2005 13:04:26 -0000      1.21
+++ lib/openat.c        16 Dec 2005 09:00:25 -0000
@@ -23,19 +23,13 @@
 
 #include "openat.h"
 
-#include <stdlib.h>
-#include <stdarg.h>
-#include <unistd.h>
-#include <errno.h>
-#include <fcntl.h>
-
 #include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
+#include "openat-priv.h"
 #include "save-cwd.h"
 
-#include "gettext.h"
-#define _(msgid) gettext (msgid)
-
-#include "openat-priv.h"
+#include <stdarg.h>
+#include <stddef.h>
+#include <errno.h>
 
 /* Replacement for Solaris' openat function.
    <http://www.google.com/search?q=openat+site:docs.sun.com>
@@ -46,61 +40,31 @@
    Otherwise, upon failure, set errno and return -1, as openat does.
    Upon successful completion, return a file descriptor.  */
 int
-rpl_openat (int fd, char const *file, int flags, ...)
+openat (int fd, char const *file, int flags, ...)
 {
-  struct saved_cwd saved_cwd;
-  int saved_errno;
-  int err;
   mode_t mode = 0;
 
   if (flags & O_CREAT)
     {
       va_list arg;
       va_start (arg, flags);
-      /* Use the promoted type (int), not mode_t, as second argument.  */
-      mode = (mode_t) va_arg (arg, int);
-      va_end (arg);
-    }
-
-  if (fd == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file))
-    return open (file, flags, mode);
-
-  {
-    char *proc_file;
-    BUILD_PROC_NAME (proc_file, fd, file);
-    err = open (proc_file, flags, mode);
-    /* If the syscall succeeds, or if it fails with an unexpected
-       errno value, then return right away.  Otherwise, fall through
-       and resort to using save_cwd/restore_cwd.  */
-    if (0 <= err || ! EXPECTED_ERRNO (errno))
-      return err;
-  }
 
-  if (save_cwd (&saved_cwd) != 0)
-    openat_save_fail (errno);
+      /* If mode_t is narrower than int, use the promoted type (int),
+         not mode_t.  Use sizeof to guess whether mode_t is nerrower;
+         we don't know of any practical counterexamples.  */
+      if (sizeof (mode_t) < sizeof (int))
+       mode = va_arg (arg, int);
+      else
+       mode = va_arg (arg, mode_t);
 
-  if (fchdir (fd) != 0)
-    {
-      saved_errno = errno;
-      free_cwd (&saved_cwd);
-      errno = saved_errno;
-      return -1;
+      va_end (arg);
     }
 
-  err = open (file, flags, mode);
-  saved_errno = (err < 0 ? errno : 0);
-
-  if (restore_cwd (&saved_cwd) != 0)
-    openat_restore_fail (errno);
-
-  free_cwd (&saved_cwd);
-
-  if (saved_errno)
-    errno = saved_errno;
-  return err;
+  return openat_permissive (fd, file, flags, mode, NULL);
 }
 
-/* Like openat above, but set *RESTORE_FAILED if unable to save
+/* Like openat (FD, FILE, FLAGS, MODE), but if CWD_ERRNO is
+   nonnull, set *CWD_ERRNO to an errno value if unable to save
    or restore the initial working directory.  This is needed only
    the first time remove.c's remove_dir opens a command-line
    directory argument.
@@ -111,23 +75,13 @@ rpl_openat (int fd, char const *file, in
    in that case.  */
 
 int
-openat_permissive (int fd, char const *file, int flags,
-                  bool *cwd_restore_failed, ...)
+openat_permissive (int fd, char const *file, int flags, mode_t mode,
+                  int *cwd_errno)
 {
   struct saved_cwd saved_cwd;
   int saved_errno;
-  int save_restore_errno = 0;
   int err;
-  mode_t mode = 0;
-
-  if (flags & O_CREAT)
-    {
-      va_list arg;
-      va_start (arg, cwd_restore_failed);
-      /* Use the promoted type (int), not mode_t, as second argument.  */
-      mode = (mode_t) va_arg (arg, int);
-      va_end (arg);
-    }
+  bool save_ok;
 
   if (fd == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file))
     return open (file, flags, mode);
@@ -143,37 +97,31 @@ openat_permissive (int fd, char const *f
       return err;
   }
 
-  if (save_cwd (&saved_cwd) != 0)
+  save_ok = (save_cwd (&saved_cwd) == 0);
+  if (! save_ok)
     {
-      *cwd_restore_failed = true;
-      save_restore_errno = errno;
+      if (! cwd_errno)
+       openat_save_fail (errno);
+      *cwd_errno = errno;
     }
 
-  if (fchdir (fd) != 0)
-    {
-      saved_errno = errno;
-      free_cwd (&saved_cwd);
-      errno = saved_errno;
-      return -1;
-    }
-
-  err = open (file, flags, mode);
-  saved_errno = (err < 0 ? errno : 0);
+  err = fchdir (fd);
+  saved_errno = errno;
 
-  if ( ! *cwd_restore_failed && restore_cwd (&saved_cwd) != 0)
+  if (! err)
     {
-      *cwd_restore_failed = true;
-      save_restore_errno = errno;
+      err = open (file, flags, mode);
+      saved_errno = errno;
+      if (save_ok && restore_cwd (&saved_cwd) != 0)
+       {
+         if (! cwd_errno)
+           openat_restore_fail (errno);
+         *cwd_errno = errno;
+       }
     }
 
-  if ( ! *cwd_restore_failed)
-    free_cwd (&saved_cwd);
-
-  if (saved_errno)
-    errno = saved_errno;
-  else if (save_restore_errno)
-    errno = save_restore_errno;
-
+  free_cwd (&saved_cwd);
+  errno = saved_errno;
   return err;
 }
 
@@ -198,43 +146,38 @@ fdopendir (int fd)
   int saved_errno;
   DIR *dir;
 
-  {
-    char *proc_file;
-    BUILD_PROC_NAME (proc_file, fd, ".");
-    dir = opendir (proc_file);
-    saved_errno = (dir == NULL ? errno : 0);
-    /* If the syscall succeeds, or if it fails with an unexpected
-       errno value, then return right away.  Otherwise, fall through
-       and resort to using save_cwd/restore_cwd.  */
-    if (dir != NULL || ! EXPECTED_ERRNO (errno))
-      goto close_and_return;
-  }
+  char *proc_file;
+  BUILD_PROC_NAME (proc_file, fd, ".");
+  dir = opendir (proc_file);
+  saved_errno = errno;
+
+  /* If the syscall fails with an expected errno value, resort to
+     save_cwd/restore_cwd.  */
+  if (! dir && EXPECTED_ERRNO (saved_errno))
+    {
+      if (save_cwd (&saved_cwd) != 0)
+       openat_save_fail (errno);
 
-  if (save_cwd (&saved_cwd) != 0)
-    openat_save_fail (errno);
+      if (fchdir (fd) != 0)
+       {
+         dir = NULL;
+         saved_errno = errno;
+       }
+      else
+       {
+         dir = opendir (".");
+         saved_errno = errno;
+
+         if (restore_cwd (&saved_cwd) != 0)
+           openat_restore_fail (errno);
+       }
 
-  if (fchdir (fd) != 0)
-    {
-      saved_errno = errno;
       free_cwd (&saved_cwd);
-      errno = saved_errno;
-      return NULL;
     }
 
-  dir = opendir (".");
-  saved_errno = (dir == NULL ? errno : 0);
-
-  if (restore_cwd (&saved_cwd) != 0)
-    openat_restore_fail (errno);
-
-  free_cwd (&saved_cwd);
-
- close_and_return:;
   if (dir)
     close (fd);
-
-  if (saved_errno)
-    errno = saved_errno;
+  errno = saved_errno;
   return dir;
 }
 
@@ -275,26 +218,22 @@ fstatat (int fd, char const *file, struc
   if (save_cwd (&saved_cwd) != 0)
     openat_save_fail (errno);
 
-  if (fchdir (fd) != 0)
+  err = fchdir (fd);
+  saved_errno = errno;
+
+  if (! err)
     {
+      err = (flag == AT_SYMLINK_NOFOLLOW
+            ? lstat (file, st)
+            : stat (file, st));
       saved_errno = errno;
-      free_cwd (&saved_cwd);
-      errno = saved_errno;
-      return -1;
-    }
-
-  err = (flag == AT_SYMLINK_NOFOLLOW
-        ? lstat (file, st)
-        : stat (file, st));
-  saved_errno = (err < 0 ? errno : 0);
 
-  if (restore_cwd (&saved_cwd) != 0)
-    openat_restore_fail (errno);
+      if (restore_cwd (&saved_cwd) != 0)
+       openat_restore_fail (errno);
+    }
 
   free_cwd (&saved_cwd);
-
-  if (saved_errno)
-    errno = saved_errno;
+  errno = saved_errno;
   return err;
 }
 
@@ -329,23 +268,19 @@ unlinkat (int fd, char const *file, int 
   if (save_cwd (&saved_cwd) != 0)
     openat_save_fail (errno);
 
-  if (fchdir (fd) != 0)
+  err = fchdir (fd);
+  saved_errno = errno;
+
+  if (! err)
     {
+      err = (flag == AT_REMOVEDIR ? rmdir (file) : unlink (file));
       saved_errno = errno;
-      free_cwd (&saved_cwd);
-      errno = saved_errno;
-      return -1;
-    }
-
-  err = (flag == AT_REMOVEDIR ? rmdir (file) : unlink (file));
-  saved_errno = (err < 0 ? errno : 0);
 
-  if (restore_cwd (&saved_cwd) != 0)
-    openat_restore_fail (errno);
+      if (restore_cwd (&saved_cwd) != 0)
+       openat_restore_fail (errno);
+    }
 
   free_cwd (&saved_cwd);
-
-  if (saved_errno)
-    errno = saved_errno;
+  errno = saved_errno;
   return err;
 }
Index: lib/openat.h
===================================================================
RCS file: /fetish/cu/lib/openat.h,v
retrieving revision 1.14
diff -p -u -r1.14 openat.h
--- lib/openat.h        30 Nov 2005 12:40:09 -0000      1.14
+++ lib/openat.h        16 Dec 2005 09:00:25 -0000
@@ -36,38 +36,39 @@
 #endif
 
 #ifndef AT_FDCWD
-# define AT_FDCWD (-3041965)           /* same value as Solaris 9 */
-# define AT_SYMLINK_NOFOLLOW 4096      /* same value as Solaris 9 */
-# define AT_REMOVEDIR (0x1)            /* same value as Solaris 9 */
-
-# ifdef __OPENAT_PREFIX
-#  undef openat
-#  define __OPENAT_CONCAT(x, y) x ## y
-#  define __OPENAT_XCONCAT(x, y) __OPENAT_CONCAT (x, y)
-#  define __OPENAT_ID(y) __OPENAT_XCONCAT (__OPENAT_PREFIX, y)
-#  define openat __OPENAT_ID (openat)
+/* Use the same values as Solaris 9.  This shouldn't matter, but
+   there's no real reason to differ.  */
+# define AT_FDCWD (-3041965)
+# define AT_SYMLINK_NOFOLLOW 4096
+# define AT_REMOVEDIR 1
+#endif
+
+#ifdef __OPENAT_PREFIX
+
+# undef openat
+# define __OPENAT_CONCAT(x, y) x ## y
+# define __OPENAT_XCONCAT(x, y) __OPENAT_CONCAT (x, y)
+# define __OPENAT_ID(y) __OPENAT_XCONCAT (__OPENAT_PREFIX, y)
+# define openat __OPENAT_ID (openat)
 int openat (int fd, char const *file, int flags, /* mode_t mode */ ...);
-int openat_permissive (int fd, char const *file, int flags, bool 
*restore_failed, ...);
-#  if ! HAVE_FDOPENDIR
-#   define fdopendir __OPENAT_ID (fdopendir)
-#  endif
+int openat_permissive (int fd, char const *file, int flags, mode_t mode,
+                      int *cwd_errno);
+# if ! HAVE_FDOPENDIR
+#  define fdopendir __OPENAT_ID (fdopendir)
+# endif
 DIR *fdopendir (int fd);
-#  define fstatat __OPENAT_ID (fstatat)
+# define fstatat __OPENAT_ID (fstatat)
 int fstatat (int fd, char const *file, struct stat *st, int flag);
-#  define unlinkat __OPENAT_ID (unlinkat)
+# define unlinkat __OPENAT_ID (unlinkat)
 int unlinkat (int fd, char const *file, int flag);
-void openat_restore_fail (int) ATTRIBUTE_NORETURN;
-void openat_save_fail (int) ATTRIBUTE_NORETURN;
-#  define openat_ro(Fd, File, Flags, RF) openat_permissive (Fd, File, Flags, 
RF)
-# else
-#  define openat_restore_fail(Errno) /* empty */
-#  define openat_save_fail(Errno) /* empty */
-# endif
 
-#endif
+#else
+
+# define openat_permissive(Fd, File, Flags, Mode, Cwd_errno) \
+    openat (Fd, File, Flags, Mode)
 
-#ifndef openat_ro
-# define openat_ro(Fd, File, Flags, RF) openat (Fd, File, Flags)
 #endif
 
 int mkdirat (int fd, char const *file, mode_t mode);
+void openat_restore_fail (int) ATTRIBUTE_NORETURN;
+void openat_save_fail (int) ATTRIBUTE_NORETURN;
Index: src/remove.c
===================================================================
RCS file: /fetish/cu/src/remove.c,v
retrieving revision 1.139
diff -p -u -r1.139 remove.c
--- src/remove.c        23 Nov 2005 04:52:48 -0000      1.139
+++ src/remove.c        16 Dec 2005 09:00:25 -0000
@@ -74,12 +74,6 @@ enum
     CONSECUTIVE_READDIR_UNLINK_THRESHOLD = 200
   };
 
-enum
-  {
-    OPENAT_CWD_RESTORE__REQUIRE = false,
-    OPENAT_CWD_RESTORE__ALLOW_FAILURE = true
-  };
-
 enum Ternary
   {
     T_UNKNOWN = 2,
@@ -991,30 +985,25 @@ remove_entry (int fd_cwd, Dirstack_state
    unlink- or rmdir-like system call -- use that value instead of ENOTDIR
    if an opened file turns out not to be a directory.  This is important
    when the preceding non-dir-unlink failed due to e.g., EPERM or EACCES.
-   The caller must set OPENAT_CWD_RESTORE_ALLOW_FAILURE to true the first
+   The caller must use a nonnnull CWD_ERRNO the first
    time this function is called for each command-line-specified directory.
-   Set *CWD_RESTORE_FAILED if OPENAT_CWD_RESTORE_ALLOW_FAILURE is true
-   and openat_ro fails to restore the initial working directory.
-   CWD_RESTORE_FAILED may be NULL.  */
+   If CWD_ERRNO is not null, set *CWD_ERRNO to the appropriate error number
+   if this function fails to restore the initial working directory.
+   If it is null, report an error and exit if the working directory
+   isn't restored.  */
 static DIR *
 fd_to_subdirp (int fd_cwd, char const *f,
               struct rm_options const *x, int prev_errno,
               struct stat *subdir_sb, Dirstack_state *ds,
-              bool openat_cwd_restore_allow_failure,
-              bool *cwd_restore_failed)
+              int *cwd_errno ATTRIBUTE_UNUSED)
 {
-  int fd_sub;
-  bool dummy;
+  int fd_sub = openat_permissive (fd_cwd, f, O_RDONLY | OPEN_NO_FOLLOW_SYMLINK,
+                                 0, cwd_errno);
 
   /* Record dev/ino of F.  We may compare them against saved values
      to thwart any attempt to subvert the traversal.  They are also used
      to detect directory cycles.  */
-  if ((fd_sub = (openat_cwd_restore_allow_failure
-                ? openat_ro (fd_cwd, f, O_RDONLY | OPEN_NO_FOLLOW_SYMLINK,
-                             (cwd_restore_failed
-                              ? cwd_restore_failed : &dummy))
-                : openat (fd_cwd, f, O_RDONLY | OPEN_NO_FOLLOW_SYMLINK))) < 0
-      || fstat (fd_sub, subdir_sb) != 0)
+  if (fd_sub < 0 || fstat (fd_sub, subdir_sb) != 0)
     {
       if (errno != ENOENT || !x->ignore_missing_files)
        error (0, errno,
@@ -1126,9 +1115,7 @@ remove_cwd_entries (DIR **dirp,
        case RM_NONEMPTY_DIR:
          {
            DIR *subdir_dirp = fd_to_subdirp (dirfd (*dirp), f,
-                                             x, errno, subdir_sb, ds,
-                                             OPENAT_CWD_RESTORE__REQUIRE,
-                                             NULL);
+                                             x, errno, subdir_sb, ds, NULL);
            if (subdir_dirp == NULL)
              {
                AD_mark_as_unremovable (ds, f);
@@ -1193,7 +1180,7 @@ The following directory is part of the c
 
 static enum RM_status
 remove_dir (int fd_cwd, Dirstack_state *ds, char const *dir,
-           struct rm_options const *x, bool *cwd_restore_failed)
+           struct rm_options const *x, int *cwd_errno)
 {
   enum RM_status status;
   struct stat dir_sb;
@@ -1206,9 +1193,7 @@ remove_dir (int fd_cwd, Dirstack_state *
      fd_to_subdirp's fstat, along with the `fstat' and the dev/ino
      comparison in AD_push ensure that we detect it and fail.  */
 
-  DIR *dirp = fd_to_subdirp (fd_cwd, dir, x, 0, &dir_sb, ds,
-                            OPENAT_CWD_RESTORE__ALLOW_FAILURE,
-                            cwd_restore_failed);
+  DIR *dirp = fd_to_subdirp (fd_cwd, dir, x, 0, &dir_sb, ds, cwd_errno);
 
   if (dirp == NULL)
     return RM_ERROR;
@@ -1320,7 +1305,7 @@ remove_dir (int fd_cwd, Dirstack_state *
 
 static enum RM_status
 rm_1 (Dirstack_state *ds, char const *filename,
-      struct rm_options const *x, bool *cwd_restore_failed)
+      struct rm_options const *x, int *cwd_errno)
 {
   char const *base = base_name (filename);
   if (DOT_OR_DOTDOT (base))
@@ -1345,11 +1330,7 @@ rm_1 (Dirstack_state *ds, char const *fi
       if (setjmp (ds->current_arg_jumpbuf))
        status = RM_ERROR;
       else
-       {
-         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);
     }
 
   ds_clear (ds);
@@ -1364,12 +1345,12 @@ rm (size_t n_files, char const *const *f
 {
   enum RM_status status = RM_OK;
   Dirstack_state *ds = ds_init ();
-  bool cwd_restore_failed = false;
+  int cwd_errno = 0;
   size_t i;
 
   for (i = 0; i < n_files; i++)
     {
-      if (cwd_restore_failed && IS_RELATIVE_FILE_NAME (file[i]))
+      if (cwd_errno && IS_RELATIVE_FILE_NAME (file[i]))
        {
          error (0, 0, _("cannot remove relative-named %s"), quote (file[i]));
          status = RM_ERROR;
@@ -1377,14 +1358,14 @@ rm (size_t n_files, char const *const *f
        }
 
       cycle_check_init (&ds->cycle_check_state);
-      enum RM_status s = rm_1 (ds, file[i], x, &cwd_restore_failed);
+      enum RM_status s = rm_1 (ds, file[i], x, &cwd_errno);
       assert (VALID_STATUS (s));
       UPDATE_STATUS (status, s);
     }
 
-  if (x->require_restore_cwd && cwd_restore_failed)
+  if (x->require_restore_cwd && cwd_errno)
     {
-      error (0, 0,
+      error (0, cwd_errno,
             _("cannot restore current working directory"));
       status = RM_ERROR;
     }