Libguestfs - Feb 2012 - [PATCH RFC] blkid: start using libblkid directly instead

Hi Rich:
What do you think about this idea?
although this still has some problems like do 'vfs-type /dev/vda'.
Can you give some comments about this? Is this a bad idea?

Thanks
-Wanlong Gao

-----------------------------------------------------------------------------
Use libblkid directly instead of the binary command in blkid.

Signed-off-by: Wanlong Gao <gaowanlong at cn.fujitsu.com>
---
 appliance/packagelist.in |    1 +
 configure.ac             |   10 ++++++++
 daemon/Makefile.am       |    1 +
 daemon/blkid.c           |   55 ++++++++++++++++++++++-----------------------
 4 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/appliance/packagelist.in b/appliance/packagelist.in
index 2ab6b80..575b8dd 100644
--- a/appliance/packagelist.in
+++ b/appliance/packagelist.in
@@ -45,6 +45,7 @@
   vim-minimal
   xz
   zfs-fuse
+  libblkid
 #endif /* REDHAT */
 
 #ifdef DEBIAN
diff --git a/configure.ac b/configure.ac
index cc11b2f..0beef28 100644
--- a/configure.ac
+++ b/configure.ac
@@ -393,6 +393,16 @@ if test "x$have_libselinux" = "xyes";
then
         AC_DEFINE([HAVE_LIBSELINUX],[1],[Define to 1 if you have libselinux])
 fi
 AC_SUBST([SELINUX_LIB])
+AC_CHECK_HEADERS([blkid/blkid.h])
+AC_CHECK_LIB([blkid],[blkid_new_probe],[
+        have_libblkid="$ac_cv_header_blkid_blkid_h"
+        LIBBLKID="-lblkid"
+        LIBS="$LIBS $LIBBLKID"
+        ],[have_libblkid=no])
+if test "x$have_libblkid" = "xyes"; then
+        AC_DEFINE([HAVE_LIBBLKID],[1],[Define to 1 if you have libblkid])
+fi
+AC_SUBST([LIBBLKID])
 
 dnl Check for systemtap/DTrace userspace probes (optional).
 dnl http://sourceware.org/systemtap/wiki/AddingUserSpaceProbingToApps
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 3a698cc..5e4db57 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -167,6 +167,7 @@ guestfsd_LDADD = \
 	liberrnostring.a \
 	libprotocol.a \
 	$(SELINUX_LIB) \
+	$(LIBBLKID) \
 	$(AUGEAS_LIBS) \
 	$(top_builddir)/gnulib/lib/.libs/libgnu.a \
 	$(GETADDRINFO_LIB) \
diff --git a/daemon/blkid.c b/daemon/blkid.c
index 8728c29..0c4ca71 100644
--- a/daemon/blkid.c
+++ b/daemon/blkid.c
@@ -23,6 +23,8 @@
 #include <string.h>
 #include <unistd.h>
 #include <limits.h>
+#include <fcntl.h>
+#include <blkid/blkid.h>
 
 #include "daemon.h"
 #include "actions.h"
@@ -30,40 +32,37 @@
 static char *
 get_blkid_tag (const char *device, const char *tag)
 {
-  char *out, *err;
-  int r;
+  int fd, rc;
+  const char *data = NULL;
+  blkid_probe blkprobe;
 
-  r = commandr (&out, &err,
-                "blkid",
-                /* Adding -c option kills all caching, even on RHEL 5. */
-                "-c", "/dev/null",
-                "-o", "value", "-s", tag, device,
NULL);
-  if (r != 0 && r != 2) {
-    if (r >= 0)
-      reply_with_error ("%s: %s (blkid returned %d)", device, err,
r);
-    else
-      reply_with_error ("%s: %s", device, err);
-    free (out);
-    free (err);
+  if (!device || !tag)
     return NULL;
-  }
 
-  free (err);
+  fd = open (device, O_RDONLY);
+  if (fd < 0)
+    return NULL;
 
-  if (r == 2) {                 /* means UUID etc not found */
-    free (out);
-    out = strdup ("");
-    if (out == NULL)
-      reply_with_perror ("strdup");
-    return out;
-  }
+  blkprobe = blkid_new_probe ();
+  if (!blkprobe)
+    goto done;
+  if (blkid_probe_set_device (blkprobe, fd, 0, 0))
+    goto done;
+
+  blkid_probe_enable_superblocks(blkprobe, 1);
+
+  blkid_probe_set_superblocks_flags (blkprobe, BLKID_SUBLKS_LABEL |
+                                     BLKID_SUBLKS_UUID | BLKID_SUBLKS_TYPE);
 
-  /* Trim trailing \n if present. */
-  size_t len = strlen (out);
-  if (len > 0 && out[len-1] == '\n')
-    out[len-1] = '\0';
+  rc = blkid_do_safeprobe (blkprobe);
+  if (!rc)
+    blkid_probe_lookup_value (blkprobe, tag, &data, NULL);
 
-  return out;                   /* caller frees */
+done:
+  close (fd);
+  if (blkprobe)
+    blkid_free_probe (blkprobe);
+  return data ? strdup ((char *) data) : NULL;
 }
 
 char *
-- 
1.7.9

On Tue, Feb 14, 2012 at 03:30:26PM +0800, Wanlong Gao
wrote:
> Hi Rich:
> What do you think about this idea?
> although this still has some problems like do 'vfs-type /dev/vda'.
> Can you give some comments about this? Is this a bad idea?
It's one of those things that might be an improvement, but it would
have to be distinctly better than the existing approach of calling out
to the 'blkid' binary.

Is the libblkid library interface stable?  Is it meant to be used by
programs other than the blkid binary?

