[Libguestfs] RFC: copy-attributes command
Richard W.M. Jones
rjones at redhat.com
Tue Jan 7 21:04:36 UTC 2014
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?
>
> --
> Pino Toscano
> 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!
> + CHROOT_OUT;
> +
> + if (r == -1) {
> + reply_with_perror ("chmod: %s", dest);
> + return -1;
> + }
> + }
> +
> + if (xattributes && optgroup_linuxxattrs_available ()) {
> + if (!copy_xattrs (src, dest))
> + return -1;
> + }
> +
> + return 0;
> +}
> diff --git a/daemon/xattr.c b/daemon/xattr.c
> index af8bfd4..97a94d5 100644
> --- a/daemon/xattr.c
> +++ b/daemon/xattr.c
> @@ -545,8 +545,98 @@ do_lgetxattr (const char *path, const char *name, size_t *size_r)
> return buf; /* caller frees */
> }
>
> +int
> +copy_xattrs (const char *src, const char *dest)
> +{
> +#if defined(HAVE_LISTXATTR) && defined(HAVE_GETXATTR) && defined(HAVE_SETXATTR)
I wonder if there are any platforms that lack one of listxattr,
getxattr and setxattr, but at the same time have one of these calls.
The xattr code (in general) is incredibly complex because of all these
tests.
I guess Mac OS X probably has none of them, and RHEL 5 / Ubuntu 10.10
probably had only some of them. We don't care about RHEL 5 since it
now has its own branch ("oldlinux"), so the code might be made simpler
by an accompanying patch which reduces all of the HAVE_*XATTR* macros
down to a single one (HAVE_LINUX_XATTRS).
> + 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.
> + return 1;
> +
> + error:
> + return 0;
> +#else
> + return 0;
> +#endif
> +}
> +
> #else /* no xattr.h */
>
> OPTGROUP_LINUXXATTRS_NOT_AVAILABLE
>
> +int
> +copy_xattrs (const char *src, const char *dest)
> +{
> + abort ();
> +}
> +
> #endif /* no xattr.h */
> diff --git a/fish/Makefile.am b/fish/Makefile.am
> index bd0485f..fb0e99e 100644
> --- a/fish/Makefile.am
> +++ b/fish/Makefile.am
> @@ -279,6 +279,7 @@ if ENABLE_APPLIANCE
> TESTS += \
> test-copy.sh \
> test-edit.sh \
> + test-file-attrs.sh \
> test-find0.sh \
> test-inspect.sh \
> test-glob.sh \
> 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.
> +part-disk /dev/sda mbr
> +mkfs ext2 /dev/sda1
> +mount /dev/sda1 /
> +
> +touch /foo
> +touch /bar
> +setxattr user.test foo 3 /foo
> +setxattr user.test2 secondtest 10 /foo
> +setxattr user.foo another 7 /bar
> +lxattrlist / "foo bar"
> +copy-attributes /foo /bar xattributes:true
> +lxattrlist / "foo bar"
> +EOF
> +
> +if [ "$(cat test.out)" != "[0] = {
> + attrname:
> + attrval: 2\x00
> +}
> +[1] = {
> + attrname: user.test
> + attrval: foo
> +}
> +[2] = {
> + attrname: user.test2
> + attrval: secondtest
> +}
> +[3] = {
> + attrname:
> + attrval: 1\x00
> +}
> +[4] = {
> + attrname: user.foo
> + attrval: another
> +}
> +[0] = {
> + attrname:
> + attrval: 2\x00
> +}
> +[1] = {
> + attrname: user.test
> + attrval: foo
> +}
> +[2] = {
> + attrname: user.test2
> + attrval: secondtest
> +}
> +[3] = {
> + attrname:
> + attrval: 3\x00
> +}
> +[4] = {
> + attrname: user.foo
> + attrval: another
> +}
> +[5] = {
> + attrname: user.test
> + attrval: foo
> +}
> +[6] = {
> + attrname: user.test2
> + attrval: secondtest
> +}" ]; then
> + echo "$0: unexpected output:"
> + cat test.out
> + exit 1
> +fi
> +
> +rm test.out
> 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.
- Is there anything else which is useful to copy?
I would have thought owner uid and group gid should be copied.
Also sticky/setuid/setgid bits as mentioned above.
> +=over 4
> +
> +=item C<permissions>
> +
> +Copy the UNIX permissions from C<source> to C<destination>.
> +
> +=item C<xattributes>
> +
> +Copy the extended attributes from C<source> to C<destination>." };
> +
> ]
>
> (* Non-API meta-commands available only in guestfish.
> diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
> index d1b9f6a..21c8d99 100644
> --- a/src/MAX_PROC_NR
> +++ b/src/MAX_PROC_NR
> @@ -1 +1 @@
> -414
> +415
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.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
More information about the Libguestfs
mailing list