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

[Wanlong Gao]

On 02/14/2012 04:56 PM, Richard W.M. Jones wrote:

> 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.


Yeah, let Kzak answer these questions. :-)

Thanks
-Wanlong Gao

> 
>> 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.
>