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
