bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] cp/mv: xattr support


From: Kamil Dudka
Subject: Re: [PATCH] cp/mv: xattr support
Date: Tue, 20 Jan 2009 17:15:37 +0100
User-agent: KMail/1.9.6 (enterprise 0.20071012.724442)

Hello Pádraig,

thank you for review.

On Tuesday 20 January 2009 11:36:20 Pádraig Brady wrote:
> 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.
I will find out more information and try to document this behavior. Note 
acl/selinux/xattr support can be compiled or not individually in coreutils.

> 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?
Not yet, maybe subject for another patch.

> Don't we need this?
> #ifndef USE_XATTR
> # define USE_XATTR 0
> #endif
I don't think so. USE_XATTR is defined in both cases by gl_FUNC_XATTR.

> Do we need a require_preserve_xattr?
> I.E. should all these be tristate instead of booleans?
So you want cp to fail on attr_copy_{file,fd} fail with 
option --preserve=xattr, but not with option --preserve=all? No problem I 
think.

Tristate means enum?

Note the current description of require_preserve_context in copy.h is actually 
wrong as the context is not preserved by "cp -a".

> > 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"
Not possible now because --preserve=xattr is not treated as error even if 
coreutils is compiled without xattr support. Do you it should be treated as 
error?


Kamil

Attachment: 0001-cp-mv-add-xattr-support.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]