Libguestfs - Jul 2016 - Re: [PATCH] osinfo: revamp db reading (RHBZ#1359652)

More recent versions of libosinfo switched the internal directory with
the XML files of OSes to a different layout (still with the same XML
format), causing libguestfs to not read them anymore.  Furthermore, the
internal directory is going to disappear soon, replaced by a public
osinfo database [1].

Revamp the way libguestfs reads the data: first try the upcoming osinfo
layout, falling back to the current libosinfo layout (which is the same
as osinfo), and then to the old flat layout.

[1] https://gitlab.com/libosinfo/libosinfo/blob/master/docs/database-layout.txt
---
 src/osinfo.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 140 insertions(+), 28 deletions(-)

diff --git a/src/osinfo.c b/src/osinfo.c
index f4e2c71..0caacfa 100644
--- a/src/osinfo.c
+++ b/src/osinfo.c
@@ -55,6 +55,7 @@
 #include <assert.h>
 #include <sys/types.h>
 #include <libintl.h>
+#include <sys/stat.h>
 
 #include <libxml/parser.h>
 #include <libxml/xpath.h>
@@ -148,36 +149,109 @@ guestfs_int_osinfo_map (guestfs_h *g, const struct
guestfs_isoinfo *isoinfo,
  * Note that failure to find or parse the XML files is *not* a fatal
  * error, since we should fall back silently if these are not
  * available.  Although we'll emit some debug if this happens.
+ *
+ * Try to use the shared osinfo database layout (and location) first:
+ * https://gitlab.com/libosinfo/libosinfo/blob/master/docs/database-layout.txt
  */
-#define LIBOSINFO_DB_OS_PATH LIBOSINFO_DB_PATH "/oses"
-
 static int read_osinfo_db_xml (guestfs_h *g, const char *filename);
 
+static int read_osinfo_db_flat (guestfs_h *g, const char *directory);
+static int read_osinfo_db_three_levels (guestfs_h *g, const char *directory);
+static int read_osinfo_db_directory (guestfs_h *g, const char *directory);
+
 static int
 read_osinfo_db (guestfs_h *g)
 {
-  DIR *dir = NULL;
-  struct dirent *d;
   int r;
   size_t i;
+  const char *path;
 
   assert (osinfo_db_size == 0);
 
-  dir = opendir (LIBOSINFO_DB_OS_PATH);
+  /* (1) Try the shared osinfo directory, using either the
+   * $OSINFO_SYSTEM_DIR envvar or its default value.
+   */
+  path = getenv ("OSINFO_SYSTEM_DIR");
+  if (path == NULL)
+    path = "/usr/share/osinfo";
+  r = read_osinfo_db_three_levels (g, path);
+  if (r == -1)
+    goto error;
+  else if (r == 1)
+    return 0;
+
+  /* (2) Try the libosinfo directory, using the newer three-directory
+   * layout ($LIBOSINFO_DB_PATH / "os" / $group-ID / [file.xml]).
+   */
+  r = read_osinfo_db_three_levels (g, LIBOSINFO_DB_PATH "/os");
+  if (r == -1)
+    goto error;
+  else if (r == 1)
+    return 0;
+
+  /* (3) Try the libosinfo directory, using the old flat directory
+   * layout ($LIBOSINFO_DB_PATH / "oses" / [file.xml]).
+   */
+  r = read_osinfo_db_flat (g, LIBOSINFO_DB_PATH "/oses");
+  if (r == -1)
+    goto error;
+  else if (r == 1)
+    return 0;
+
+  /* Nothing found. */
+  return 0;
+
+ error:
+  /* Fatal error: free any database entries which have been read, and
+   * mark the database as having a permanent error.
+   */
+  if (osinfo_db_size > 0) {
+    for (i = 0; i < (size_t) osinfo_db_size; ++i)
+      free_osinfo_db_entry (&osinfo_db[i]);
+  }
+  free (osinfo_db);
+  osinfo_db = NULL;
+  osinfo_db_size = -1;
+
+  return -1;
+}
+
+static int
+read_osinfo_db_flat (guestfs_h *g, const char *directory)
+{
+  debug (g, "osinfo: loading flat database from %s", directory);
+
+  return read_osinfo_db_directory (g, directory);
+}
+
+static int
+read_osinfo_db_three_levels (guestfs_h *g, const char *directory)
+{
+  DIR *dir;
+  int r;
+
+  dir = opendir (directory);
   if (!dir) {
-    debug (g, "osinfo: %s: %s", LIBOSINFO_DB_OS_PATH, strerror
(errno));
+    debug (g, "osinfo: %s: %s", directory, strerror (errno));
     return 0; /* This is not an error: RHBZ#948324. */
   }
 
-  debug (g, "osinfo: loading database from %s",
LIBOSINFO_DB_OS_PATH);
+  debug (g, "osinfo: loading 3-level-directories database from %s",
directory);
 
   for (;;) {
+    struct dirent *d;
+    CLEANUP_FREE char *pathname = NULL;
+    struct stat sb;
+
     errno = 0;
     d = readdir (dir);
     if (!d) break;
 
-    if (STRSUFFIX (d->d_name, ".xml")) {
-      r = read_osinfo_db_xml (g, d->d_name);
+    pathname = safe_asprintf (g, "%s/%s", directory, d->d_name);
+
+    /* Iterate only on directories. */
+    if (stat (pathname, &sb) == 0 && S_ISDIR (sb.st_mode)) {
+      r = read_osinfo_db_directory (g, pathname);
       if (r == -1)
         goto error;
     }
@@ -185,7 +259,7 @@ read_osinfo_db (guestfs_h *g)
 
   /* Check for failure in readdir. */
   if (errno != 0) {
-    perrorf (g, "readdir: %s", LIBOSINFO_DB_OS_PATH);
+    perrorf (g, "readdir: %s", directory);
     goto error;
   }
 
@@ -193,26 +267,67 @@ read_osinfo_db (guestfs_h *g)
   r = closedir (dir);
   dir = NULL;
   if (r == -1) {
-    perrorf (g, "closedir: %s", LIBOSINFO_DB_OS_PATH);
+    perrorf (g, "closedir: %s", directory);
     goto error;
   }
 
-  return 0;
+  return 1;
 
  error:
   if (dir)
     closedir (dir);
 
-  /* Fatal error: free any database entries which have been read, and
-   * mark the database as having a permanent error.
-   */
-  if (osinfo_db_size > 0) {
-    for (i = 0; i < (size_t) osinfo_db_size; ++i)
-      free_osinfo_db_entry (&osinfo_db[i]);
+  return -1;
+}
+
+static int
+read_osinfo_db_directory (guestfs_h *g, const char *directory)
+{
+  DIR *dir;
+  int r;
+
+  dir = opendir (directory);
+  if (!dir) {
+    debug (g, "osinfo: %s: %s", directory, strerror (errno));
+    return 0; /* This is not an error: RHBZ#948324. */
   }
-  free (osinfo_db);
-  osinfo_db = NULL;
-  osinfo_db_size = -1;
+
+  for (;;) {
+    struct dirent *d;
+
+    errno = 0;
+    d = readdir (dir);
+    if (!d) break;
+
+    if (STRSUFFIX (d->d_name, ".xml")) {
+      CLEANUP_FREE char *pathname = NULL;
+
+      pathname = safe_asprintf (g, "%s/%s", directory, d->d_name);
+      r = read_osinfo_db_xml (g, pathname);
+      if (r == -1)
+        goto error;
+    }
+  }
+
+  /* Check for failure in readdir. */
+  if (errno != 0) {
+    perrorf (g, "readdir: %s", directory);
+    goto error;
+  }
+
+  /* Close the directory handle. */
+  r = closedir (dir);
+  dir = NULL;
+  if (r == -1) {
+    perrorf (g, "closedir: %s", directory);
+    goto error;
+  }
+
+  return 1;
+
+ error:
+  if (dir)
+    closedir (dir);
 
   return -1;
 }
@@ -221,13 +336,12 @@ static int read_iso_node (guestfs_h *g, xmlNodePtr
iso_node, struct osinfo *osin
 static int read_media_node (guestfs_h *g, xmlXPathContextPtr xpathCtx,
xmlNodePtr media_node, struct osinfo *osinfo);
 static int read_os_node (guestfs_h *g, xmlXPathContextPtr xpathCtx, xmlNodePtr
os_node, struct osinfo *osinfo);
 
-/* Read a single XML file from LIBOSINFO_DB_OS_PATH/filename.  Only
- * memory allocation failures are fatal errors here.
+/* Read a single XML file from pathname (which is a full path).
+ * Only memory allocation failures are fatal errors here.
  */
 static int
-read_osinfo_db_xml (guestfs_h *g, const char *filename)
+read_osinfo_db_xml (guestfs_h *g, const char *pathname)
 {
-  CLEANUP_FREE char *pathname = NULL;
   CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
   CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
   CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL;
@@ -236,8 +350,6 @@ read_osinfo_db_xml (guestfs_h *g, const char *filename)
   struct osinfo *osinfo;
   size_t i;
 
-  pathname = safe_asprintf (g, "%s/%s", LIBOSINFO_DB_OS_PATH,
filename);
-
   doc = xmlReadFile (pathname, NULL, XML_PARSE_NONET);
   if (doc == NULL) {
     debug (g, "osinfo: unable to parse XML file %s", pathname);
@@ -298,7 +410,7 @@ read_osinfo_db_xml (guestfs_h *g, const char *filename)
 
 #if 0
       debug (g, "osinfo: %s: %s%s%s%s=> arch %s live %s product %s type
%d distro %d version %d.%d",
-             filename,
+             pathname,
              osinfo->re_system_id ? "<system-id/> " :
"",
              osinfo->re_volume_id ? "<volume-id/> " :
"",
              osinfo->re_publisher_id ? "<publisher-id/> " :
"",
-- 
2.7.4

On Mon, Jul 25, 2016 at 05:47:05PM +0200, Pino Toscano
wrote:
> +  if (path == NULL)
> +    path = "/usr/share/osinfo";
I wonder if we should use $prefix here.  I guess there are
arguments both ways.

Should we just run the external osinfo-query program?

Anyway it's fine as it stands, so ACK.

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 Tuesday, 26 July 2016 11:16:07 CEST Richard W.M. Jones
wrote:
> On Mon, Jul 25, 2016 at 05:47:05PM +0200, Pino Toscano wrote:
> > +  if (path == NULL)
> > +    path = "/usr/share/osinfo";
> 
> I wonder if we should use $prefix here.  I guess there are
> arguments both ways.
The specification defines that directory as fallback.  Also, $prefix
would be libguestfs' own, not osinfo's.
> Should we just run the external osinfo-query program?
Unfortunately, it does not expose all the information we need.
> Anyway it's fine as it stands, so ACK.
Thanks, pushed.
-- 
Pino Toscano