Belated review below...
Kamil Dudka wrote:
> From 1c81d575178528f3553b8f4e58a41d2bdf81a010 Mon Sep 17 00:00:00 2001
> From: Kamil Dudka <kdu...@redhat.com>
> 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.
> +...@itemx 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.
_______________________________________________
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
