Libguestfs - Jan 2014 - [PATCH] builder, edit, fish: use copy-attributes

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.

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

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#)

On Tue, Jan 07, 2014 at 09:04:36PM +0000, Richard W.M. Jones
wrote:
> - 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.
Also there's no reason we can't have another bool optarg
to select whether the other bool optargs should default to
true or false ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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:
> > 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.
At least in the gnu/stubs-$bits.h of RHEL 5 they are not declared as 
__stub_*, so they should be implemented (and being Linux, it means that 
most probably there are the right syscalls for them).
So I would not think the change proposed below (and implemented) would 
cause regressions in such old Linux systems.
> 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).
Sounds reasonable; attached a patch for it.

-- 
Pino Toscano

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

This allows one to copy attributes (like permissions, xattrs,
ownership) from a file to another.
---
 daemon/daemon.h         |   3 +
 daemon/file.c           |  72 ++++++++++++++++++++++
 daemon/xattr.c          |  69 +++++++++++++++++++++
 fish/Makefile.am        |   1 +
 fish/test-file-attrs.sh | 157 ++++++++++++++++++++++++++++++++++++++++++++++++
 generator/actions.ml    |  38 ++++++++++++
 src/MAX_PROC_NR         |   2 +-
 7 files changed, 341 insertions(+), 1 deletion(-)
 create mode 100755 fish/test-file-attrs.sh

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..856ab5f 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,74 @@ do_filesize (const char *path)
 
   return buf.st_size;
 }
+
+int
+do_copy_attributes (const char *src, const char *dest, int all, int mode, int
xattributes, int ownership)
+{
+  int r;
+  struct stat srcstat, deststat;
+
+  static const unsigned int file_mask = 07777;
+
+  /* If it was specified to copy everything, manually enable all the flags
+   * not manually specified to avoid checking for flag || all everytime.
+   */
+  if (all) {
+    if (!(optargs_bitmask & GUESTFS_COPY_ATTRIBUTES_MODE_BITMASK))
+      mode = 1;
+    if (!(optargs_bitmask & GUESTFS_COPY_ATTRIBUTES_XATTRIBUTES_BITMASK))
+      xattributes = 1;
+    if (!(optargs_bitmask & GUESTFS_COPY_ATTRIBUTES_OWNERSHIP_BITMASK))
+      ownership = 1;
+  }
+
+  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 (mode &&
+      ((srcstat.st_mode & file_mask) != (deststat.st_mode &
file_mask))) {
+    CHROOT_IN;
+    r = chmod (dest, (srcstat.st_mode & file_mask));
+    CHROOT_OUT;
+
+    if (r == -1) {
+      reply_with_perror ("chmod: %s", dest);
+      return -1;
+    }
+  }
+
+  if (ownership &&
+      (srcstat.st_uid != deststat.st_uid || srcstat.st_gid != deststat.st_gid))
{
+    CHROOT_IN;
+    r = chown (dest, srcstat.st_uid, srcstat.st_gid);
+    CHROOT_OUT;
+
+    if (r == -1) {
+      reply_with_perror ("chown: %s", dest);
+      return -1;
+    }
+  }
+
+  if (xattributes && optgroup_linuxxattrs_available ()) {
+    if (!copy_xattrs (src, dest))
+      /* copy_xattrs replies with an error already. */
+      return -1;
+  }
+
+  return 0;
+}
diff --git a/daemon/xattr.c b/daemon/xattr.c
index ebacc02..abed5ff 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -541,8 +541,77 @@ 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)
+{
+  ssize_t len, vlen, ret, attrval_len = 0;
+  CLEANUP_FREE char *buf = NULL, *attrval = NULL;
+  size_t i;
+
+  buf = _listxattrs (src, listxattr, &len);
+  if (buf == NULL)
+    /* _listxattrs issues reply_with_perror already. */
+    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 (ret == -1) {
+      reply_with_perror ("setxattr: %s, %s", dest, &buf[i]);
+      goto error;
+    }
+  }
+
+  return 1;
+
+ error:
+  return 0;
+}
+
 #else /* no HAVE_LINUX_XATTRS */
 
 OPTGROUP_LINUXXATTRS_NOT_AVAILABLE
 
