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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs