Libguestfs - Jan 2014 - [PATCH] daemon: xattr: move the listxattrs code in an own function

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

On Fri, Jan 10, 2014 at 02:17:48PM +0100, Pino Toscano
wrote:
> > 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.
On the basis that most users will be creating a new file (which will
have no xattrs except in some very odd corner cases), just leave your
implementation for now, but don't specify it in the documentation so
we could change it later.
> > - 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.
The API is now pretty confusing.  Each OBool flag is really a
tristate.  It can either be "true", "false" or "not
specified".

Therefore I think it should be:

  xattributes:true            # copies only xattrs and nothing else
  all:true                    # copies everything
  all:true xattributes:false  # copies everything except xattrs

In other words, 'all' changes the default (ie. "not
specified") state
of the other flags.

To be clearer, the four OBool parameters would be:

  [OBool "all"; OBool "mode"; OBool "ownership";
OBool "xattributes"]
> > 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?
For virt-edit, I would _not_ want it to copy the old time(s).  The
time would be updated.

But I guess there's a use for copying the time(s), so this could be
another flag.

We're allowed to extend the API later by adding optional flags (see
"Note about extending functions" in generator/README for the
complicated rules about extending APIs while preserving binary
compatibility).
> +=over 4
> +
> +=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<all>
^ Typo in this line.
> +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>.
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 Fri, Jan 10, 2014 at 01:33:38PM +0000, Richard W.M. Jones
wrote:
> The API is now pretty confusing.  Each OBool flag is really a
> tristate.  It can either be "true", "false" or
"not specified".
> 
> Therefore I think it should be:
> 
>   xattributes:true            # copies only xattrs and nothing else
>   all:true                    # copies everything
>   all:true xattributes:false  # copies everything except xattrs
> 
> In other words, 'all' changes the default (ie. "not
specified") state
> of the other flags.
> 
> To be clearer, the four OBool parameters would be:
> 
>   [OBool "all"; OBool "mode"; OBool
"ownership"; OBool "xattributes"]
So looking at your v2 patch a bit more, I think this is what you
already did, except that "skipmode" is backwards from the other flags.
I think we're in agreement, except I think "skipmode" should just
become "mode" and not have negated meaning compared to the other
flags.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Move in an own function the code that does the (l)listxattrs allocating
the buffer of the right legth, as it will be useful later.

No functional changes, just code motion.
---
 daemon/xattr.c | 64 ++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/daemon/xattr.c b/daemon/xattr.c
index b84cf3d..e01e9e2 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -54,6 +54,7 @@ optgroup_linuxxattrs_available (void)
 static guestfs_int_xattr_list *getxattrs (const char *path, ssize_t
(*listxattr) (const char *path, char *list, size_t size), ssize_t (*getxattr)
(const char *path, const char *name, void *value, size_t size));
 static int _setxattr (const char *xattr, const char *val, int vallen, const
char *path, int (*setxattr) (const char *path, const char *name, const void
*value, size_t size, int flags));
 static int _removexattr (const char *xattr, const char *path, int
(*removexattr) (const char *path, const char *name));
+static char *_listxattrs (const char *path, ssize_t (*listxattr) (const char
*path, char *list, size_t size), ssize_t *size);
 
 guestfs_int_xattr_list *
 do_getxattrs (const char *path)
@@ -111,27 +112,10 @@ getxattrs (const char *path,
   size_t i, j;
   guestfs_int_xattr_list *r = NULL;
 
-  CHROOT_IN;
-  len = listxattr (path, NULL, 0);
-  CHROOT_OUT;
-  if (len == -1) {
-    reply_with_perror ("listxattr: %s", path);
+  buf = _listxattrs (path, listxattr, &len);
+  if (buf == NULL)
+    /* _listxattrs issues reply_with_perror already. */
     goto error;
-  }
-
-  buf = malloc (len);
-  if (buf == NULL) {
-    reply_with_perror ("malloc");
-    goto error;
-  }
-
-  CHROOT_IN;
-  len = listxattr (path, buf, len);
-  CHROOT_OUT;
-  if (len == -1) {
-    reply_with_perror ("listxattr: %s", path);
-    goto error;
-  }
 
   r = calloc (1, sizeof (*r));
   if (r == NULL) {
@@ -252,6 +236,46 @@ _removexattr (const char *xattr, const char *path,
   return 0;
 }
 
+static char *
+_listxattrs (const char *path,
+             ssize_t (*listxattr) (const char *path, char *list, size_t size),
+             ssize_t *size)
+{
+  int r;
+  char *buf = NULL;
+  ssize_t len;
+
+  CHROOT_IN;
+  len = listxattr (path, NULL, 0);
+  CHROOT_OUT;
+  if (len == -1) {
+    reply_with_perror ("listxattr: %s", path);
+    goto error;
+  }
+
+  buf = malloc (len);
+  if (buf == NULL) {
+    reply_with_perror ("malloc");
+    goto error;
+  }
+
+  CHROOT_IN;
+  len = listxattr (path, buf, len);
+  CHROOT_OUT;
+  if (len == -1) {
+    reply_with_perror ("listxattr: %s", path);
+    goto error;
+  }
+
+  if (size)
+    *size = len;
+  return buf;
+
+ error:
+  free (buf);
+  return NULL;
+}
+
 guestfs_int_xattr_list *
 do_internal_lxattrlist (const char *path, char *const *names)
 {
-- 
1.8.3.1

On Fri, Jan 10, 2014 at 03:15:04PM +0100, Pino Toscano
wrote:
> Move in an own function the code that does the (l)listxattrs allocating
> the buffer of the right legth, as it will be useful later.
> 
> No functional changes, just code motion.
> ---
>  daemon/xattr.c | 64
++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/daemon/xattr.c b/daemon/xattr.c
> index b84cf3d..e01e9e2 100644
> --- a/daemon/xattr.c
> +++ b/daemon/xattr.c
> @@ -54,6 +54,7 @@ optgroup_linuxxattrs_available (void)
>  static guestfs_int_xattr_list *getxattrs (const char *path, ssize_t
(*listxattr) (const char *path, char *list, size_t size), ssize_t (*getxattr)
(const char *path, const char *name, void *value, size_t size));
>  static int _setxattr (const char *xattr, const char *val, int vallen,
const char *path, int (*setxattr) (const char *path, const char *name, const
void *value, size_t size, int flags));
>  static int _removexattr (const char *xattr, const char *path, int
(*removexattr) (const char *path, const char *name));
> +static char *_listxattrs (const char *path, ssize_t (*listxattr) (const
char *path, char *list, size_t size), ssize_t *size);
>  
>  guestfs_int_xattr_list *
>  do_getxattrs (const char *path)
> @@ -111,27 +112,10 @@ getxattrs (const char *path,
>    size_t i, j;
>    guestfs_int_xattr_list *r = NULL;
>  
> -  CHROOT_IN;
> -  len = listxattr (path, NULL, 0);
> -  CHROOT_OUT;
> -  if (len == -1) {
> -    reply_with_perror ("listxattr: %s", path);
> +  buf = _listxattrs (path, listxattr, &len);
> +  if (buf == NULL)
> +    /* _listxattrs issues reply_with_perror already. */
>      goto error;
> -  }
> -
> -  buf = malloc (len);
> -  if (buf == NULL) {
> -    reply_with_perror ("malloc");
> -    goto error;
> -  }
> -
> -  CHROOT_IN;
> -  len = listxattr (path, buf, len);
> -  CHROOT_OUT;
> -  if (len == -1) {
> -    reply_with_perror ("listxattr: %s", path);
> -    goto error;
> -  }
>  
>    r = calloc (1, sizeof (*r));
>    if (r == NULL) {
> @@ -252,6 +236,46 @@ _removexattr (const char *xattr, const char *path,
>    return 0;
>  }
>  
> +static char *
> +_listxattrs (const char *path,
> +             ssize_t (*listxattr) (const char *path, char *list, size_t
size),
> +             ssize_t *size)
> +{
> +  int r;
> +  char *buf = NULL;
> +  ssize_t len;
> +
> +  CHROOT_IN;
> +  len = listxattr (path, NULL, 0);
> +  CHROOT_OUT;
> +  if (len == -1) {
> +    reply_with_perror ("listxattr: %s", path);
> +    goto error;
> +  }
> +
> +  buf = malloc (len);
> +  if (buf == NULL) {
> +    reply_with_perror ("malloc");
> +    goto error;
> +  }
> +
> +  CHROOT_IN;
> +  len = listxattr (path, buf, len);
> +  CHROOT_OUT;
> +  if (len == -1) {
> +    reply_with_perror ("listxattr: %s", path);
> +    goto error;
> +  }
> +
> +  if (size)
> +    *size = len;
> +  return buf;
> +
> + error:
> +  free (buf);
> +  return NULL;
> +}
> +
>  guestfs_int_xattr_list *
>  do_internal_lxattrlist (const char *path, char *const *names)
>  {
> -- 
> 1.8.3.1
Looks like just code motion, so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org