+int
+copy_xattrs (const char *src, const char *dest)
+{
+  abort ();
+}
+
 #endif /* no HAVE_LINUX_XATTRS */
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..78bd817
--- /dev/null
+++ b/fish/test-file-attrs.sh
@@ -0,0 +1,157 @@
+#!/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 mode:true
+stat /bar | grep mode:
+
+echo -----
+
+stat /foo | grep uid:
+stat /foo | grep gid:
+chown 10 11 /foo
+stat /foo | grep uid:
+stat /foo | grep gid:
+stat /bar | grep uid:
+stat /bar | grep gid:
+copy-attributes /foo /bar ownership:true
+stat /bar | grep uid:
+stat /bar | grep gid:
+
+echo -----
+
+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"
+
+echo -----
+
+touch /new
+chmod 0111 /new
+copy-attributes /foo /new all:true mode:false
+stat /new | grep mode:
+stat /new | grep uid:
+stat /new | grep gid:
+lxattrlist / new
+copy-attributes /foo /new mode:true
+stat /new | grep mode:
+EOF
+
+if [ "$(cat test.out)" != "mode: 33226
+mode: 33226
+-----
+uid: 0
+gid: 0
+uid: 10
+gid: 11
+uid: 0
+gid: 0
+uid: 10
+gid: 11
+-----
+[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
+}
+-----
+mode: 32841
+uid: 10
+gid: 11
+[0] = {
+  attrname: 
+  attrval: 2\x00
+}
+[1] = {
+  attrname: user.test
+  attrval: foo
+}
+[2] = {
+  attrname: user.test2
+  attrval: secondtest
+}
+mode: 33226" ]; 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..b4b746f 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -11647,6 +11647,44 @@ 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
"all"; OBool "mode"; OBool "xattributes"; OBool
"ownership"];
+    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 C<no> attribute is copied, so make sure to specify any
+(or C<all> to copy everything).
+
+The optional arguments specify which attributes can be copied:
+
+=over 4
+
+=item C<mode>
+
+Copy part of the file mode from C<source> to C<destination>. Only
the
+UNIX permissions and the sticky/setuid/setgid bits can be copied.
+
+=item C<xattributes>
+
+Copy the Linux extended attributes (xattrs) from C<source> to
C<destination>.
+This flag does nothing if the I<linuxxattrs> feature is not available
+(see C<guestfs_feature_available>).
+
+=item C<ownership>
+
+Copy the owner uid and the group gid of C<source> to
C<destination>.
+
+=item C<all>
+
+Copy B<all> the attributes from C<source> to C<destination>.
Enabling it
+enables all the other flags, if they are not specified already.
+
+=back" };
+
 ]
 
 (* 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
-- 
1.8.3.1

On Mon, Jan 13, 2014 at 02:45:08PM +0100, Pino Toscano
wrote:
> This allows one to copy attributes (like permissions, xattrs,
> ownership) from a file to another.
> ---
>  daemon/daemon.h         |   3 +
>  daemon/file.c           |  72 ++++++++++++++++++++++
>  daemon/xattr.c          |  69 +++++++++++++++++++++
>  fish/Makefile.am        |   1 +
>  fish/test-file-attrs.sh | 157
++++++++++++++++++++++++++++++++++++++++++++++++
>  generator/actions.ml    |  38 ++++++++++++
>  src/MAX_PROC_NR         |   2 +-
>  7 files changed, 341 insertions(+), 1 deletion(-)
>  create mode 100755 fish/test-file-attrs.sh
> 
> 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..856ab5f 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,74 @@ do_filesize (const char *path)
>  
>    return buf.st_size;
>  }
> +
> +int
> +do_copy_attributes (const char *src, const char *dest, int all, int mode,
int xattributes, int ownership)
> +{
> +  int r;
> +  struct stat srcstat, deststat;
> +
> +  static const unsigned int file_mask = 07777;
> +
> +  /* If it was specified to copy everything, manually enable all the flags
> +   * not manually specified to avoid checking for flag || all everytime.
> +   */
> +  if (all) {
> +    if (!(optargs_bitmask & GUESTFS_COPY_ATTRIBUTES_MODE_BITMASK))
> +      mode = 1;
> +    if (!(optargs_bitmask &
GUESTFS_COPY_ATTRIBUTES_XATTRIBUTES_BITMASK))
> +      xattributes = 1;
> +    if (!(optargs_bitmask &
GUESTFS_COPY_ATTRIBUTES_OWNERSHIP_BITMASK))
> +      ownership = 1;
> +  }
> +
> +  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 (mode &&
> +      ((srcstat.st_mode & file_mask) != (deststat.st_mode &
file_mask))) {
> +    CHROOT_IN;
> +    r = chmod (dest, (srcstat.st_mode & file_mask));
> +    CHROOT_OUT;
> +
> +    if (r == -1) {
> +      reply_with_perror ("chmod: %s", dest);
> +      return -1;
> +    }
> +  }
> +
> +  if (ownership &&
> +      (srcstat.st_uid != deststat.st_uid || srcstat.st_gid !=
deststat.st_gid)) {
> +    CHROOT_IN;
> +    r = chown (dest, srcstat.st_uid, srcstat.st_gid);
> +    CHROOT_OUT;
> +
> +    if (r == -1) {
> +      reply_with_perror ("chown: %s", dest);
> +      return -1;
> +    }
> +  }
> +
> +  if (xattributes && optgroup_linuxxattrs_available ()) {
> +    if (!copy_xattrs (src, dest))
> +      /* copy_xattrs replies with an error already. */
> +      return -1;
> +  }
> +
> +  return 0;
> +}
> diff --git a/daemon/xattr.c b/daemon/xattr.c
> index ebacc02..abed5ff 100644
> --- a/daemon/xattr.c
> +++ b/daemon/xattr.c
> @@ -541,8 +541,77 @@ 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)
> +{
> +  ssize_t len, vlen, ret, attrval_len = 0;
> +  CLEANUP_FREE char *buf = NULL, *attrval = NULL;
> +  size_t i;
> +
> +  buf = _listxattrs (src, listxattr, &len);
> +  if (buf == NULL)
> +    /* _listxattrs issues reply_with_perror already. */
> +    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 (ret == -1) {
> +      reply_with_perror ("setxattr: %s, %s", dest, &buf[i]);
> +      goto error;
> +    }
> +  }
> +
> +  return 1;
> +
> + error:
> +  return 0;
> +}
> +
>  #else /* no HAVE_LINUX_XATTRS */
>  
>  OPTGROUP_LINUXXATTRS_NOT_AVAILABLE
>  
> +int
> +copy_xattrs (const char *src, const char *dest)
> +{
> +  abort ();
> +}
> +
>  #endif /* no HAVE_LINUX_XATTRS */
> 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..78bd817
> --- /dev/null
> +++ b/fish/test-file-attrs.sh
> @@ -0,0 +1,157 @@
> +#!/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 mode:true
> +stat /bar | grep mode:
> +
> +echo -----
> +
> +stat /foo | grep uid:
> +stat /foo | grep gid:
> +chown 10 11 /foo
> +stat /foo | grep uid:
> +stat /foo | grep gid:
> +stat /bar | grep uid:
> +stat /bar | grep gid:
> +copy-attributes /foo /bar ownership:true
> +stat /bar | grep uid:
> +stat /bar | grep gid:
> +
> +echo -----
> +
> +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"
> +
> +echo -----
> +
> +touch /new
> +chmod 0111 /new
> +copy-attributes /foo /new all:true mode:false
> +stat /new | grep mode:
> +stat /new | grep uid:
> +stat /new | grep gid:
> +lxattrlist / new
> +copy-attributes /foo /new mode:true
> +stat /new | grep mode:
> +EOF
> +
> +if [ "$(cat test.out)" != "mode: 33226
> +mode: 33226
> +-----
> +uid: 0
> +gid: 0
> +uid: 10
> +gid: 11
> +uid: 0
> +gid: 0
> +uid: 10
> +gid: 11
> +-----
> +[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
> +}
> +-----
> +mode: 32841
> +uid: 10
> +gid: 11
> +[0] = {
> +  attrname: 
> +  attrval: 2\x00
> +}
> +[1] = {
> +  attrname: user.test
> +  attrval: foo
> +}
> +[2] = {
> +  attrname: user.test2
> +  attrval: secondtest
> +}
> +mode: 33226" ]; 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..b4b746f 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -11647,6 +11647,44 @@ 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 "all"; OBool "mode"; OBool "xattributes";
OBool "ownership"];
> +    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 C<no> attribute is copied, so make sure to specify any
> +(or C<all> to copy everything).
> +
> +The optional arguments specify which attributes can be copied:
> +
> +=over 4
> +
> +=item C<mode>
> +
> +Copy part of the file mode from C<source> to C<destination>.
Only the
> +UNIX permissions and the sticky/setuid/setgid bits can be copied.
> +
> +=item C<xattributes>
> +
> +Copy the Linux extended attributes (xattrs) from C<source> to
C<destination>.
> +This flag does nothing if the I<linuxxattrs> feature is not
available
> +(see C<guestfs_feature_available>).
> +
> +=item C<ownership>
> +
> +Copy the owner uid and the group gid of C<source> to
C<destination>.
> +
> +=item C<all>
> +
> +Copy B<all> the attributes from C<source> to
C<destination>. Enabling it
> +enables all the other flags, if they are not specified already.
> +
> +=back" };
> +
>  ]
>  
>  (* 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
ACK.

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#)

Make use of the new copy-attributes command to properly copy all file
attributes from a file to the new version of it.
---
 builder/perl_edit.ml | 29 +---------------------------
 edit/edit.c          | 49 ++---------------------------------------------
 fish/edit.c          | 54 ++--------------------------------------------------
 3 files changed, 5 insertions(+), 127 deletions(-)

diff --git a/builder/perl_edit.ml b/builder/perl_edit.ml
index aa4c2e6..28e5dea 100644
--- a/builder/perl_edit.ml
+++ b/builder/perl_edit.ml
@@ -42,7 +42,7 @@ let rec edit_file ~debug (g : Guestfs.guestfs) file expr   
g#upload tmpfile file;
 
   (* However like virt-edit we do need to copy attributes. *)
-  copy_attributes g file_old file;
+  g#copy_attributes ~all:true file_old file;
   g#rm file_old
 
 and do_perl_edit ~debug g file expr @@ -76,30 +76,3 @@ and do_perl_edit ~debug
g file expr    );
 
   Unix.rename (file ^ ".out") file
