Libguestfs - Apr 2014 - [PATCH] daemon: xattr: factorize do_getxattr and do_lgetxattr

Move all the common code to a new _getxattr function, much like done for
other xattrs operations.

Mostly code motion, no functional changes.
---
 daemon/xattr.c | 54 +++++++++++-------------------------------------------
 1 file changed, 11 insertions(+), 43 deletions(-)

diff --git a/daemon/xattr.c b/daemon/xattr.c
index abed5ff..d8ad59a 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -55,6 +55,7 @@ static guestfs_int_xattr_list *getxattrs (const char *path,
ssize_t (*listxattr)
 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);
+static char *_getxattr (const char *name, const char *path, ssize_t (*getxattr)
(const char *path, const char *name, void *value, size_t size), size_t *size_r);
 
 guestfs_int_xattr_list *
 do_getxattrs (const char *path)
@@ -448,6 +449,15 @@ do_internal_lxattrlist (const char *path, char *const
*names)
 char *
 do_getxattr (const char *path, const char *name, size_t *size_r)
 {
+  return _getxattr (name, path, getxattr, size_r);
+}
+
+static char *
+_getxattr (const char *name, const char *path,
+           ssize_t (*getxattr) (const char *path, const char *name,
+                                void *value, size_t size),
+           size_t *size_r)
+{
   ssize_t r;
   char *buf;
   size_t len;
@@ -496,49 +506,7 @@ do_getxattr (const char *path, const char *name, size_t
*size_r)
 char *
 do_lgetxattr (const char *path, const char *name, size_t *size_r)
 {
-  ssize_t r;
-  char *buf;
-  size_t len;
-
-  CHROOT_IN;
-  r = lgetxattr (path, name, NULL, 0);
-  CHROOT_OUT;
-  if (r == -1) {
-    reply_with_perror ("lgetxattr");
-    return NULL;
-  }
-
-  len = r;
-
-  if (len > XATTR_SIZE_MAX) {
-    reply_with_error ("extended attribute is too large");
-    return NULL;
-  }
-
-  buf = malloc (len);
-  if (buf == NULL) {
-    reply_with_perror ("malloc");
-    return NULL;
-  }
-
-  CHROOT_IN;
-  r = lgetxattr (path, name, buf, len);
-  CHROOT_OUT;
-  if (r == -1) {
-    reply_with_perror ("lgetxattr");
-    free (buf);
-    return NULL;
-  }
-
-  if (len != (size_t) r) {
-    reply_with_error ("lgetxattr: unexpected size (%zu/%zd)", len,
r);
-    free (buf);
-    return NULL;
-  }
-
-  /* Must set size_r last thing before returning. */
-  *size_r = len;
-  return buf; /* caller frees */
+  return _getxattr (name, path, lgetxattr, size_r);
 }
 
 int
-- 
1.9.0

On Wed, Apr 30, 2014 at 04:00:20PM +0200, Pino Toscano
wrote:
> Move all the common code to a new _getxattr function, much like done for
> other xattrs operations.
> 
> Mostly code motion, no functional changes.
> ---
>  daemon/xattr.c | 54 +++++++++++-------------------------------------------
>  1 file changed, 11 insertions(+), 43 deletions(-)
> 
> diff --git a/daemon/xattr.c b/daemon/xattr.c
> index abed5ff..d8ad59a 100644
> --- a/daemon/xattr.c
> +++ b/daemon/xattr.c
> @@ -55,6 +55,7 @@ static guestfs_int_xattr_list *getxattrs (const char
*path, ssize_t (*listxattr)
>  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);
> +static char *_getxattr (const char *name, const char *path, ssize_t
(*getxattr) (const char *path, const char *name, void *value, size_t size),
size_t *size_r);
>  
>  guestfs_int_xattr_list *
>  do_getxattrs (const char *path)
> @@ -448,6 +449,15 @@ do_internal_lxattrlist (const char *path, char *const
*names)
>  char *
>  do_getxattr (const char *path, const char *name, size_t *size_r)
>  {
> +  return _getxattr (name, path, getxattr, size_r);
> +}
> +
> +static char *
> +_getxattr (const char *name, const char *path,
> +           ssize_t (*getxattr) (const char *path, const char *name,
> +                                void *value, size_t size),
> +           size_t *size_r)
> +{
>    ssize_t r;
>    char *buf;
>    size_t len;
> @@ -496,49 +506,7 @@ do_getxattr (const char *path, const char *name,
size_t *size_r)
>  char *
>  do_lgetxattr (const char *path, const char *name, size_t *size_r)
>  {
> -  ssize_t r;
> -  char *buf;
> -  size_t len;
> -
> -  CHROOT_IN;
> -  r = lgetxattr (path, name, NULL, 0);
> -  CHROOT_OUT;
> -  if (r == -1) {
> -    reply_with_perror ("lgetxattr");
> -    return NULL;
> -  }
> -
> -  len = r;
> -
> -  if (len > XATTR_SIZE_MAX) {
> -    reply_with_error ("extended attribute is too large");
> -    return NULL;
> -  }
> -
> -  buf = malloc (len);
> -  if (buf == NULL) {
> -    reply_with_perror ("malloc");
> -    return NULL;
> -  }
> -
> -  CHROOT_IN;
> -  r = lgetxattr (path, name, buf, len);
> -  CHROOT_OUT;
> -  if (r == -1) {
> -    reply_with_perror ("lgetxattr");
> -    free (buf);
> -    return NULL;
> -  }
> -
> -  if (len != (size_t) r) {
> -    reply_with_error ("lgetxattr: unexpected size (%zu/%zd)",
len, r);
> -    free (buf);
> -    return NULL;
> -  }
> -
> -  /* Must set size_r last thing before returning. */
> -  *size_r = len;
> -  return buf; /* caller frees */
> +  return _getxattr (name, path, lgetxattr, size_r);
>  }
It's a minor thing, but this changes the error messages, so that they
will say things like "getxattr: <errno string>" instead of
"lgetxattr: ...".

It would be good to pass in the name of the function as a string to
_getxattr so that it can print the name correctly in error messages.

ACK with that change.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wednesday 30 April 2014 17:52:14 Richard W.M. Jones
wrote:
> On Wed, Apr 30, 2014 at 04:00:20PM +0200, Pino Toscano wrote:
> > Move all the common code to a new _getxattr function, much like done
> > for other xattrs operations.
> > 
> > Mostly code motion, no functional changes.
> > ---
> > 
> >  daemon/xattr.c | 54
> >  +++++++++++------------------------------------------- 1 file
> >  changed, 11 insertions(+), 43 deletions(-)
> > 
> > diff --git a/daemon/xattr.c b/daemon/xattr.c
> > index abed5ff..d8ad59a 100644
> > --- a/daemon/xattr.c
> > +++ b/daemon/xattr.c
> > @@ -55,6 +55,7 @@ static guestfs_int_xattr_list *getxattrs (const
> > char *path, ssize_t (*listxattr)> 
> >  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);> 
> > +static char *_getxattr (const char *name, const char *path, ssize_t
> > (*getxattr) (const char *path, const char *name, void *value,
> > size_t size), size_t *size_r);> 
> >  guestfs_int_xattr_list *
> >  do_getxattrs (const char *path)
> > 
> > @@ -448,6 +449,15 @@ do_internal_lxattrlist (const char *path, char
> > *const *names)> 
> >  char *
> >  do_getxattr (const char *path, const char *name, size_t *size_r)
> >  {
> > 
> > +  return _getxattr (name, path, getxattr, size_r);
> > +}
> > +
> > +static char *
> > +_getxattr (const char *name, const char *path,
> > +           ssize_t (*getxattr) (const char *path, const char *name,
> > +                                void *value, size_t size),
> > +           size_t *size_r)
> > +{
> > 
> >    ssize_t r;
> >    char *buf;
> >    size_t len;
> > 
> > @@ -496,49 +506,7 @@ do_getxattr (const char *path, const char
> > *name, size_t *size_r)> 
> >  char *
> >  do_lgetxattr (const char *path, const char *name, size_t *size_r)
> >  {
> > 
> > -  ssize_t r;
> > -  char *buf;
> > -  size_t len;
> > -
> > -  CHROOT_IN;
> > -  r = lgetxattr (path, name, NULL, 0);
> > -  CHROOT_OUT;
> > -  if (r == -1) {
> > -    reply_with_perror ("lgetxattr");
> > -    return NULL;
> > -  }
> > -
> > -  len = r;
> > -
> > -  if (len > XATTR_SIZE_MAX) {
> > -    reply_with_error ("extended attribute is too large");
> > -    return NULL;
> > -  }
> > -
> > -  buf = malloc (len);
> > -  if (buf == NULL) {
> > -    reply_with_perror ("malloc");
> > -    return NULL;
> > -  }
> > -
> > -  CHROOT_IN;
> > -  r = lgetxattr (path, name, buf, len);
> > -  CHROOT_OUT;
> > -  if (r == -1) {
> > -    reply_with_perror ("lgetxattr");
> > -    free (buf);
> > -    return NULL;
> > -  }
> > -
> > -  if (len != (size_t) r) {
> > -    reply_with_error ("lgetxattr: unexpected size
(%zu/%zd)", len,
> > r); -    free (buf);
> > -    return NULL;
> > -  }
> > -
> > -  /* Must set size_r last thing before returning. */
> > -  *size_r = len;
> > -  return buf; /* caller frees */
> > +  return _getxattr (name, path, lgetxattr, size_r);
> > 
> >  }
> 
> It's a minor thing, but this changes the error messages, so that they
> will say things like "getxattr: <errno string>" instead of
"lgetxattr:
> ...".
> 
> It would be good to pass in the name of the function as a string to
> _getxattr so that it can print the name correctly in error messages.
Other common functions in xattr.c have the same minor issue.
> ACK with that change.
If that's OK for you, I can push this patch as it is, and fix the 
function names in error strings for the whole xattr.c at once.

-- 
Pino Toscano