On Friday 10 January 2014 13:33:38 Richard W.M. Jones
wrote:> On Fri, Jan 10, 2014 at 02:17:48PM +0100, Pino Toscano wrote:
> > > This code looks as if it will copy the xattrs, but it won't
> > >
> > > remove any which don't exist in the source. eg:
> > > source xattrs before:
> > > user.foo = 1
> > >
> > > dest xattrs before:
> > > user.bar = 2
> > >
> > > dest xattrs after:
> > > user.foo = 1
> > > user.bar = 2
> > >
> > > That may or may not be what we want, but I would say it's a
bit
> > > unexpected.
> >
> > Yes, the current behaviour is done on purpose; my thought that,
> > given
> > the command is "copy attributes", it would just copy what
specified
> > from the source to the destination.
> >
> > I see reasons for both the behaviours, so I'm not totally sure
which
> > one pick.
>
> On the basis that most users will be creating a new file (which will
> have no xattrs except in some very odd corner cases), just leave your
> implementation for now, but don't specify it in the documentation so
> we could change it later.
After all, the documentation says that it just copies the xattrs from
the source to the destination, but it does not explicitly mention what
is done with the xattrs in the destination not in the source ... :-)
On Friday 10 January 2014 13:36:06 Richard W.M. Jones
wrote:> On Fri, Jan 10, 2014 at 01:33:38PM +0000, Richard W.M. Jones wrote:
> > The API is now pretty confusing. Each OBool flag is really a
> > tristate. It can either be "true", "false" or
"not specified".
I see, I did not pay much attention to the use of optargs_bitmask
outside the auto-generated RPC stuff the daemon code.
> > Therefore I think it should be:
> > xattributes:true # copies only xattrs and nothing else
> > all:true # copies everything
> > all:true xattributes:false # copies everything except xattrs
> >
> > In other words, 'all' changes the default (ie. "not
specified")
> > state
> > of the other flags.
Given the above, this indeed becomes straightforward to have.
> > To be clearer, the four OBool parameters would be:
> > [OBool "all"; OBool "mode"; OBool
"ownership"; OBool
> > "xattributes"]
>
> So looking at your v2 patch a bit more, I think this is what you
> already did, except that "skipmode" is backwards from the other
flags.
> I think we're in agreement, except I think "skipmode" should
just
> become "mode" and not have negated meaning compared to the other
> flags.
OK, I will turn it back as it was before (into "mode"), but making use
of the tristate information to have it true by default.
> We're allowed to extend the API later by adding optional flags (see
> "Note about extending functions" in generator/README for the
> complicated rules about extending APIs while preserving binary
> compatibility).
Yes, I read it earlier, that's why I'm not that concerned about adding
all the potential attributes now.
Attached there is the v3 of the patch.
--
Pino Toscano