-
-and copy_attributes g src dest -  let has_linuxxattrs = g#feature_available
[|"linuxxattrs"|] in
-
-  (* Get the mode. *)
-  let stat = g#stat src in
-
-  (* Get the SELinux context.  XXX Should we copy over other extended
-   * attributes too?
-   *)
-  let selinux_context -    if has_linuxxattrs then (
-      try Some (g#getxattr src "security.selinux") with _ -> None
-    ) else None in
-
-  (* Set the permissions (inc. sticky and set*id bits), UID, GID. *)
-  let mode = Int64.to_int stat.G.mode
-  and uid = Int64.to_int stat.G.uid and gid = Int64.to_int stat.G.gid in
-  g#chmod (mode land 0o7777) dest;
-  g#chown uid gid dest;
-
-  (* Set the SELinux context. *)
-  match selinux_context with
-  | None -> ()
-  | Some selinux_context ->
-    g#setxattr "security.selinux"
-      selinux_context (String.length selinux_context) dest
diff --git a/edit/edit.c b/edit/edit.c
index e5e3a29..07790be 100644
--- a/edit/edit.c
+++ b/edit/edit.c
@@ -56,7 +56,6 @@ static void edit_files (int argc, char *argv[]);
 static void edit (const char *filename, const char *root);
 static char *edit_interactively (const char *tmpfile);
 static char *edit_non_interactively (const char *tmpfile);
-static int copy_attributes (const char *src, const char *dest);
 static int is_windows (guestfs_h *g, const char *root);
 static char *windows_path (guestfs_h *g, const char *root, const char
*filename);
 static char *generate_random_name (const char *filename);
@@ -361,7 +360,8 @@ edit (const char *filename, const char *root)
     /* Set the permissions, UID, GID and SELinux context of the new
      * file to match the old file (RHBZ#788641).
      */
-    if (copy_attributes (filename, newname) == -1)
+    if (guestfs_copy_attributes (g, filename, newname,
+        GUESTFS_COPY_ATTRIBUTES_ALL, 1, -1) == -1)
       goto error;
 
     /* Backup or overwrite the file. */
@@ -510,51 +510,6 @@ edit_non_interactively (const char *tmpfile)
 }
 
 static int
