[PATCH] ACL handling simplification + support for NFSv4

[PATCH] ACL handling simplification + support for NFSv4

[Ondrej Valousek]

The following patch is aiming to improve & simplify ACL handling in copy-acl.c
Changes/Benefits:
* we no longer try to decode ACLs, instead we just copy the whole xattr with ACLs
making the code simpler and faster
* side effect is support for NFSv4 acls
Disadvantages:
* I agree the patch is maybe too aggresive, but works suprisingly well, but let me know
if I missed something important
* it pulls in dependency on libattr (so needs to be linked with -lattr, the automake
changes are not yet included in this patch), but on the other way, we could possibly ditch
dependency on libacl.
* we can't do any ACLs conversions, but these are not (AFAIK) being done anyway in the
old code

As I said, my goal is really for "cp -p" to preserve ACLs regardless of the type.
I tried to experiment with variety of filesystems (posix and nfs) and it seems to me
to work exactly the way the old code worked (plus handy support for NFSv4).

Ondrej

---
 lib/copy-acl.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/copy-acl.c b/lib/copy-acl.c
index 5fc42b7f6..341d1f605 100644
--- a/lib/copy-acl.c
+++ b/lib/copy-acl.c
@@ -29,6 +29,20 @@
 #define _(msgid) gettext (msgid)
 
 
+#if USE_XATTR
+
+# include <attr/libattr.h>
+
+static int
+copy_attr_permissions (const char *name, struct error_context *ctx)
+{
+	int action = attr_copy_action (name, ctx);
+	return action == ATTR_ACTION_PERMISSIONS;
+}
+
+#endif  /* USE_XATTR */
+
+
 /* Copy access control lists from one file to another. If SOURCE_DESC is
    a valid file descriptor, use file descriptor operations, else use
    filename based operations on SRC_NAME. Likewise for DEST_DESC and
@@ -43,7 +57,22 @@ int
 copy_acl (const char *src_name, int source_desc, const char *dst_name,
           int dest_desc, mode_t mode)
 {
-  int ret = qcopy_acl (src_name, source_desc, dst_name, dest_desc, mode);
+  int ret;
+#ifdef USE_XATTR
+  ret = chmod_or_fchmod (dst_name, dest_desc, mode);
+  /* Rather than fiddling with acls one by one, we just copy the whole ACL xattrs 
+   * (Posix or NFSv4). Of course, that won't address ACLs conversion 
+   * (i.e. posix <-> nfs4) but we can't do it anyway, so for now, we don't care
+   */
+  if(ret == 0)
+    ret = source_desc <= 0 && dest_desc <= 0
+      ? attr_copy_file (src_name, dst_name, copy_attr_permissions, NULL)
+      : attr_copy_fd (src_name, source_desc, dst_name, dest_desc, copy_attr_permissions, NULL);
+#else
+  /* no XATTR, so we proceed the old dusty way */
+  ret = qcopy_acl (src_name, source_desc, dst_name, dest_desc, mode);
+#endif
+
   switch (ret)
     {
     case -2:

Re: [PATCH] ACL handling simplification + support for NFSv4

[Paul Eggert]

On 2022-12-30 01:18, Ondrej Valousek wrote:

> * we no longer try to decode ACLs, instead we just copy the whole xattr with ACLs
> making the code simpler and faster

What happens if you try to copy ACLs from a filesystem using NFSv4 ACLs 
to one using POSIXish ACLs, or vice versa?

> * it pulls in dependency on libattr (so needs to be linked with -lattr, the automake
> changes are not yet included in this patch), but on the other way, we could possibly ditch
> dependency on libacl.

This would all have to be done, yes.


> +#ifdef USE_XATTR
> +  ret = chmod_or_fchmod (dst_name, dest_desc, mode);

Why is this call needed? Won't a successful attr_copy_file mean that the 
chmod_or_fchmod is unnecessary? I.e., can't we do the chmod_or_lchmod 
only if the attr_copy_file fails with a particular errno value that says 
"this file lacks extended attributes"?

> +    ret = source_desc <= 0 && dest_desc <= 0
> +      ? attr_copy_file (src_name, dst_name, copy_attr_permissions, NULL)
> +      : attr_copy_fd (src_name, source_desc, dst_name, dest_desc, copy_attr_permissions, NULL);

When these functions fail, what are their errno values and should we 
treat any of them specially? (I can't easily find documentation for 
these functions.)

RE: [PATCH] ACL handling simplification + support for NFSv4

[Ondrej Valousek]

> What happens if you try to copy ACLs from a filesystem using NFSv4 ACLs to one using POSIXish ACLs, or vice versa?
It fails, we can't do ACL conversion.

> Why is this call needed? Won't a successful attr_copy_file mean that the chmod_or_fchmod is unnecessary? I.e., can't we do the chmod_or_lchmod only if the attr_copy_file fails with a particular errno value that says "this file lacks extended attributes"?
No. ACL only substitute the lower mode bits, we still need to copy the (possible) sticky bits. We also need to copy mode in case no ACL (i.e. no xattrs to copy) presented.

> When these functions fail, what are their errno values and should we treat any of them specially? (I can't easily find documentation for these functions.)
It returns -1, so it behaves the same way as the original function - we do not need to treat it differently.