]> Savannah Git Hosting - gnulib.git/commitdiff
fchmodat, lchmod: simplify
authorPaul Eggert <eggert@cs.ucla.edu>
Sun, 23 Feb 2020 06:47:06 +0000 (22:47 -0800)
committerPaul Eggert <eggert@cs.ucla.edu>
Sun, 23 Feb 2020 08:13:39 +0000 (00:13 -0800)
It appears that we may have overengineered lchmod and fchmodat,
in that the code was prepared for some hypothetical platforms but
was so complicated that it was hard to understand.  I attempted to
improve the situation by simplifying the code when this
simplification should not hurt on real platforms; we can re-add
complexity later to port to platforms I didn’t know about.
* lib/fchmodat.c (fchmodat):
* lib/lchmod.c (lchmod):
Put the ‘defined __linux__ || defined __ANDROID__’ #ifdef only
around the /proc code that needs it.
* lib/fchmodat.c (fchmodat): Coalese calls to orig_fchmodat.
* lib/lchmod.c (__need_system_sys_stat_h): Omit; no longer needed.
Do not include <config.h> twice.
(orig_lchmod) [HAVE_LCHMOD]: Remove, since we need not wrap
lchmod on any known hosts.
(lchmod): Do not defer to fchmodat, so that the lchmod module
need not depend on the fchmodat module (which is a circular
dependency).  Do not use openat, since ‘open’ suffices.
Coalesce calls to lchmod/chmod.
* lib/lchmod.c, lib/sys_stat.in.h (lchmod):
* m4/sys_stat_h.m4 (REPLACE_FSTAT):
* modules/lchmod (Depends-on, configure.ac):
* modules/sys_stat (Depends-on):
Do not worry about replacing lchmod, since that shouldn’t happen.
* m4/lchmod.m4 (gl_FUNC_LCHMOD): Do not check for fchmodat.
Do not worry about whether lchmod works on non-symlinks,
since every known lchmod works on non-symlinks.
* modules/lchmod (Depends-on):
Remove circular dependency on fchmodat.

ChangeLog
lib/fchmodat.c
lib/lchmod.c
lib/sys_stat.in.h
m4/lchmod.m4
m4/sys_stat_h.m4
modules/lchmod
modules/sys_stat

index 71a6ce343986da1d5932d44122bc18a4dfa3755d..c21e04d00e74b14b38b080aa3a3cd93a9c82769a 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,36 @@
+2020-02-22  Paul Eggert  <eggert@cs.ucla.edu>
+
+       fchmodat, lchmod: simplify
+       It appears that we may have overengineered lchmod and fchmodat,
+       in that the code was prepared for some hypothetical platforms but
+       was so complicated that it was hard to understand.  I attempted to
+       improve the situation by simplifying the code when this
+       simplification should not hurt on real platforms; we can re-add
+       complexity later to port to platforms I didn’t know about.
+       * lib/fchmodat.c (fchmodat):
+       * lib/lchmod.c (lchmod):
+       Put the ‘defined __linux__ || defined __ANDROID__’ #ifdef only
+       around the /proc code that needs it.
+       * lib/fchmodat.c (fchmodat): Coalese calls to orig_fchmodat.
+       * lib/lchmod.c (__need_system_sys_stat_h): Omit; no longer needed.
+       Do not include <config.h> twice.
+       (orig_lchmod) [HAVE_LCHMOD]: Remove, since we need not wrap
+       lchmod on any known hosts.
+       (lchmod): Do not defer to fchmodat, so that the lchmod module
+       need not depend on the fchmodat module (which is a circular
+       dependency).  Do not use openat, since ‘open’ suffices.
+       Coalesce calls to lchmod/chmod.
+       * lib/lchmod.c, lib/sys_stat.in.h (lchmod):
+       * m4/sys_stat_h.m4 (REPLACE_FSTAT):
+       * modules/lchmod (Depends-on, configure.ac):
+       * modules/sys_stat (Depends-on):
+       Do not worry about replacing lchmod, since that shouldn’t happen.
+       * m4/lchmod.m4 (gl_FUNC_LCHMOD): Do not check for fchmodat.
+       Do not worry about whether lchmod works on non-symlinks,
+       since every known lchmod works on non-symlinks.
+       * modules/lchmod (Depends-on):
+       Remove circular dependency on fchmodat.
+
 2020-02-22  Bruno Haible  <bruno@clisp.org>
 
        lchmod: Fix link error on Solaris 10 (regression from 2020-02-16).
