Libguestfs - Sep 2016 - [PATCH] libvirt: read disk paths from pools (RHBZ#1366049)

A disk of type 'volume' is stored as
  <source pool='pool_name' volume='volume_name'/>
and its real location is inside the 'volume_name', as
'pool_name': in
this case, query libvirt for the actual path of the specified volume in
the specified pool.

Adjust the code so that:
- for_each_disk gets the virConnectPtr, needed to do operations with
  libvirt
- when extracting the disk filename depending on the type, the code
  snippet doing it can directly set 'filename', without setting an XPath
  result variable

Only file-based volumes are supported for now; more types can be added
(with proper testing) later on.
---
 src/libvirt-domain.c | 131 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 117 insertions(+), 14 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index d54814f..50759e3 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -40,8 +40,9 @@
 #if defined(HAVE_LIBVIRT)
 
 static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom);
-static ssize_t for_each_disk (guestfs_h *g, xmlDocPtr doc, int (*f) (guestfs_h
*g, const char *filename, const char *format, int readonly, const char
*protocol, char *const *server, const char *username, void *data), void *data);
+static ssize_t for_each_disk (guestfs_h *g, virConnectPtr conn, xmlDocPtr doc,
int (*f) (guestfs_h *g, const char *filename, const char *format, int readonly,
const char *protocol, char *const *server, const char *username, void *data),
void *data);
 static int libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, char
**label_rtn, char **imagelabel_rtn);
+static char *filename_from_pool (guestfs_h *g, virConnectPtr conn, const char
*pool_nane, const char *volume_name);
 
 static void
 ignore_errors (void *ignore, virErrorPtr ignore2)