-copy_attributes (const char *src, const char *dest)
-{
-  CLEANUP_FREE_STAT struct guestfs_stat *stat = NULL;
-  const char *linuxxattrs[] = { "linuxxattrs", NULL };
-  int has_linuxxattrs;
-  CLEANUP_FREE char *selinux_context = NULL;
-  size_t selinux_context_size;
-
-  has_linuxxattrs = guestfs_feature_available (g, (char **) linuxxattrs);
-
-  /* Get the mode. */
-  stat = guestfs_stat (g, src);
-  if (stat == NULL)
-    return -1;
-
-  /* Get the SELinux context.  XXX Should we copy over other extended
-   * attributes too?
-   */
-  if (has_linuxxattrs) {
-    guestfs_push_error_handler (g, NULL, NULL);
-
-    selinux_context = guestfs_getxattr (g, src, "security.selinux",
-                                        &selinux_context_size);
-    /* selinux_context could be NULL.  This isn't an error. */
-
-    guestfs_pop_error_handler (g);
-  }
-
-  /* Set the permissions (inc. sticky and set*id bits), UID, GID. */
-  if (guestfs_chmod (g, stat->mode & 07777, dest) == -1)
-    return -1;
-  if (guestfs_chown (g, stat->uid, stat->gid, dest) == -1)
-    return -1;
-
-  /* Set the SELinux context. */
-  if (has_linuxxattrs && selinux_context) {
-    if (guestfs_setxattr (g, "security.selinux", selinux_context,
-                          (int) selinux_context_size, dest) == -1)
-      return -1;
-  }
-
-  return 0;
-}
-
-static int
 is_windows (guestfs_h *g, const char *root)
 {
   int w;
diff --git a/fish/edit.c b/fish/edit.c
index 754a34a..bd02f4b 100644
--- a/fish/edit.c
+++ b/fish/edit.c
@@ -32,7 +32,6 @@
 #include "fish.h"
 
 static char *generate_random_name (const char *filename);
-static int copy_attributes (const char *src, const char *dest);
 
 /* guestfish edit command, suggested by Ján Ondrej, implemented by RWMJ */
 
@@ -135,7 +134,8 @@ run_edit (const char *cmd, size_t argc, char *argv[])
   /* Set the permissions, UID, GID and SELinux context of the new
    * file to match the old file (RHBZ#788641).
    */
-  if (copy_attributes (remotefilename, newname) == -1)
+  if (guestfs_copy_attributes (g, remotefilename, newname,
+      GUESTFS_COPY_ATTRIBUTES_ALL, 1, -1) == -1)
     return -1;
 
   if (guestfs_mv (g, newname, remotefilename) == -1)
@@ -177,53 +177,3 @@ generate_random_name (const char *filename)
 
   return ret; /* caller will free */
 }
