Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L

[Jim Meyering]

Bruno Haible wrote:

> Kamil Dudka wrote:
>> >   a) A non-symlink, non-directory. Here acl_extended_file_nofollow and
>> >      acl_extended_file are equivalent.
>>
>> If I understand this, you expect non-directories cannot be mount points, thus
>> the call cannot trigger the mount, right?
>
> That's what I was assuming, yes. My kernel knowledge is rusty, though. Is it
> now possible to use regular files as mount points?
>
>> >   c) A mount point.
>> >   f) A symlink to a mount point, with dereferencing (stat()).
>>
>> The behavior of ls wrt. tregerring mounts is implementation defined, isn't 
>> it?
>
> Yes. But if you trigger an autofs mount in case f), it will be seen as a
> bug, in the same way as it was considered a bug in case c).
>
>> > So I'm asking to change
>> >
>> >     ret = acl_extended_file_wrap (name);
>> >
>> > to
>> >
>> >     if (S_ISDIR (sb->st_mode))
>> >       ret = acl_extended_file_wrap (name);
>> >     else
>> >       ret = acl_extended_file (name);
>>
>> Is optimization the only purpose of this change?  I would then prefer to get
>> it working first and optimize it later on.
>
> I'm trying to explain to you that
>   - your patch is introducing a performance regression in case a), which is
>     the most frequent case,
>   - your patch does not completely work yet: case f).

I propose to push Kamil's fix (mainly to have a record of it,
in case we need it later), but then to immediately revert it
along with the file-has-acl.c change that started this.
That seems to be the right thing to do, going forward,
since the kernel folks have recently reverted the behavior
that motivated the earlier file-has-acl.c patch.

