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

[Paul Eggert]

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 
> 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);

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.