Re: [Libguestfs] RFC: copy-attributes command

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!

> + 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; > + }

> + /* 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.

> 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.

> 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?

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 would have thought owner uid and group gid should be copied.

- Is there anything else which is useful to copy?

In general terms it all looks fine to me.

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