@@ -311,7 +312,7 @@ guestfs_impl_add_libvirt_dom (guestfs_h *g, void *domvp,
    * all disks are added or none are added.
    */
   ckp = guestfs_int_checkpoint_drives (g);
-  r = for_each_disk (g, doc, add_disk, &data);
+  r = for_each_disk (g, virDomainGetConnect (dom), doc, add_disk, &data);
   if (r == -1)
     guestfs_int_rollback_drives (g, ckp);
 
@@ -466,6 +467,7 @@ libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc,
  */
 static ssize_t
 for_each_disk (guestfs_h *g,
+               virConnectPtr conn,
                xmlDocPtr doc,
                int (*f) (guestfs_h *g,
                          const char *filename, const char *format,
@@ -509,6 +511,8 @@ for_each_disk (guestfs_h *g,
       CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpprotocol = NULL;
       CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xphost = NULL;
       CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpusername = NULL;
+      CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xppool = NULL;
+      CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpvolume = NULL;
       xmlAttrPtr attr;
       int readonly;
       int t;
@@ -628,22 +632,65 @@ for_each_disk (guestfs_h *g,
          * TODO: secrets: ./auth/secret/@type,
          * ./auth/secret/@usage || ./auth/secret/@uuid
          */
+      } else if (STREQ (type, "volume")) { /* type =
"volume", use source/@volume */
+        CLEANUP_FREE char *pool = NULL;
+        CLEANUP_FREE char *volume = NULL;
+
+        xpathCtx->node = nodes->nodeTab[i];
+
+        /* Get the source pool.  Required. */
+        xppool = xmlXPathEvalExpression (BAD_CAST "./source/@pool",
+                                         xpathCtx);
+        if (xppool == NULL ||
+            xppool->nodesetval == NULL ||
+            xppool->nodesetval->nodeNr == 0)
+          continue;
+        assert (xppool->nodesetval->nodeTab[0]);
+        assert (xppool->nodesetval->nodeTab[0]->type =+               
XML_ATTRIBUTE_NODE);
+        attr = (xmlAttrPtr) xppool->nodesetval->nodeTab[0];
+        pool = (char *) xmlNodeListGetString (doc, attr->children, 1);
+
+        /* Get the source volume.  Required. */
+        xpvolume = xmlXPathEvalExpression (BAD_CAST
"./source/@volume",
+                                           xpathCtx);
+        if (xpvolume == NULL ||
+            xpvolume->nodesetval == NULL ||
+            xpvolume->nodesetval->nodeNr == 0)
+          continue;
+        assert (xpvolume->nodesetval->nodeTab[0]);
+        assert (xpvolume->nodesetval->nodeTab[0]->type =+             
XML_ATTRIBUTE_NODE);
+        attr = (xmlAttrPtr) xpvolume->nodesetval->nodeTab[0];
+        volume = (char *) xmlNodeListGetString (doc, attr->children, 1);
+
+        debug (g, "disk[%zu]: pool: %s; volume: %s", i, pool,
volume);
+
+        filename = filename_from_pool (g, conn, pool, volume);
+        if (filename == NULL)
+          continue; /* filename_from_pool already called error() */
       } else
         continue; /* type <> "file", "block", or
"network", skip it */
 
-      assert (xpfilename);
-      assert (xpfilename->nodesetval);
-      if (xpfilename->nodesetval->nodeNr > 0) {
-        assert (xpfilename->nodesetval->nodeTab[0]);
-        assert (xpfilename->nodesetval->nodeTab[0]->type =-           
XML_ATTRIBUTE_NODE);
-        attr = (xmlAttrPtr) xpfilename->nodesetval->nodeTab[0];
-        filename = (char *) xmlNodeListGetString (doc, attr->children, 1);
-        debug (g, "disk[%zu]: filename: %s", i, filename);
+      /* Allow a disk type to directly get the filename, with no need
+       * for an XPath evaluation.
+       */
+      if (filename == NULL) {
+        assert (xpfilename);
+        assert (xpfilename->nodesetval);
+        if (xpfilename->nodesetval->nodeNr > 0) {
+          assert (xpfilename->nodesetval->nodeTab[0]);
+          assert (xpfilename->nodesetval->nodeTab[0]->type =+         
XML_ATTRIBUTE_NODE);
+          attr = (xmlAttrPtr) xpfilename->nodesetval->nodeTab[0];
+          filename = (char *) xmlNodeListGetString (doc, attr->children, 1);
+        }
+        else
+          /* For network protocols (eg. nbd), name may be omitted. */
+          filename = safe_strdup (g, "");
       }
-      else
-        /* For network protocols (eg. nbd), name may be omitted. */
-        filename = safe_strdup (g, "");
+
+      debug (g, "disk[%zu]: filename: %s", i, filename);
 
       /* Get the disk format (may not be set). */
       xpathCtx->node = nodes->nodeTab[i];
@@ -784,6 +831,62 @@ get_domain_xml (guestfs_h *g, virDomainPtr dom)
   return doc;
 }
 
+static char *
+filename_from_pool (guestfs_h *g, virConnectPtr conn,
+                    const char *pool_name, const char *volume_name)
+{
+  char *filename = NULL;
+  virErrorPtr err;
+  virStoragePoolPtr pool = NULL;
+  virStorageVolPtr vol = NULL;
+  virStorageVolInfo info;
+  int ret;
+
+  pool = virStoragePoolLookupByName (conn, pool_name);
+  if (pool == NULL) {
+    err = virGetLastError ();
+    error (g, _("no libvirt pool called '%s': %s"),
+           pool_name, err->message);
+    goto cleanup;
+  }
+
+  vol = virStorageVolLookupByName (pool, volume_name);
+  if (vol == NULL) {
+    err = virGetLastError ();
+    error (g, _("no volume called '%s' in the libvirt pool
'%s': %s"),
+           volume_name, pool_name, err->message);
+    goto cleanup;
+  }
+
+  ret = virStorageVolGetInfo (vol, &info);
+  if (ret < 0) {
+    err = virGetLastError ();
+    error (g, _("cannot get information of the libvirt volume
'%s': %s"),
+           volume_name, err->message);
+    goto cleanup;
+  }
+
+  debug (g, "type of libvirt volume %s: %d", volume_name, info.type);
+
+  /* Support only file-based volumes for now. */
+  if (info.type != VIR_STORAGE_VOL_FILE)
+    goto cleanup;
+
+  filename = virStorageVolGetPath (vol);
+  if (filename == NULL) {
+    err = virGetLastError ();
+    error (g, _("cannot get the filename of the libvirt volume
'%s': %s"),
+           volume_name, err->message);
+    goto cleanup;
+  }
+
+ cleanup:
+  if (vol) virStorageVolFree (vol);
+  if (pool) virStoragePoolFree (pool);
+
+  return filename;
+}
+
 #else /* no libvirt at compile time */
 
 #define NOT_IMPL(r)                                                     \
-- 
2.7.4

On Tue, Sep 20, 2016 at 05:07:23PM +0200, Pino Toscano
wrote:
> A disk of type 'volume' is stored as
>   <source pool='pool_name' volume='volume_name'/>
> and its real location is inside the 'volume_name', as
'pool_name': in
> this case, query libvirt for the actual path of the specified volume in
> the specified pool.
> 
> Adjust the code so that:
> - for_each_disk gets the virConnectPtr, needed to do operations with
>   libvirt
> - when extracting the disk filename depending on the type, the code
>   snippet doing it can directly set 'filename', without setting an
XPath
>   result variable
> 
> Only file-based volumes are supported for now; more types can be added
> (with proper testing) later on.
> ---
>  src/libvirt-domain.c | 131
+++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 117 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index d54814f..50759e3 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -40,8 +40,9 @@
>  #if defined(HAVE_LIBVIRT)
>  
>  static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom);
> -static ssize_t for_each_disk (guestfs_h *g, xmlDocPtr doc, int (*f)
(guestfs_h *g, const char *filename, const char *format, int readonly, const
char *protocol, char *const *server, const char *username, void *data), void
*data);
> +static ssize_t for_each_disk (guestfs_h *g, virConnectPtr conn, xmlDocPtr
doc, int (*f) (guestfs_h *g, const char *filename, const char *format, int
readonly, const char *protocol, char *const *server, const char *username, void
*data), void *data);
>  static int libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, char
**label_rtn, char **imagelabel_rtn);
> +static char *filename_from_pool (guestfs_h *g, virConnectPtr conn, const
char *pool_nane, const char *volume_name);
>  
>  static void
>  ignore_errors (void *ignore, virErrorPtr ignore2)
> @@ -311,7 +312,7 @@ guestfs_impl_add_libvirt_dom (guestfs_h *g, void
*domvp,
>     * all disks are added or none are added.
>     */
>    ckp = guestfs_int_checkpoint_drives (g);
> -  r = for_each_disk (g, doc, add_disk, &data);
> +  r = for_each_disk (g, virDomainGetConnect (dom), doc, add_disk,
&data);
>    if (r == -1)
>      guestfs_int_rollback_drives (g, ckp);
>  
> @@ -466,6 +467,7 @@ libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc,
>   */
>  static ssize_t
>  for_each_disk (guestfs_h *g,
> +               virConnectPtr conn,
>                 xmlDocPtr doc,
>                 int (*f) (guestfs_h *g,
>                           const char *filename, const char *format,
> @@ -509,6 +511,8 @@ for_each_disk (guestfs_h *g,
>        CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpprotocol = NULL;
>        CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xphost = NULL;
>        CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpusername = NULL;
> +      CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xppool = NULL;
> +      CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpvolume = NULL;
>        xmlAttrPtr attr;
>        int readonly;
>        int t;
> @@ -628,22 +632,65 @@ for_each_disk (guestfs_h *g,
>           * TODO: secrets: ./auth/secret/@type,
>           * ./auth/secret/@usage || ./auth/secret/@uuid
>           */
> +      } else if (STREQ (type, "volume")) { /* type =
"volume", use source/@volume */
> +        CLEANUP_FREE char *pool = NULL;
> +        CLEANUP_FREE char *volume = NULL;
> +
> +        xpathCtx->node = nodes->nodeTab[i];
> +
> +        /* Get the source pool.  Required. */
> +        xppool = xmlXPathEvalExpression (BAD_CAST
"./source/@pool",
> +                                         xpathCtx);
> +        if (xppool == NULL ||
> +            xppool->nodesetval == NULL ||
> +            xppool->nodesetval->nodeNr == 0)
> +          continue;
> +        assert (xppool->nodesetval->nodeTab[0]);
> +        assert (xppool->nodesetval->nodeTab[0]->type => +     
XML_ATTRIBUTE_NODE);
> +        attr = (xmlAttrPtr) xppool->nodesetval->nodeTab[0];
> +        pool = (char *) xmlNodeListGetString (doc, attr->children, 1);
> +
> +        /* Get the source volume.  Required. */
> +        xpvolume = xmlXPathEvalExpression (BAD_CAST
"./source/@volume",
> +                                           xpathCtx);
> +        if (xpvolume == NULL ||
> +            xpvolume->nodesetval == NULL ||
> +            xpvolume->nodesetval->nodeNr == 0)
> +          continue;
> +        assert (xpvolume->nodesetval->nodeTab[0]);
> +        assert (xpvolume->nodesetval->nodeTab[0]->type => +   
XML_ATTRIBUTE_NODE);
> +        attr = (xmlAttrPtr) xpvolume->nodesetval->nodeTab[0];
> +        volume = (char *) xmlNodeListGetString (doc, attr->children,
1);
> +
> +        debug (g, "disk[%zu]: pool: %s; volume: %s", i, pool,
volume);
> +
> +        filename = filename_from_pool (g, conn, pool, volume);
> +        if (filename == NULL)
> +          continue; /* filename_from_pool already called error() */
>        } else
>          continue; /* type <> "file", "block", or
"network", skip it */
This comment should be amended.  Probably best to say "type is not
handled above, skip it" rather than enumerating every type here.
> -      assert (xpfilename);
> -      assert (xpfilename->nodesetval);
> -      if (xpfilename->nodesetval->nodeNr > 0) {
> -        assert (xpfilename->nodesetval->nodeTab[0]);
> -        assert (xpfilename->nodesetval->nodeTab[0]->type => - 
XML_ATTRIBUTE_NODE);
> -        attr = (xmlAttrPtr) xpfilename->nodesetval->nodeTab[0];
> -        filename = (char *) xmlNodeListGetString (doc, attr->children,
1);
> -        debug (g, "disk[%zu]: filename: %s", i, filename);
> +      /* Allow a disk type to directly get the filename, with no need
> +       * for an XPath evaluation.
> +       */
I didn't understand this hunk until I parsed the comment to mean that
the previous code (for type == "volume") was setting the filename.
Perhaps the comment should include the magic words "in the code
above"?
> +      if (filename == NULL) {
> +        assert (xpfilename);
> +        assert (xpfilename->nodesetval);
> +        if (xpfilename->nodesetval->nodeNr > 0) {
> +          assert (xpfilename->nodesetval->nodeTab[0]);
> +          assert (xpfilename->nodesetval->nodeTab[0]->type =>
+                  XML_ATTRIBUTE_NODE);
> +          attr = (xmlAttrPtr) xpfilename->nodesetval->nodeTab[0];
> +          filename = (char *) xmlNodeListGetString (doc,
attr->children, 1);
> +        }
> +        else
> +          /* For network protocols (eg. nbd), name may be omitted. */
> +          filename = safe_strdup (g, "");
>        }
> -      else
> -        /* For network protocols (eg. nbd), name may be omitted. */
> -        filename = safe_strdup (g, "");
> +
> +      debug (g, "disk[%zu]: filename: %s", i, filename);
>        /* Get the disk format (may not be set). */
>        xpathCtx->node = nodes->nodeTab[i];
> @@ -784,6 +831,62 @@ get_domain_xml (guestfs_h *g, virDomainPtr dom)
>    return doc;
>  }
>  
> +static char *
> +filename_from_pool (guestfs_h *g, virConnectPtr conn,
> +                    const char *pool_name, const char *volume_name)
> +{
> +  char *filename = NULL;
> +  virErrorPtr err;
> +  virStoragePoolPtr pool = NULL;
> +  virStorageVolPtr vol = NULL;
> +  virStorageVolInfo info;
> +  int ret;
> +
> +  pool = virStoragePoolLookupByName (conn, pool_name);
> +  if (pool == NULL) {
> +    err = virGetLastError ();
> +    error (g, _("no libvirt pool called '%s': %s"),
> +           pool_name, err->message);
> +    goto cleanup;
> +  }
> +
> +  vol = virStorageVolLookupByName (pool, volume_name);
> +  if (vol == NULL) {
> +    err = virGetLastError ();
> +    error (g, _("no volume called '%s' in the libvirt pool
'%s': %s"),
> +           volume_name, pool_name, err->message);
> +    goto cleanup;
> +  }
> +
> +  ret = virStorageVolGetInfo (vol, &info);
> +  if (ret < 0) {
> +    err = virGetLastError ();
> +    error (g, _("cannot get information of the libvirt volume
'%s': %s"),
> +           volume_name, err->message);
> +    goto cleanup;
> +  }
> +
> +  debug (g, "type of libvirt volume %s: %d", volume_name,
info.type);
> +
> +  /* Support only file-based volumes for now. */
> +  if (info.type != VIR_STORAGE_VOL_FILE)
> +    goto cleanup;
> +
> +  filename = virStorageVolGetPath (vol);
> +  if (filename == NULL) {
> +    err = virGetLastError ();
> +    error (g, _("cannot get the filename of the libvirt volume
'%s': %s"),
> +           volume_name, err->message);
> +    goto cleanup;
> +  }
> +
> + cleanup:
> +  if (vol) virStorageVolFree (vol);
> +  if (pool) virStoragePoolFree (pool);
> +
> +  return filename;
> +}
> +
>  #else /* no libvirt at compile time */
It looks OK although I didn't test it.

Can we add a test?  There is already a test for the corresponding
virt-v2v functionality (v2v/test-v2v-o-libvirt.sh).  Just make sure
that the pool name is chosen randomly (see commit 2efd8d0b1da).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW