Re: [Libguestfs] RFC: copy-attributes command

Pino Toscano
2014-01-10T13:17:48Z

On Tuesday 07 January 2014 21:04:36 Richard W.M. Jones
wrote:
> On Tue, Jan 07, 2014 at 04:06:43PM +0100, Pino Toscano wrote:
> > Hi,
> >
> > attached there is a prototype of patch for adding a new
> > copy-attributes command. Such command would allow copy the
> > attributes of a "file" to>
> > another, so for example in guestfish:
> >   copy-attributes foo bar permissions:true xattributes:false
> >
> > would only copy the permissions of foo to bar, not copying its
> > extended attributes too.
>
> I think the general idea of the new API is fine.
>
> More comments about the code below.
>
> > Just few notes:
> > - my first daemon command, so possibly I could be missing something
> > - copy_xattrs is in xattr.c to avoid spreading the usage of xattr
> > API in>
> >   many places
> >
> > - copy_xattrs does a bit of code repetition with other stuff in
> > xattr.c,>
> >   but I'm not sure how to avoid it without making the xattr
listing
> >   code (getxattrs) a bit more complex that what it is already
> >
> > Comments?
> >
> >
> > diff --git a/daemon/daemon.h b/daemon/daemon.h
> > index b77d764..6535658 100644
> > --- a/daemon/daemon.h
> > +++ b/daemon/daemon.h
> > @@ -231,6 +231,9 @@ extern void journal_finalize (void);
> >
> >  /*-- in proto.c --*/
> >  extern void main_loop (int sock) __attribute__((noreturn));
> >
> > +/*-- in xattr.c --*/
> > +extern int copy_xattrs (const char *src, const char *dest);
> > +
> >
> >  /* ordinary daemon functions use these to indicate errors
> >
> >   * NB: you don't need to prefix the string with the current
> >   command,
> >   * it is added automatically by the client-side RPC stubs.
> >
> > diff --git a/daemon/file.c b/daemon/file.c
> > index f348f87..fd6da71 100644
> > --- a/daemon/file.c
> > +++ b/daemon/file.c
> > @@ -28,6 +28,7 @@
> >
> >  #include "guestfs_protocol.h"
> >  #include "daemon.h"
> >  #include "actions.h"
> >
> > +#include "optgroups.h"
> >
> >  GUESTFSD_EXT_CMD(str_file, file);
> >  GUESTFSD_EXT_CMD(str_zcat, zcat);
> >
> > @@ -584,3 +585,46 @@ do_filesize (const char *path)
> >
> >    return buf.st_size;
> >
> >  }
> >
> > +
> > +int
> > +do_copy_attributes (const char *src, const char *dest, int
> > permissions, int xattributes) +{
> > +  int r;
> > +  struct stat srcstat, deststat;
> > +
> > +  CHROOT_IN;
> > +  r = stat (src, &srcstat);
> > +  CHROOT_OUT;
> > +
> > +  if (r == -1) {
> > +    reply_with_perror ("stat: %s", src);
> > +    return -1;
> > +  }
> > +
> > +  CHROOT_IN;
> > +  r = stat (dest, &deststat);
> > +  CHROOT_OUT;
> > +
> > +  if (r == -1) {
> > +    reply_with_perror ("stat: %s", dest);
> > +    return -1;
> > +  }
> > +
> > +  if (permissions && ((srcstat.st_mode & 0777) !=
(deststat.st_mode
> > & 0777))) { +    CHROOT_IN;
> > +    r = chmod (dest, (srcstat.st_mode & 0777));
>
> I suspect you want 07777 in order to copy sticky/setuid/setgid bits.
> Perhaps those should be a separate flag, but we definitely want to
> copy them!

Right, fixed to be part of the permissions (or mode actually, see
below).

> > +  ssize_t len, vlen, ret;
> > +  CLEANUP_FREE char *buf = NULL, *attrval = NULL;
> > +  size_t i, attrval_len = 0;
> > +
> > +  CHROOT_IN;
> > +  len = listxattr (src, NULL, 0);
> > +  CHROOT_OUT;
> > +  if (len == -1) {
> > +    reply_with_perror ("listxattr: %s", src);
> > +    goto error;
> > +  }
> > +
> > +  buf = malloc (len);
> > +  if (buf == NULL) {
> > +    reply_with_perror ("malloc");
> > +    goto error;
> > +  }
> > +
> > +  CHROOT_IN;
> > +  len = listxattr (src, buf, len);
> > +  CHROOT_OUT;
> > +  if (len == -1) {
> > +    reply_with_perror ("listxattr: %s", src);
> > +    goto error;
> > +  }

This two-pass snippet to do (l)listxattr is already elsewhere in
xattr.c, I will move it to an own function.

> > +  /* What we get from the kernel is a string
"foo\0bar\0baz" of
> > length +   * len.
> > +   */
> > +  for (i = 0; i < (size_t) len; i += strlen (&buf[i]) + 1) {
> > +    CHROOT_IN;
> > +    vlen = getxattr (src, &buf[i], NULL, 0);
> > +    CHROOT_OUT;
> > +    if (vlen == -1) {
> > +      reply_with_perror ("getxattr: %s, %s", src,
&buf[i]);
> > +      goto error;
> > +    }
> > +
> > +    if (vlen > XATTR_SIZE_MAX) {
> > +      /* The next call to getxattr will fail anyway, so ... */
> > +      reply_with_error ("extended attribute is too large");
> > +      goto error;
> > +    }
> > +
> > +    if (vlen > attrval_len) {
> > +      char *new = realloc (attrval, vlen);
> > +      if (new == NULL) {
> > +        reply_with_perror ("realloc");
> > +        goto error;
> > +      }
> > +      attrval = new;
> > +      attrval_len = vlen;
> > +    }
> > +
> > +    CHROOT_IN;
> > +    vlen = getxattr (src, &buf[i], attrval, vlen);
> > +    CHROOT_OUT;
> > +    if (vlen == -1) {
> > +      reply_with_perror ("getxattr: %s, %s", src,
&buf[i]);
> > +      goto error;
> > +    }
> > +
> > +    CHROOT_IN;
> > +    ret = setxattr (dest, &buf[i], attrval, vlen, 0);
> > +    CHROOT_OUT;
> > +    if (vlen == -1) {
> > +      reply_with_perror ("setxattr: %s, %s", dest,
&buf[i]);
> > +      goto error;
> > +    }
> > +  }
>
> 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.

