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 <kdu...@redhat.com>
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 <kdu...@redhat.com>
+
+ 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 <br...@clisp.org>
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 <meyer...@redhat.com>
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 <meyer...@redhat.com>
+
+ 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 <kdu...@redhat.com>
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
