Re: [PATCH] cp/mv: xattr support

[Pádraig Brady]

Belated review below...

Kamil Dudka wrote:
> From 1c81d575178528f3553b8f4e58a41d2bdf81a010 Mon Sep 17 00:00:00 2001
> From: Kamil Dudka <address@hidden>
> Date: Tue, 18 Nov 2008 16:27:44 +0100
> Subject: [PATCH] cp/mv: add xattr support
> 
> (copy_attr_quote): New function to quote file name in errors messages
> 

s/errors/error/

> 
> diff --git a/NEWS b/NEWS
> index cbea67c..305dcc6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,8 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>  
>  ** New features
>  
> +  cp/mv: add xattr support, new option --preserve=xattr in cp
> +
> 

I would say:
   Add extended attribute support available on certain filesystems
   like ext2 and XFS.
     cp: Tries to copy xattrs when --preserve=xattr specified
     mv: Always tries to copy xattrs
     install: Never copies xattrs

> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 935129f..95cec82 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -7372,6 +7372,9 @@ Preserve in the destination files
>  any links between corresponding source files.
>  @c Give examples illustrating how hard links are preserved.
>  @c Also, show how soft links map to hard links with -L and -H.
> address@hidden xattr
> +Preserve extended (xattr) attributes if coreutils is compiled with xattr
> +support.
> 

I would expand to:
 Preserve extended attributes if @command{cp} is built with xattr support,
 and xattrs are supported and enabled on your file system.

If SELinux contexts and ACLs are implemented using xattrs,
does --preserve=xattr copy these also, or does it just copy
particular namespaces?  Worth clarifying in any case.

I'm a bit surprised that -a doesn't copy xattrs actually:
-a does copy ACLs (but doesn't copy [selinux] context),
but I guess if xattrs are a superset of SELinux context,
then that's consistent.

>  @itemx all
>  Preserve all file attributes.
>  Equivalent to specifying all of the above.

I would also mention how `mv` and `install` handle xattrs.
Also add indexes for "extended attributes", "xattr".

Also probably not for this patch, but I would also add
indexes for ACL to mentions of "access control".
Also there is no description for --preserve=context?

> diff --git a/src/copy.c b/src/copy.c
> index bc1b20e..d057778 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -54,6 +54,13 @@
>  #include "areadlink.h"
>  #include "yesno.h"

Don't we need this?
#ifndef USE_XATTR
# define USE_XATTR 0
#endif

> +#if USE_XATTR
> +# include <attr/error_context.h>
> +# include <attr/libattr.h>
> +# include <stdarg.h>
> +# include "verror.h"
> +#endif

> diff --git a/src/cp.c b/src/cp.c
> index 95eba0c..9b081d4 100644
> --- a/src/cp.c
> +++ b/src/cp.c
> @@ -197,7 +197,8 @@ Mandatory arguments to long options are mandatory for 
> short options too.\n\
>    -p                           same as 
> --preserve=mode,ownership,timestamps\n\
>        --preserve[=ATTR_LIST]   preserve the specified attributes (default:\n\
>                                   mode,ownership,timestamps), if possible\n\
> -                                 additional attributes: context, links, 
> all\n\
> +                                 additional attributes: context, links, 
> xattr,\n\
> +                                 all\n\
>  "), stdout);
>        fputs (_("\
>        --no-preserve=ATTR_LIST  don't preserve the specified attributes\n\
> @@ -774,6 +775,7 @@ cp_option_init (struct cp_options *x)
>    x->preserve_timestamps = false;
>    x->preserve_security_context = false;
>    x->require_preserve_context = false;
> +  x->preserve_xattr = false;

Do we need a require_preserve_xattr?
I.E. should all these be tristate instead of booleans?

> diff --git a/tests/misc/xattr b/tests/misc/xattr
> new file mode 100755
> index 0000000..2f50134
> --- /dev/null
> +++ b/tests/misc/xattr
> @@ -0,0 +1,80 @@
> +#!/bin/sh
> +# Ensure that cp --preserve=xattr and mv preserve extended attributes.

we should add a test for `install` to ensure/make obvious
it doesn't preserve xattrs.

> +# Skip this test if cp was built without xattr support:
> +grep '^#define USE_XATTR 1' $CONFIG_HEADER > /dev/null ||
> +  skip_test_ "coreutils built without xattr support"

I'd rather test a binary as it's little less coupled I think:

cp --preserve=xattr --help >/dev/null 2>&1 ||
  skip_test_ "coreutils built without xattr support"

> +# mv should preserve xattr when not copying content
> +# (implicit and probably expected behavior)

This confused me a little. I would change to:

# mv should preserve xattr when renaming within a filesystem.
# This is implicitly done by rename() and doesn't need explicit
# xattr support in mv.

> +mv a b || fail=1
> +getfattr -d b > out_b || skip_test_ "failed to get xattr of file"
> +grep -F "$xattr_pair" out_b > /dev/null || fail=1

should we fail here, if rename() doesn't preserve xattrs?
probably should be just a warning, as we've no control
over what rename() does.

cheers,
Pádraig.