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

[Bruno Haible <bruno@clisp.org>]

Hi Ondrej,

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

This function — like all functions — deserves a comment before the
function. I would propose:

/* Returns 1 if NAME is the name of an extended attribute that is related
   to permissions, i.e. ACLs.  Returns 0 otherwise.  */

In view of this description, 'copy_attr_permissions' is a strange name
for a function that returns a boolean value. How about renaming it to
'is_permissions_attr' ?

> +{
> +	int action = attr_copy_action (name, ctx);
> +	return action == ATTR_ACTION_PERMISSIONS;

Gnulib style: Please indent with spaces, not with tabs.

>  /* 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)

GNU coding style: Please add a space after 'if'.

> +    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

Instead of putting this code into copy_acl, I think qcopy_acl would be the
better place. The difference between qcopy_acl and copy_acl is, per definition,
that the former is silent (does not write to stdout or stderr), whereas the
latter signals errors.

> diff --git a/m4/xattr.m4 b/m4/xattr.m4
> new file mode 100644
> index 000000000..5f9248939
> --- /dev/null
> +++ b/m4/xattr.m4

> +  AC_DEFINE_UNQUOTED([USE_XATTR], [`test $use_xattr != yes; echo $?`],

The use of backquotes here is ugly. How about simplifying it by use of a
shell variable? E.g.

     if test $use_xattr = yes; then
       use_xattr_value=1
     else
       use_xattr_value=0
     fi
     AC_DEFINE_UNQUOTED([USE_XATTR], [$use_xattr_value])

> diff --git a/modules/acl b/modules/acl
> index 1a3a14e6c..ca2239823 100644
> --- a/modules/acl
> +++ b/modules/acl
> @@ -4,6 +4,7 @@ Access control lists of files, with diagnostics.  (Unportable.)
>  Files:
>  lib/copy-acl.c
>  lib/set-acl.c
> +m4/xattr.m4
>  
>  Depends-on:
>  error

This change would go into modules/qcopy-acl, not modules/acl.

And gl_FUNC_XATTR needs to be invoked from somewhere, otherwise all of
m4/xattr.m4 is dead code.

Bruno