> > diff --git a/fish/test-file-attrs.sh b/fish/test-file-attrs.sh
> > new file mode 100755
> > index 0000000..85c6d29
> > --- /dev/null
> > +++ b/fish/test-file-attrs.sh
> > @@ -0,0 +1,118 @@
> > +#!/bin/bash -
> > +# libguestfs
> > +# Copyright (C) 2014 Red Hat Inc.
> > +#
> > +# This program is free software; you can redistribute it and/or
> > modify
> > +# it under the terms of the GNU General Public License as
> > published by
> > +# the Free Software Foundation; either version 2 of
> > the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write to the Free Software
> > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > 02110-1301 USA.
> > +
> > +# Test guestfish file attributes commands (chmod, copy-attributes,
> > etc). +
> > +set -e
> > +export LANG=C
> > +
> > +rm -f test.out
> > +
> > +$VG ./guestfish > test.out <<EOF
> > +scratch 50MB
> > +run
> > +part-disk /dev/sda mbr
> > +mkfs ext2 /dev/sda1
> > +mount /dev/sda1 /
> > +
> > +touch /foo
> > +touch /bar
> > +chmod 0712 /foo
> > +stat /foo | grep mode:
> > +copy-attributes /foo /bar permissions:true
> > +stat /bar | grep mode:
> > +EOF
> > +
> > +if [ "$(cat test.out)" != "mode: 33226
> > +mode: 33226" ]; then
> > +    echo "$0: unexpected output:"
> > +    cat test.out
> > +    exit 1
> > +fi
> > +
> > +$VG ./guestfish > test.out <<EOF
> > +scratch 50MB
> > +run
>
> Is it possible to combine these tests into a single guestfish run?
> The reason is that under virtualization (especially in Koji),
> appliance start up can be slow.

Sure, done.

> > diff --git a/generator/actions.ml b/generator/actions.ml
> > index 9fa7acb..676a3ea 100644
> > --- a/generator/actions.ml
> > +++ b/generator/actions.ml
> > @@ -11647,6 +11647,28 @@ This function is used internally when
> > setting up the appliance." };>
> >  This function is used internally when closing the appliance.  Note
> >  it's only called when ./configure --enable-valgrind-daemon is
> >  used." };
> >
> > +  { defaults with
> > +    name = "copy_attributes";
> > +    style = RErr, [Pathname "src"; Pathname
"dest"], [OBool
> > "permissions"; OBool "xattributes"];
> > +    proc_nr = Some 415;
> > +    shortdesc = "copy the attributes of a path (file/directory)
to
> > another"; +    longdesc = "\
> > +Copy the attributes of a path (which can be a file or a directory)
> > +to another path.
> > +
> > +By default B<no> attribute is copied; you have to specify which
> > attributes
> > +to copy enabling the optional arguments:
> Thoughts:
>
> - Should it default to copying attributes?

You've convinced me, so I've turned the "permissions" flag
into
"skipmode", so specifying nothing now copies the file mode (so
permissions + bits).

> The common use case (for virt-edit) is: "I want this other file which
> is identical to this original file, except in name and content".  That
> is (I think) an argument for copying everything by default.

I've added a "all" argument which would enable every change,
overriding
even mode:false.

> I would have thought owner uid and group gid should be copied.

Oh true, forgot about them; added a new "ownership" argument.

> - Is there anything else which is useful to copy?

Maybe there's also copying atime/mtime too; worth having a single
argument ("time") for both, or two separate?

> In general terms it all looks fine to me.

Attached there is a second version of the patch.

> You probably want a follow-on patch which changes guestfish edit /
> virt-edit / etc (any place we copy attributes) to use the new call.

Yes, changing those will come after the new API is settled and
committed.

--
Pino Toscano