index bb48b44f53661180c3bacd3c6a83a55502f8ef23..8950168608f06e0ec43ec5bf5ec682f234fbe1eb 100644 (file)
@@ -68,8 +68,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
     {
       struct stat st;
 
-#  if defined O_PATH && defined AT_EMPTY_PATH \
-      && (defined __linux__ || defined __ANDROID__)
+#  if defined O_PATH && defined AT_EMPTY_PATH
       /* Open a file descriptor with O_NOFOLLOW, to make sure we don't
          follow symbolic links, if /proc is mounted.  O_PATH is used to
          avoid a failure if the file is not readable.
@@ -80,7 +79,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
 
       /* Up to Linux 5.3 at least, when FILE refers to a symbolic link, the
          chmod call below will change the permissions of the symbolic link
-         - which is undersired - and on many file systems (ext4, btrfs, jfs,
+         - which is undesired - and on many file systems (ext4, btrfs, jfs,
          xfs, ..., but not reiserfs) fail with error EOPNOTSUPP - which is
          misleading.  Therefore test for a symbolic link explicitly.
          Use fstatat because fstat does not work on O_PATH descriptors
@@ -99,6 +98,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           return -1;
         }
 
+#   if defined __linux__ || defined __ANDROID__
       static char const fmt[] = "/proc/self/fd/%d";
       char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
       sprintf (buf, fmt, fd);
@@ -112,10 +112,10 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           errno = chmod_errno;
           return chmod_result;
         }
-      /* /proc is not mounted.  */
-      /* Fall back on orig_fchmodat, despite the race.  */
-      return orig_fchmodat (dir, file, mode, 0);
-#  elif (defined __linux__ || defined __ANDROID__) || !HAVE_LCHMOD
+#   endif
+      /* /proc is not mounted or would not work as in GNU/Linux.  */
+
+#  else
       int fstatat_result = fstatat (dir, file, &st, AT_SYMLINK_NOFOLLOW);
       if (fstatat_result != 0)
         return fstatat_result;
@@ -124,11 +124,10 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           errno = EOPNOTSUPP;
           return -1;
         }
-      /* Fall back on orig_fchmodat, despite the race.  */
-      return orig_fchmodat (dir, file, mode, 0);
-#  else
-      return orig_fchmodat (dir, file, mode, 0);
 #  endif
+
+      /* Fall back on orig_fchmodat with no flags, despite a possible race.  */
+      flags = 0;
     }
 # endif
 
index e62111cb8b7ca9dc0841f717b32ddadf0b117a82..e11321162347a742ada7c281f20c7536b6484d8d 100644 (file)
 
 #include <config.h>
 
-/* If the user's config.h happens to include <sys/stat.h>, let it include only
-   the system's <sys/stat.h> here, so that orig_fchmodat doesn't recurse to
-   rpl_fchmodat.  */
-#define __need_system_sys_stat_h
-#include <config.h>
-
 /* Specification.  */
 #include <sys/stat.h>
-#undef __need_system_sys_stat_h
-
-#if HAVE_LCHMOD
-static inline int
-orig_lchmod (char const *file, mode_t mode)
-{
-  return lchmod (file, mode);
-}
-#endif
 
 #include <errno.h>
 #include <fcntl.h>