I've CC'd this to kzak who will have a much better idea than me.
> Thanks
> -Wanlong Gao
> 
>
-----------------------------------------------------------------------------
> Use libblkid directly instead of the binary command in blkid.
> 
> Signed-off-by: Wanlong Gao <gaowanlong at cn.fujitsu.com>
> ---
>  appliance/packagelist.in |    1 +
>  configure.ac             |   10 ++++++++
>  daemon/Makefile.am       |    1 +
>  daemon/blkid.c           |   55
++++++++++++++++++++++-----------------------
>  4 files changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/appliance/packagelist.in b/appliance/packagelist.in
> index 2ab6b80..575b8dd 100644
> --- a/appliance/packagelist.in
> +++ b/appliance/packagelist.in
> @@ -45,6 +45,7 @@
>    vim-minimal
>    xz
>    zfs-fuse
> +  libblkid
>  #endif /* REDHAT */
>  
>  #ifdef DEBIAN
On Debian/Ubuntu, this library is called libblkid1 so that needs to be
added to the DEBIAN section.
> diff --git a/configure.ac b/configure.ac
> index cc11b2f..0beef28 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -393,6 +393,16 @@ if test "x$have_libselinux" =
"xyes"; then
>          AC_DEFINE([HAVE_LIBSELINUX],[1],[Define to 1 if you have
libselinux])
>  fi
>  AC_SUBST([SELINUX_LIB])
> +AC_CHECK_HEADERS([blkid/blkid.h])
> +AC_CHECK_LIB([blkid],[blkid_new_probe],[
> +        have_libblkid="$ac_cv_header_blkid_blkid_h"
> +        LIBBLKID="-lblkid"
> +        LIBS="$LIBS $LIBBLKID"
> +        ],[have_libblkid=no])
> +if test "x$have_libblkid" = "xyes"; then
> +        AC_DEFINE([HAVE_LIBBLKID],[1],[Define to 1 if you have libblkid])
> +fi
> +AC_SUBST([LIBBLKID])
This is quite convoluted: blkid has a pkg-config file which seems like
it would be easier to use.  See existing examples using PKG_CHECK_MODULES.
>  dnl Check for systemtap/DTrace userspace probes (optional).
>  dnl http://sourceware.org/systemtap/wiki/AddingUserSpaceProbingToApps
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 3a698cc..5e4db57 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -167,6 +167,7 @@ guestfsd_LDADD = \
>  	liberrnostring.a \
>  	libprotocol.a \
>  	$(SELINUX_LIB) \
> +	$(LIBBLKID) \
>  	$(AUGEAS_LIBS) \
>  	$(top_builddir)/gnulib/lib/.libs/libgnu.a \
>  	$(GETADDRINFO_LIB) \
> diff --git a/daemon/blkid.c b/daemon/blkid.c
> index 8728c29..0c4ca71 100644
> --- a/daemon/blkid.c
> +++ b/daemon/blkid.c
> @@ -23,6 +23,8 @@
>  #include <string.h>
>  #include <unistd.h>
>  #include <limits.h>
> +#include <fcntl.h>
> +#include <blkid/blkid.h>
>  
>  #include "daemon.h"
>  #include "actions.h"
> @@ -30,40 +32,37 @@
>  static char *
>  get_blkid_tag (const char *device, const char *tag)
>  {
> -  char *out, *err;
> -  int r;
> +  int fd, rc;
> +  const char *data = NULL;
> +  blkid_probe blkprobe;
>  
> -  r = commandr (&out, &err,
> -                "blkid",
> -                /* Adding -c option kills all caching, even on RHEL 5. */
> -                "-c", "/dev/null",
> -                "-o", "value", "-s", tag,
device, NULL);
> -  if (r != 0 && r != 2) {
> -    if (r >= 0)
> -      reply_with_error ("%s: %s (blkid returned %d)", device,
err, r);
> -    else
> -      reply_with_error ("%s: %s", device, err);
> -    free (out);
> -    free (err);
> +  if (!device || !tag)
>      return NULL;
> -  }
>  
> -  free (err);
> +  fd = open (device, O_RDONLY);
> +  if (fd < 0)
> +    return NULL;
>  
> -  if (r == 2) {                 /* means UUID etc not found */
> -    free (out);
> -    out = strdup ("");
> -    if (out == NULL)
> -      reply_with_perror ("strdup");
> -    return out;
> -  }
> +  blkprobe = blkid_new_probe ();
> +  if (!blkprobe)
> +    goto done;
> +  if (blkid_probe_set_device (blkprobe, fd, 0, 0))
> +    goto done;
> +
> +  blkid_probe_enable_superblocks(blkprobe, 1);
> +
> +  blkid_probe_set_superblocks_flags (blkprobe, BLKID_SUBLKS_LABEL |
> +                                     BLKID_SUBLKS_UUID |
BLKID_SUBLKS_TYPE);
>  
> -  /* Trim trailing \n if present. */
> -  size_t len = strlen (out);
> -  if (len > 0 && out[len-1] == '\n')
> -    out[len-1] = '\0';
> +  rc = blkid_do_safeprobe (blkprobe);
> +  if (!rc)
> +    blkid_probe_lookup_value (blkprobe, tag, &data, NULL);
>  
> -  return out;                   /* caller frees */
> +done:
> +  close (fd);
> +  if (blkprobe)
> +    blkid_free_probe (blkprobe);
> +  return data ? strdup ((char *) data) : NULL;
>  }
>  
>  char *
> -- 
> 1.7.9
Does this avoid the cache like the existing code?

Do all the existing tests pass?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top