-
-static int
-copy_attributes (const char *src, const char *dest)
-{
-  struct guestfs_stat *stat;
-  const char *linuxxattrs[] = { "linuxxattrs", NULL };
-  int has_linuxxattrs;
-  CLEANUP_FREE char *selinux_context = NULL;
-  size_t selinux_context_size;
-
-  has_linuxxattrs = guestfs_feature_available (g, (char **) linuxxattrs);
-
-  /* Get the mode. */
-  stat = guestfs_stat (g, src);
-  if (stat == NULL)
-    return -1;
-
-  /* Get the SELinux context.  XXX Should we copy over other extended
-   * attributes too?
-   */
-  if (has_linuxxattrs) {
-    guestfs_push_error_handler (g, NULL, NULL);
-
-    selinux_context = guestfs_getxattr (g, src, "security.selinux",
-                                        &selinux_context_size);
-    /* selinux_context could be NULL.  This isn't an error. */
-
-    guestfs_pop_error_handler (g);
-  }
-
-  /* Set the permissions (inc. sticky and set*id bits), UID, GID. */
-  if (guestfs_chmod (g, stat->mode & 07777, dest) == -1) {
-    guestfs_free_stat (stat);
-    return -1;
-  }
-  if (guestfs_chown (g, stat->uid, stat->gid, dest) == -1) {
-    guestfs_free_stat (stat);
-    return -1;
-  }
-  guestfs_free_stat (stat);
-
-  /* Set the SELinux context. */
-  if (has_linuxxattrs && selinux_context) {
-    if (guestfs_setxattr (g, "security.selinux", selinux_context,
-                          (int) selinux_context_size, dest) == -1)
-      return -1;
-  }
-
-  return 0;
-}
-- 
1.8.3.1