@@ -60,24 +45,18 @@ orig_lchmod (char const *file, mode_t mode)
 int
 lchmod (char const *file, mode_t mode)
 {
-#if HAVE_FCHMODAT
-  /* Gnulib's fchmodat contains the workaround.  No need to duplicate it
-     here.  */
-  return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
-#elif NEED_LCHMOD_NONSYMLINK_FIX \
-      && defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH \
-      && (defined __linux__ || defined __ANDROID__)            /* newer Linux */
+#if defined O_PATH && defined AT_EMPTY_PATH
   /* Open a file descriptor with O_NOFOLLOW, to make sure we don't
      follow symbolic links, if /proc is mounted.  O_PATH is used to
      avoid a failure if the file is not readable.
      Cf. <https://sourceware.org/bugzilla/show_bug.cgi?id=14578>  */
-  int fd = openat (AT_FDCWD, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
+  int fd = open (file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
   if (fd < 0)
     return fd;
 
   /* Up to Linux 5.3 at least, when FILE refers to a symbolic link, the
      chmod call below will change the permissions of the symbolic link
-     - which is undersired - and on many file systems (ext4, btrfs, jfs,
+     - which is undesired - and on many file systems (ext4, btrfs, jfs,
      xfs, ..., but not reiserfs) fail with error EOPNOTSUPP - which is
      misleading.  Therefore test for a symbolic link explicitly.
      Use fstatat because fstat does not work on O_PATH descriptors
@@ -97,6 +76,7 @@ lchmod (char const *file, mode_t mode)
       return -1;
     }
 
+# if defined __linux__ || defined __ANDROID__
   static char const fmt[] = "/proc/self/fd/%d";
   char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
   sprintf (buf, fmt, fd);
@@ -110,12 +90,10 @@ lchmod (char const *file, mode_t mode)
       errno = chmod_errno;
       return chmod_result;
     }
-  /* /proc is not mounted.  */
-  /* Fall back on chmod, despite the race.  */
-  return chmod (file, mode);
+# endif
+  /* /proc is not mounted or would not work as in GNU/Linux.  */
+
 #elif HAVE_LSTAT
-# if (NEED_LCHMOD_NONSYMLINK_FIX && (defined __linux__ || defined __ANDROID__)) \
-     || !HAVE_LCHMOD                               /* older Linux, Solaris 10 */
   struct stat st;
   int lstat_result = lstat (file, &st);
   if (lstat_result != 0)
@@ -125,12 +103,8 @@ lchmod (char const *file, mode_t mode)
       errno = EOPNOTSUPP;
       return -1;
     }
-  /* Fall back on chmod, despite the race.  */
-  return chmod (file, mode);
-# else               /* GNU/kFreeBSD, GNU/Hurd, macOS, FreeBSD, NetBSD, HP-UX */
-  return orig_lchmod (file, mode);
-# endif
-#else                                                       /* native Windows */
-  return chmod (file, mode);
 #endif
+
+  /* Fall back on chmod, despite a possible race.  */
+  return chmod (file, mode);
 }
index a27a79a7fc565679c1c3aa9beb30e82c57044dae..dc8881e6f67eeae35b4d459941c98cdb9c40fd0d 100644 (file)
@@ -518,23 +518,11 @@ _GL_WARN_ON_USE (futimens, "futimens is not portable - "
 #if @GNULIB_LCHMOD@
 /* Change the mode of FILENAME to MODE, without dereferencing it if FILENAME
    denotes a symbolic link.  */
-# if @REPLACE_LCHMOD@
-#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
-#   undef lchmod
-#   define lchmod rpl_lchmod
-#  endif
-_GL_FUNCDECL_RPL (lchmod, int,
-                  (char const *filename, mode_t mode)
-                  _GL_ARG_NONNULL ((1)));
-_GL_CXXALIAS_RPL (lchmod, int,
-                  (char const *filename, mode_t mode));
-# else
-#  if !@HAVE_LCHMOD@ || defined __hpux
+# if !@HAVE_LCHMOD@ || defined __hpux
 _GL_FUNCDECL_SYS (lchmod, int, (const char *filename, mode_t mode)
                                _GL_ARG_NONNULL ((1)));
-#  endif
-_GL_CXXALIAS_SYS (lchmod, int, (const char *filename, mode_t mode));
 # endif
+_GL_CXXALIAS_SYS (lchmod, int, (const char *filename, mode_t mode));
 _GL_CXXALIASWARN (lchmod);
 #elif defined GNULIB_POSIXCHECK
 # undef lchmod
index 61e3f11228729733a551713e06c989227ac169dd..b9e8a97cb31d394b438bf108bcebcc9766d4772f 100644 (file)
@@ -1,4 +1,4 @@
-#serial 6
+#serial 7
 
 dnl Copyright (C) 2005-2006, 2008-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
@@ -17,62 +17,9 @@ AC_DEFUN([gl_FUNC_LCHMOD],
 
   AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles
 
-  AC_CHECK_FUNCS_ONCE([fchmodat lchmod lstat])
+  AC_CHECK_FUNCS_ONCE([lchmod lstat])
   if test "$ac_cv_func_lchmod" = no; then
     HAVE_LCHMOD=0
-  else
-    AC_CACHE_CHECK([whether lchmod works on non-symlinks],
-      [gl_cv_func_lchmod_works],
-      [AC_RUN_IFELSE(
-         [AC_LANG_PROGRAM(
-            [
-              AC_INCLUDES_DEFAULT[
-              #ifndef S_IRUSR
-               #define S_IRUSR 0400
-              #endif
-              #ifndef S_IWUSR
-               #define S_IWUSR 0200
-              #endif
-              #ifndef S_IRWXU
-               #define S_IRWXU 0700
-              #endif
-              #ifndef S_IRWXG
-               #define S_IRWXG 0070
-              #endif
-              #ifndef S_IRWXO
-               #define S_IRWXO 0007
-              #endif
-            ]],
-            [[
-              int permissive = S_IRWXU | S_IRWXG | S_IRWXO;
-              int desired = S_IRUSR | S_IWUSR;
-              static char const f[] = "conftest.lchmod";
-              struct stat st;
-              if (creat (f, permissive) < 0)
-                return 1;
-              if (lchmod (f, desired) != 0)
-                return 1;
-              if (stat (f, &st) != 0)
-                return 1;
-              return ! ((st.st_mode & permissive) == desired);
-            ]])],
-         [gl_cv_func_lchmod_works=yes],
-         [gl_cv_func_lchmod_works=no],
-         [case "$host_os" in
-            dnl Guess no on Linux with glibc, yes otherwise.
-            linux-gnu*) gl_cv_func_lchmod_works="guessing no" ;;
-            *)          gl_cv_func_lchmod_works="$gl_cross_guess_normal" ;;
-          esac
-         ])
-       rm -f conftest.lchmod])
-    case $gl_cv_func_lchmod_works in
-      *yes) ;;
-      *)
-        AC_DEFINE([NEED_LCHMOD_NONSYMLINK_FIX], [1],
-          [Define to 1 if lchmod does not work right on non-symlinks.])
-        REPLACE_LCHMOD=1
-        ;;
-    esac
   fi
 ])
 
