[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Use xattr (Linux) in copy-acl.c

From: Paul Eggert
Subject: Re: [PATCH] Use xattr (Linux) in copy-acl.c
Date: Tue, 3 Jan 2023 12:40:58 -0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2

Thanks again for looking into this.

On 2023-01-03 06:08, Ondrej Valousek wrote:

+static int
+copy_attr_permissions (const char *name, struct error_context *ctx)
+       int action = attr_copy_action (name, ctx);
+       return action == ATTR_ACTION_PERMISSIONS;

This returns bool not int, so it should be declared to return bool. The body can be a one-liner, no?

+#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 
+   * (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);

Please add brief commentary as to why we need both chmod_or_fchmod and attr_copy_* (i.e., why it doesn't suffice to just do the attr_copy_*), and why it's important to do chmod_or_fchmod before doing attr_copy_*?

Also, please use Gnulib style in comments; no "*" on each line.

Also, the test "source_desc <= 0 && dest_desc <= 0" looks wrong. Shouldn't it be "source_desc < 0 || dest_desc < 0"?

+    if test $ac_cv_header_attr_libattr_h = yes \
+        && test $ac_cv_header_attr_error_context_h = yes; then

Although quoting inside "" isn't needed for shell variables that you know have only safe characters, these ac_cv_* shell var uses should be quoted inside "" for safety, e.g., 'test "$ac_cv_header_attr_libattr_h" = yes', in case the user screws up and puts spaces into their values. Please do this systematically for ac_cv_* variables.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]