On Tue, Jan 14, 2014 at 11:16:40AM +0100, Pino Toscano
wrote:
> Make use of the new copy-attributes command to properly copy all file
> attributes from a file to the new version of it.
> ---
>  builder/perl_edit.ml | 29 +---------------------------
>  edit/edit.c          | 49 ++---------------------------------------------
>  fish/edit.c          | 54
++--------------------------------------------------
>  3 files changed, 5 insertions(+), 127 deletions(-)
> 
> diff --git a/builder/perl_edit.ml b/builder/perl_edit.ml
> index aa4c2e6..28e5dea 100644
> --- a/builder/perl_edit.ml
> +++ b/builder/perl_edit.ml
> @@ -42,7 +42,7 @@ let rec edit_file ~debug (g : Guestfs.guestfs) file expr
>    g#upload tmpfile file;
>  
>    (* However like virt-edit we do need to copy attributes. *)
> -  copy_attributes g file_old file;
> +  g#copy_attributes ~all:true file_old file;
>    g#rm file_old
>  
>  and do_perl_edit ~debug g file expr > @@ -76,30 +76,3 @@ and
do_perl_edit ~debug g file expr >    );
>  
>    Unix.rename (file ^ ".out") file
> -
> -and copy_attributes g src dest > -  let has_linuxxattrs =
g#feature_available [|"linuxxattrs"|] in
> -
> -  (* Get the mode. *)
> -  let stat = g#stat src in
> -
> -  (* Get the SELinux context.  XXX Should we copy over other extended
> -   * attributes too?
> -   *)
> -  let selinux_context > -    if has_linuxxattrs then (
> -      try Some (g#getxattr src "security.selinux") with _ ->
None
> -    ) else None in
> -
> -  (* Set the permissions (inc. sticky and set*id bits), UID, GID. *)
> -  let mode = Int64.to_int stat.G.mode
> -  and uid = Int64.to_int stat.G.uid and gid = Int64.to_int stat.G.gid in
> -  g#chmod (mode land 0o7777) dest;
> -  g#chown uid gid dest;
> -
> -  (* Set the SELinux context. *)
> -  match selinux_context with
> -  | None -> ()
> -  | Some selinux_context ->
> -    g#setxattr "security.selinux"
> -      selinux_context (String.length selinux_context) dest
> diff --git a/edit/edit.c b/edit/edit.c
> index e5e3a29..07790be 100644
> --- a/edit/edit.c
> +++ b/edit/edit.c
> @@ -56,7 +56,6 @@ static void edit_files (int argc, char *argv[]);
>  static void edit (const char *filename, const char *root);
>  static char *edit_interactively (const char *tmpfile);
>  static char *edit_non_interactively (const char *tmpfile);
> -static int copy_attributes (const char *src, const char *dest);
>  static int is_windows (guestfs_h *g, const char *root);
>  static char *windows_path (guestfs_h *g, const char *root, const char
*filename);
>  static char *generate_random_name (const char *filename);
> @@ -361,7 +360,8 @@ edit (const char *filename, const char *root)
>      /* Set the permissions, UID, GID and SELinux context of the new
>       * file to match the old file (RHBZ#788641).
>       */
> -    if (copy_attributes (filename, newname) == -1)
> +    if (guestfs_copy_attributes (g, filename, newname,
> +        GUESTFS_COPY_ATTRIBUTES_ALL, 1, -1) == -1)
>        goto error;
>  
>      /* Backup or overwrite the file. */
> @@ -510,51 +510,6 @@ edit_non_interactively (const char *tmpfile)
>  }
>  
>  static int
> -copy_attributes (const char *src, const char *dest)
> -{
> -  CLEANUP_FREE_STAT struct guestfs_stat *stat = NULL;
> -  const char *linuxxattrs[] = { "linuxxattrs", NULL };
> -  int has_linuxxattrs;
> -  CLEANUP_FREE char *selinux_context = NULL;
> -  size_t selinux_context_size;
> -
> -  has_linuxxattrs = guestfs_feature_available (g, (char **) linuxxattrs);
> -
> -  /* Get the mode. */
> -  stat = guestfs_stat (g, src);
> -  if (stat == NULL)
> -    return -1;
> -
> -  /* Get the SELinux context.  XXX Should we copy over other extended
> -   * attributes too?
> -   */
> -  if (has_linuxxattrs) {
> -    guestfs_push_error_handler (g, NULL, NULL);
> -
> -    selinux_context = guestfs_getxattr (g, src,
"security.selinux",
> -                                        &selinux_context_size);
> -    /* selinux_context could be NULL.  This isn't an error. */
> -
> -    guestfs_pop_error_handler (g);
> -  }
> -
> -  /* Set the permissions (inc. sticky and set*id bits), UID, GID. */
> -  if (guestfs_chmod (g, stat->mode & 07777, dest) == -1)
> -    return -1;
> -  if (guestfs_chown (g, stat->uid, stat->gid, dest) == -1)
> -    return -1;
> -
> -  /* Set the SELinux context. */
> -  if (has_linuxxattrs && selinux_context) {
> -    if (guestfs_setxattr (g, "security.selinux",
selinux_context,
> -                          (int) selinux_context_size, dest) == -1)
> -      return -1;
> -  }
> -
> -  return 0;
> -}
> -
> -static int
>  is_windows (guestfs_h *g, const char *root)
>  {
>    int w;
> diff --git a/fish/edit.c b/fish/edit.c
> index 754a34a..bd02f4b 100644
> --- a/fish/edit.c
> +++ b/fish/edit.c
> @@ -32,7 +32,6 @@
>  #include "fish.h"
>  
>  static char *generate_random_name (const char *filename);
> -static int copy_attributes (const char *src, const char *dest);
>  
>  /* guestfish edit command, suggested by Ján Ondrej, implemented by RWMJ */
>  
> @@ -135,7 +134,8 @@ run_edit (const char *cmd, size_t argc, char *argv[])
>    /* Set the permissions, UID, GID and SELinux context of the new
>     * file to match the old file (RHBZ#788641).
>     */
> -  if (copy_attributes (remotefilename, newname) == -1)
> +  if (guestfs_copy_attributes (g, remotefilename, newname,
> +      GUESTFS_COPY_ATTRIBUTES_ALL, 1, -1) == -1)
>      return -1;
>  
>    if (guestfs_mv (g, newname, remotefilename) == -1)
> @@ -177,53 +177,3 @@ generate_random_name (const char *filename)
>  
>    return ret; /* caller will free */
>  }
> -
> -static int
> -copy_attributes (const char *src, const char *dest)
> -{
> -  struct guestfs_stat *stat;
> -  const char *linuxxattrs[] = { "linuxxattrs", NULL };
> -  int has_linuxxattrs;
> -  CLEANUP_FREE char *selinux_context = NULL;
> -  size_t selinux_context_size;
> -
> -  has_linuxxattrs = guestfs_feature_available (g, (char **) linuxxattrs);
> -
> -  /* Get the mode. */
> -  stat = guestfs_stat (g, src);
> -  if (stat == NULL)
> -    return -1;
> -
> -  /* Get the SELinux context.  XXX Should we copy over other extended
> -   * attributes too?
> -   */
> -  if (has_linuxxattrs) {
> -    guestfs_push_error_handler (g, NULL, NULL);
> -
> -    selinux_context = guestfs_getxattr (g, src,
"security.selinux",
> -                                        &selinux_context_size);
> -    /* selinux_context could be NULL.  This isn't an error. */
> -
> -    guestfs_pop_error_handler (g);
> -  }
> -
> -  /* Set the permissions (inc. sticky and set*id bits), UID, GID. */
> -  if (guestfs_chmod (g, stat->mode & 07777, dest) == -1) {
> -    guestfs_free_stat (stat);
> -    return -1;
> -  }
> -  if (guestfs_chown (g, stat->uid, stat->gid, dest) == -1) {
> -    guestfs_free_stat (stat);
> -    return -1;
> -  }
> -  guestfs_free_stat (stat);
> -
> -  /* Set the SELinux context. */
> -  if (has_linuxxattrs && selinux_context) {
> -    if (guestfs_setxattr (g, "security.selinux",
selinux_context,
> -                          (int) selinux_context_size, dest) == -1)
> -      return -1;
> -  }
> -
> -  return 0;
> -}
> -- 
> 1.8.3.1
Looks good, ACK.

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#)