Kamil Dudka wrote: > 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, I've merged your two comment changes, kept my commit-log and ChangeLog and rebased past Bruno's latest change. This is what I expect to push: >From 603b1e3b16894cac183952b0b0fe379749a3aebe 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): 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 7f9e391..4965785 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> nonblocking tests: Fix test failure on OpenBSD/SPARC64. 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
