[Libguestfs] RFC: copy-attributes command
Pino Toscano
ptoscano at redhat.com
Fri Jan 10 13:17:48 UTC 2014
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: copy-attributes-2.diff
Type: text/x-patch
Size: 9238 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20140110/2f6bee84/attachment.bin>
More information about the Libguestfs
mailing list