On 2024-09-30 06:30, Bruno Haible wrote:
Did you mean 152 ? Or maybe 512 ?
I used "152" because that is what file-has-acl previously used. I
installed the first patch to explain this better.
I installed the 2nd patch as an optimization discovered after more
testing with coreutils.
Further improvements are in the pipeline to fix some coreutils bugs I
introduced with all this. These will involve using dirent stuff.
Speaking of dirent, I see that POSIX.1-2024 has a new function
posix_getdents that a quick web search suggests is only in Cygwin and
musl now. It would simplify parts of Gnulib, I think. But it can wait.From 0c55c3d7f74d42d273e0fffc8b774344eed65458 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 30 Sep 2024 09:24:09 -0700
Subject: [PATCH 1/2] file-has-acl: improve acl.h comment
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/acl.h: Improve comment about ‘152’.
---
lib/acl.h | 5 ++++-
lib/file-has-acl.c | 3 +--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/lib/acl.h b/lib/acl.h
index 07ab04250d..1a627323ec 100644
--- a/lib/acl.h
+++ b/lib/acl.h
@@ -65,7 +65,10 @@ struct aclinfo
int err;
/* A small array of char, big enough for most listxattr results.
- The size is somewhat arbitrary. For internal use only. */
+ The size is somewhat arbitrary; it equals the max length of a
+ trivial NFSv4 ACL (a size used by file-has-acl.c in 2023-2024
+ but no longer relevant now), and a different value might be
+ better once experience is gained. For internal use only. */
char __gl_acl_ch[152];
} u;
};
diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index 89faba565f..6924a616bd 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -349,8 +349,7 @@ file_has_aclinfo (char const *name, struct stat const *sb,
|| (S_ISDIR (sb->st_mode)
&& aclinfo_has_xattr (ai, XATTR_NAME_POSIX_ACL_DEFAULT))));
- /* If there is an NFSv4 ACL, follow up with a getxattr syscall
- to see whether the NFSv4 ACL is nontrivial. */
+ /* If there is an NFSv4 ACL, check whether it is nontrivial. */
if (nfsv4_acl)
{
/* A buffer large enough to hold any trivial NFSv4 ACL.
--
2.43.0
From 276260e7ec35181b0ea9fddf5bab397cf1061eca Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 30 Sep 2024 10:46:57 -0700
Subject: [PATCH 2/2] file-has-acl: improve performance on Linux symlinks
* lib/file-has-acl.c (USE_LINUX_XATTR): Now can be used outside #if.
(file_has_aclinfo): As an optimization on Linux, do not attempt to
get extended attributes on symlinks, as this will always fail.
Also, use lgetxattr (not getxattr) and acl_extended_file_nofollow
(not acl_extended_file) when not following symlinks, to avoid some
races.
---
ChangeLog | 10 ++++
lib/file-has-acl.c | 143 ++++++++++++++++++++++++++-------------------
2 files changed, 92 insertions(+), 61 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 0679ed28ef..f971059a0a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2024-09-30 Paul Eggert <egg...@cs.ucla.edu>
+
+ file-has-acl: improve performance on Linux symlinks
+ * lib/file-has-acl.c (USE_LINUX_XATTR): Now can be used outside #if.
+ (file_has_aclinfo): As an optimization on Linux, do not attempt to
+ get extended attributes on symlinks, as this will always fail.
+ Also, use lgetxattr (not getxattr) and acl_extended_file_nofollow
+ (not acl_extended_file) when not following symlinks, to avoid some
+ races.
+
2024-09-30 Bruno Haible <br...@clisp.org>
selinux-h: Fix two syntax errors.
diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index 6924a616bd..944f0d2200 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -33,7 +33,11 @@
static char const UNKNOWN_SECURITY_CONTEXT[] = "?";
-#define USE_LINUX_XATTR (USE_ACL && HAVE_LINUX_XATTR_H && HAVE_LISTXATTR)
+#if USE_ACL && HAVE_LINUX_XATTR_H && HAVE_LISTXATTR
+# define USE_LINUX_XATTR true
+#else
+# define USE_LINUX_XATTR false
+#endif
#if USE_LINUX_XATTR
# if USE_SELINUX_SELINUX_H
@@ -320,80 +324,95 @@ acl_nfs4_nontrivial (uint32_t *xattr, ssize_t nbytes)
SB must be set to the stat buffer of NAME,
obtained through stat() or lstat().
Set *AI to the ACL info if available.
- If FLAGS & AT_SYMLINK_FOLLOW, SB was gotten via stat and follow symlinks;
- otherwise, SB was gotten vi lstat and do not follow them. */
+ If FLAGS & AT_SYMLINK_FOLLOW, SB was gotten via stat, otherwise lstat.
+ Also, if FLAGS & AT_SYMLINK_FOLLOW, follow symlinks when retrieving
+ ACL info, otherwise do not follow them if possible. */
int
file_has_aclinfo (char const *name, struct stat const *sb,
struct aclinfo *ai, int flags)
{
-#if USE_LINUX_XATTR
- int initial_errno = errno;
- get_aclinfo (name, ai, flags);
+ /* Symbolic links lack extended attributes and ACLs on all supported
+ platforms, so don't bother trying to fetch them. If the platform
+ supports not following symlinks this defends against some races
+ and avoids a syscall; otherwise this is essential.
+
+ First, initialize *AI for cases known to be unsupported. */
- if (ai->size <= 0)
+ if (!USE_LINUX_XATTR || S_ISLNK (sb->st_mode))
{
- errno = ai->size < 0 ? ai->u.err : initial_errno;
- return ai->size;
+ ai->buf = ai->u.__gl_acl_ch;
+ ai->size = -1;
+ ai->u.err = ENOTSUP;
+ ai->scontext = (char *) UNKNOWN_SECURITY_CONTEXT;
+ ai->scontext_err = ENOTSUP;
}
- else
+
+ /* Now set *AI for cases that might be supported, then check for ACLs. */
+
+#if USE_ACL
+ if (! S_ISLNK (sb->st_mode))
{
- /* In Fedora 39, a file can have both NFSv4 and POSIX ACLs,
- but if it has an NFSv4 ACL that's the one that matters.
- In earlier Fedora the two types of ACLs were mutually exclusive.
- Attempt to work correctly on both kinds of systems. */
- bool nfsv4_acl = aclinfo_has_xattr (ai, XATTR_NAME_NFSV4_ACL);
- int ret
- = (ai->size <= 0 ? ai->size
- : (nfsv4_acl
- || aclinfo_has_xattr (ai, XATTR_NAME_POSIX_ACL_ACCESS)
- || (S_ISDIR (sb->st_mode)
- && aclinfo_has_xattr (ai, XATTR_NAME_POSIX_ACL_DEFAULT))));
-
- /* If there is an NFSv4 ACL, check whether it is nontrivial. */
- if (nfsv4_acl)
+
+# if USE_LINUX_XATTR
+ int initial_errno = errno;
+ get_aclinfo (name, ai, flags);
+
+ if (ai->size <= 0)
{
- /* A buffer large enough to hold any trivial NFSv4 ACL.
- The max length of a trivial NFSv4 ACL is 6 words for owner,
- 6 for group, 7 for everyone, all times 2 because there are both
- allow and deny ACEs. There are 6 words for owner because of
- type, flag, mask, wholen, "OWNER@"+pad and similarly for group;
- everyone is another word to hold "EVERYONE@". */
- uint32_t buf[2 * (6 + 6 + 7)];
-
- ret = getxattr (name, XATTR_NAME_NFSV4_ACL, buf, sizeof buf);
- if (ret < 0)
- switch (errno)
- {
- case ENODATA: return 0;
- case ERANGE : return 1; /* ACL must be nontrivial. */
- }
- else
+ errno = ai->size < 0 ? ai->u.err : initial_errno;
+ return ai->size;
+ }
+ else
+ {
+ /* In Fedora 39, a file can have both NFSv4 and POSIX ACLs,
+ but if it has an NFSv4 ACL that's the one that matters.
+ In earlier Fedora the two types of ACLs were mutually exclusive.
+ Attempt to work correctly on both kinds of systems. */
+ bool nfsv4_acl = aclinfo_has_xattr (ai, XATTR_NAME_NFSV4_ACL);
+ int ret
+ = (ai->size <= 0 ? ai->size
+ : (nfsv4_acl
+ || aclinfo_has_xattr (ai, XATTR_NAME_POSIX_ACL_ACCESS)
+ || (S_ISDIR (sb->st_mode)
+ && aclinfo_has_xattr (ai, XATTR_NAME_POSIX_ACL_DEFAULT))));
+
+ /* If there is an NFSv4 ACL, check whether it is nontrivial. */
+ if (nfsv4_acl)
{
- /* It looks like a trivial ACL, but investigate further. */
- ret = acl_nfs4_nontrivial (buf, ret);
+ /* A buffer large enough to hold any trivial NFSv4 ACL.
+ The max length of a trivial NFSv4 ACL is 6 words for owner,
+ 6 for group, 7 for everyone, all times 2 because there are both
+ allow and deny ACEs. There are 6 words for owner because of
+ type, flag, mask, wholen, "OWNER@"+pad and similarly for group;
+ everyone is another word to hold "EVERYONE@". */
+ uint32_t buf[2 * (6 + 6 + 7)];
+
+ ret = ((flags & ACL_SYMLINK_FOLLOW ? getxattr : lgetxattr)
+ (name, XATTR_NAME_NFSV4_ACL, buf, sizeof buf));
if (ret < 0)
+ switch (errno)
+ {
+ case ENODATA: return 0;
+ case ERANGE : return 1; /* ACL must be nontrivial. */
+ }
+ else
{
- errno = EINVAL;
- return ret;
+ /* It looks like a trivial ACL, but investigate further. */
+ ret = acl_nfs4_nontrivial (buf, ret);
+ if (ret < 0)
+ {
+ errno = EINVAL;
+ return ret;
+ }
+ errno = initial_errno;
}
- errno = initial_errno;
}
+ if (ret < 0)
+ return - acl_errno_valid (errno);
+ return ret;
}
- if (ret < 0)
- return - acl_errno_valid (errno);
- return ret;
- }
-#else
- ai->size = -1;
- ai->u.err = ENOTSUP;
- ai->scontext = (char *) UNKNOWN_SECURITY_CONTEXT;
- ai->scontext_err = ENOTSUP;
-#endif
-#if USE_ACL
- if (! S_ISLNK (sb->st_mode))
- {
-# if HAVE_ACL_GET_FILE
+# elif HAVE_ACL_GET_FILE
/* POSIX 1003.1e (draft 17 -- abandoned) specific version. */
/* Linux, FreeBSD, Mac OS X, IRIX, Tru64, Cygwin >= 2.5 */
@@ -404,7 +423,10 @@ file_has_aclinfo (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 (name);
+ ret = ((flags & ACL_SYMLINK_FOLLOW
+ ? acl_extended_file
+ : acl_extended_file_nofollow)
+ (name));
}
else /* FreeBSD, Mac OS X, IRIX, Tru64, Cygwin >= 2.5 */
{
@@ -815,7 +837,6 @@ file_has_aclinfo (char const *name, struct stat const *sb,
return acl_nontrivial (count, entries);
}
}
-
# endif
}
#endif
--
2.43.0