Here's the interesting ChangeLog entry:

        file-has-acl: revert both recent changes, 80af92af and 95f7c57f
        * lib/file-has-acl.c: While the 2011-10-03 change does fix the
        ls -lL regression introduced in coreutils-8.12, it does so at the
        cost of an additional stat call in the common case.  Besides, now
        that the kernel change that prompted commit 95f7c57f has been reverted
        (see https://bugzilla.redhat.com/show_bug.cgi?id=720325#c24)
        we have no use for commit 95f7c57f, "file-has-acl: use
        acl_extended_file_nofollow if available".

Here are the two actual commits I'm proposing.

Any objections?

>From 80af92afd10f9ed1c3621432145baf4a32ab5cea Mon Sep 17 00:00:00 2001
From: Kamil Dudka <address@hidden>
Date: Mon, 3 Oct 2011 12:17:22 +0200
Subject: [PATCH 1/2] file-has-acl: revert unintended change in behavior of ls
 -L

* lib/file-has-acl.c (acl_extended_file_wrap): New function,
derived from...
(file_has_acl): ...code here.  Call it.
This problem was introduced with 2011-07-22 commit 95f7c57f,
"file-has-acl: use acl_extended_file_nofollow if available".
See http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/28538
---
 ChangeLog          |   10 ++++++++++
 lib/file-has-acl.c |   40 ++++++++++++++++++++++++++++++----------
 2 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 7906531..6715d96 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2011-10-03  Kamil Dudka  <address@hidden>
+
+       file-has-acl: revert unintended change in behavior of ls -L
+       * lib/file-has-acl.c (acl_extended_file_wrap): New function,
+       derived from...
+       (file_has_acl): ...code here.  Call it.
+       This problem was introduced with 2011-07-22 commit 95f7c57f,
+       "file-has-acl: use acl_extended_file_nofollow if available".
+       See http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/28538
+
 2011-10-03  Bruno Haible  <address@hidden>

        poll: Avoid link errors on MSVC.
diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index 9a5e249..532cafc 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -437,6 +437,34 @@ acl_nontrivial (int count, struct acl *entries)
 #endif


+/* acl_extended_file() tests whether a file has an ACL.  But it can trigger
+   unnecessary autofs mounts.  In newer versions of libacl, a function
+   acl_extended_file_nofollow() is available that uses lgetxattr() and
+   therefore does not have this problem.  It is equivalent to
+   acl_extended_file(), except on symbolic links.  */
+
+static int
+acl_extended_file_wrap (char const *name)
+{
+  if ( ! HAVE_ACL_EXTENDED_FILE)
+    return -1;
+
+  if (HAVE_ACL_EXTENDED_FILE_NOFOLLOW)
+    {
+      struct stat sb;
+      if (! lstat (name, &sb) && ! S_ISLNK (sb.st_mode))
+        /* acl_extended_file_nofollow() uses lgetxattr() in order to
+           prevent unnecessary mounts.  It returns the same result as
+           acl_extended_file() since we already know that NAME is not a
+           symbolic link at this point (modulo the TOCTTOU race condition).  */
+        return acl_extended_file_nofollow (name);
+    }
+
+  /* fallback for symlinks and old versions of libacl */
+  return acl_extended_file (name);
+}
+
+
 /* Return 1 if NAME has a nontrivial access control list, 0 if NAME
    only has no or a base access control list, and -1 (setting errno)
    on error.  SB must be set to the stat buffer of NAME, obtained
@@ -454,20 +482,12 @@ file_has_acl (char const *name, struct stat const *sb)
       /* Linux, FreeBSD, MacOS X, IRIX, Tru64 */
       int ret;

-      if (HAVE_ACL_EXTENDED_FILE || HAVE_ACL_EXTENDED_FILE_NOFOLLOW) /* Linux 
*/
+      if (HAVE_ACL_EXTENDED_FILE) /* Linux */
         {
-#  if HAVE_ACL_EXTENDED_FILE_NOFOLLOW
-          /* acl_extended_file_nofollow() uses lgetxattr() in order to prevent
-             unnecessary mounts, but it returns the same result as we already
-             know that NAME is not a symbolic link at this point (modulo the
-             TOCTTOU race condition).  */
-          ret = acl_extended_file_nofollow (name);
-#  else
           /* On Linux, acl_extended_file is an optimized function: It only
              makes two calls to getxattr(), one for ACL_TYPE_ACCESS, one for
              ACL_TYPE_DEFAULT.  */
-          ret = acl_extended_file (name);
-#  endif
+          ret = acl_extended_file_wrap (name);
         }
       else /* FreeBSD, MacOS X, IRIX, Tru64 */
         {
--
1.7.7.rc0.362.g5a14


>From d813b688732c3a0da947f91cbb19cb78a627209e Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 5 Oct 2011 15:06:49 +0200
Subject: [PATCH 2/2] file-has-acl: revert both recent changes, 80af92af and
 95f7c57f

* lib/file-has-acl.c: While the 2011-10-03 change does fix the
ls -lL regression introduced in coreutils-8.12, it does so at the
cost of an additional stat call in the common case.  Besides, now
that the kernel change that prompted commit 95f7c57f has been reverted
(see https://bugzilla.redhat.com/show_bug.cgi?id=720325#c24)
we have no use for commit 95f7c57f, "file-has-acl: use
acl_extended_file_nofollow if available".
---
 ChangeLog          |   11 +++++++++++
 lib/acl-internal.h |    6 ------
 lib/file-has-acl.c |   30 +-----------------------------
 m4/acl.m4          |    2 +-
 4 files changed, 13 insertions(+), 36 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 6715d96..8d66d56 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2011-10-05  Jim Meyering  <address@hidden>
+
+       file-has-acl: revert both recent changes, 80af92af and 95f7c57f
+       * lib/file-has-acl.c: While the 2011-10-03 change does fix the
+       ls -lL regression introduced in coreutils-8.12, it does so at the
+       cost of an additional stat call in the common case.  Besides, now
+       that the kernel change that prompted commit 95f7c57f has been reverted
+       (see https://bugzilla.redhat.com/show_bug.cgi?id=720325#c24)
+       we have no use for commit 95f7c57f, "file-has-acl: use
+       acl_extended_file_nofollow if available".
+
 2011-10-03  Kamil Dudka  <address@hidden>

        file-has-acl: revert unintended change in behavior of ls -L
diff --git a/lib/acl-internal.h b/lib/acl-internal.h
index 4f7166e..7a105c8 100644
--- a/lib/acl-internal.h
+++ b/lib/acl-internal.h
@@ -133,12 +133,6 @@ rpl_acl_set_fd (int fd, acl_t acl)
 #  endif

 /* Linux-specific */
-#  ifndef HAVE_ACL_EXTENDED_FILE_NOFOLLOW
-#   define HAVE_ACL_EXTENDED_FILE_NOFOLLOW false
-#   define acl_extended_file_nofollow(name) (-1)
-#  endif
-
-/* Linux-specific */
 #  ifndef HAVE_ACL_FROM_MODE
 #   define HAVE_ACL_FROM_MODE false
 #   define acl_from_mode(mode) (NULL)
diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index 532cafc..4ff2808 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -437,34 +437,6 @@ acl_nontrivial (int count, struct acl *entries)
 #endif


-/* acl_extended_file() tests whether a file has an ACL.  But it can trigger
-   unnecessary autofs mounts.  In newer versions of libacl, a function
-   acl_extended_file_nofollow() is available that uses lgetxattr() and
-   therefore does not have this problem.  It is equivalent to
-   acl_extended_file(), except on symbolic links.  */
-
-static int
-acl_extended_file_wrap (char const *name)
-{
-  if ( ! HAVE_ACL_EXTENDED_FILE)
-    return -1;
-
-  if (HAVE_ACL_EXTENDED_FILE_NOFOLLOW)
-    {
-      struct stat sb;
-      if (! lstat (name, &sb) && ! S_ISLNK (sb.st_mode))
-        /* acl_extended_file_nofollow() uses lgetxattr() in order to
-           prevent unnecessary mounts.  It returns the same result as
-           acl_extended_file() since we already know that NAME is not a
-           symbolic link at this point (modulo the TOCTTOU race condition).  */
-        return acl_extended_file_nofollow (name);
-    }
-
-  /* fallback for symlinks and old versions of libacl */
-  return acl_extended_file (name);
-}
-
-
 /* Return 1 if NAME has a nontrivial access control list, 0 if NAME
    only has no or a base access control list, and -1 (setting errno)
    on error.  SB must be set to the stat buffer of NAME, obtained
@@ -487,7 +459,7 @@ file_has_acl (char const *name, struct stat const *sb)
           /* On Linux, acl_extended_file is an optimized function: It only
              makes two calls to getxattr(), one for ACL_TYPE_ACCESS, one for
              ACL_TYPE_DEFAULT.  */
-          ret = acl_extended_file_wrap (name);
+          ret = acl_extended_file (name);
         }
       else /* FreeBSD, MacOS X, IRIX, Tru64 */
         {
diff --git a/m4/acl.m4 b/m4/acl.m4
index ecf0384..d6a448a 100644
--- a/m4/acl.m4
+++ b/m4/acl.m4
@@ -33,7 +33,7 @@ AC_DEFUN([gl_FUNC_ACL],
            AC_CHECK_FUNCS(
              [acl_get_file acl_get_fd acl_set_file acl_set_fd \
               acl_free acl_from_mode acl_from_text \
-              acl_delete_def_file acl_extended_file acl_extended_file_nofollow 
\
+              acl_delete_def_file acl_extended_file \
               acl_delete_fd_np acl_delete_file_np \
               acl_copy_ext_native acl_create_entry_np \
               acl_to_short_text acl_free_text])
--
1.7.7.rc0.362.g5a14