index 30d60d920f6f6368fd97f1a9459bbed3c5ac540b..3efba5a7b98632322343871b91e3bfddbe71a6ae 100644 (file)
@@ -94,7 +94,6 @@ AC_DEFUN([gl_SYS_STAT_H_DEFAULTS],
   REPLACE_FSTAT=0;      AC_SUBST([REPLACE_FSTAT])
   REPLACE_FSTATAT=0;    AC_SUBST([REPLACE_FSTATAT])
   REPLACE_FUTIMENS=0;   AC_SUBST([REPLACE_FUTIMENS])
-  REPLACE_LCHMOD=0;     AC_SUBST([REPLACE_LCHMOD])
   REPLACE_LSTAT=0;      AC_SUBST([REPLACE_LSTAT])
   REPLACE_MKDIR=0;      AC_SUBST([REPLACE_MKDIR])
   REPLACE_MKFIFO=0;     AC_SUBST([REPLACE_MKFIFO])
index c3e0a166de71eff53b397b187a73c016d0ad4b54..ae67c03b655e267bbe7d6b71072535612d794ab9 100644 (file)
@@ -6,18 +6,17 @@ lib/lchmod.c
 m4/lchmod.m4
 
 Depends-on:
-errno         [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
+errno         [test $HAVE_LCHMOD = 0]
 extensions
-fcntl-h       [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
-fchmodat      [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
-intprops      [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
-lstat         [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
+fcntl-h       [test $HAVE_LCHMOD = 0]
+intprops      [test $HAVE_LCHMOD = 0]
+lstat         [test $HAVE_LCHMOD = 0]
 sys_stat
-unistd        [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
+unistd        [test $HAVE_LCHMOD = 0]
 
 configure.ac:
 gl_FUNC_LCHMOD
-if test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1; then
+if test $HAVE_LCHMOD = 0; then
   AC_LIBOBJ([lchmod])
   gl_PREREQ_LCHMOD
 fi
index 93e0cf07ecfbce1c2c3eb40fd3d0e94ad83d45d0..4783c7efb80d7d2aef1b99ddce81880d748b85ef 100644 (file)
@@ -63,7 +63,6 @@ sys/stat.h: sys_stat.in.h $(top_builddir)/config.status $(CXXDEFS_H) $(ARG_NONNU
              -e 's|@''REPLACE_FSTAT''@|$(REPLACE_FSTAT)|g' \
              -e 's|@''REPLACE_FSTATAT''@|$(REPLACE_FSTATAT)|g' \
              -e 's|@''REPLACE_FUTIMENS''@|$(REPLACE_FUTIMENS)|g' \
-             -e 's|@''REPLACE_LCHMOD''@|$(REPLACE_LCHMOD)|g' \
              -e 's|@''REPLACE_LSTAT''@|$(REPLACE_LSTAT)|g' \
              -e 's|@''REPLACE_MKDIR''@|$(REPLACE_MKDIR)|g' \
              -e 's|@''REPLACE_MKFIFO''@|$(REPLACE_MKFIFO)|g' \