On Mon October 3 2011 15:02:20 Bruno Haible wrote:
> The function name should explain the semantics of the function. The fact
> that it's a wrapper around acl_extended_file is something one can see by
> reading the code.
> 
> Maybe call it acl_extended_file_optimized?

Sounds good.

> > +    /* 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).  */
> 
> I have a hard time understanding this comment.
> - Why the "but"? The "same result" as what?

Fixed.  I am not a native speaker.

> - What is the "TOCTTOU race condition"?

TOC = checking whether the file is a symlink
TOU = checking whether the file has ACLs

> If it's important, please add a FIXME and an explanation.

No, it does not feel important to me, at least not for ls.

> How about this comment, right after the inner #if: 1. Describe the problem.
> 2. Describe the solution.
> 
>   /* 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.  */

Comment replaced.  Thanks for the suggestion.

Kamil
From 25b63ddf75d2ca5c86e8498f54ee9001b72c4c2c Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdu...@redhat.com>
Date: Mon, 3 Oct 2011 12:17:22 +0200
Subject: [PATCH] file-has-acl: revert unintended change in behavior of ls -L

* lib/file-has-acl.c (acl_extended_file_wrap): A wrapper around
acl_extended_file () that allows to call acl_extended_file_nofollow ()
only if the function is available and the file is not a symbolic link.
(file_has_acl): Remove code that caused problems.  Call
acl_extended_file_wrap ().
---
 ChangeLog          |    9 +++++++++
 lib/file-has-acl.c |   39 +++++++++++++++++++++++++++++----------
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a6d363a..81d9b93 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+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): A wrapper around
+	acl_extended_file () that allows to call acl_extended_file_nofollow ()
+	only if the function is available and the file is not a symbolic link.
+	(file_has_acl): Remove code that caused problems.  Call
+	acl_extended_file_wrap ().
+
 2011-10-01  Jim Meyering  <meyer...@redhat.com>
 
 	maint.mk: adjust a release-related rule not to require use of gzip
diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index 1b7e392..a92189f 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -436,6 +436,33 @@ 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_optimized (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)
@@ -453,20 +480,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_optimized (name);
         }
       else /* FreeBSD, MacOS X, IRIX, Tru64 */
         {
-- 
1.7.4.4

Reply via email to