Pino Toscano
2016-Sep-22 15:40 UTC
[Libguestfs] [PATCH v2] 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 | 134 +++++++++++++++++++++++++---- tests/disks/test-qemu-drive-libvirt.sh | 16 ++++ tests/disks/test-qemu-drive-libvirt.xml.in | 37 ++++++++ 3 files changed, 172 insertions(+), 15 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index d54814f..4d4142d 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,66 @@ for_each_disk (guestfs_h *g, * TODO: secrets: ./auth/secret/@type, * ./auth/secret/@usage || ./auth/secret/@uuid */ - } else - continue; /* type <> "file", "block", or "network", skip it */ + } 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]; - assert (xpfilename); - assert (xpfilename->nodesetval); - if (xpfilename->nodesetval->nodeNr > 0) { - assert (xpfilename->nodesetval->nodeTab[0]); - assert (xpfilename->nodesetval->nodeTab[0]->type =+ /* 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) xpfilename->nodesetval->nodeTab[0]; - filename = (char *) xmlNodeListGetString (doc, attr->children, 1); - debug (g, "disk[%zu]: filename: %s", i, filename); + 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 is not handled above, skip it */ + + /* Allow any of the code blocks above (handling a disk type) + * to directly get the filename (setting '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 +832,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) \ diff --git a/tests/disks/test-qemu-drive-libvirt.sh b/tests/disks/test-qemu-drive-libvirt.sh index 215a99e..b2656ba 100755 --- a/tests/disks/test-qemu-drive-libvirt.sh +++ b/tests/disks/test-qemu-drive-libvirt.sh @@ -47,6 +47,12 @@ export LIBGUESTFS_BACKEND=direct export LIBGUESTFS_HV="${abs_srcdir}/debug-qemu.sh" export DEBUG_QEMU_FILE="${abs_builddir}/test-qemu-drive-libvirt.out" +# Setup the fake pool. +pool_dir=tmp +rm -rf "$pool_dir" +mkdir "$pool_dir" +touch "$pool_dir/in-pool" + function check_output () { if [ ! -f "$DEBUG_QEMU_FILE" ]; then @@ -104,8 +110,18 @@ check_output grep -sq -- '-drive file=sheepdog:volume,' "$DEBUG_QEMU_FILE" || fail rm "$DEBUG_QEMU_FILE" +# Local, stored in a pool. + +$guestfish -d pool1 run ||: +check_output +grep -sq -- "-drive file=$abs_builddir/tmp/in-pool" "$DEBUG_QEMU_FILE" || fail +rm "$DEBUG_QEMU_FILE" + # To do: # HTTP - curl not yet supported by libvirt # SSH. + +# Clean up. +rm -r "$pool_dir" diff --git a/tests/disks/test-qemu-drive-libvirt.xml.in b/tests/disks/test-qemu-drive-libvirt.xml.in index e8e6252..f0b7fe0 100644 --- a/tests/disks/test-qemu-drive-libvirt.xml.in +++ b/tests/disks/test-qemu-drive-libvirt.xml.in @@ -132,4 +132,41 @@ </devices> </domain> + <domain type='test' xmlns:test='http://libvirt.org/schemas/domain/test/1.0'> + <test:runstate>5</test:runstate> <!-- 5 == VIR_DOMAIN_SHUTOFF --> + <name>pool1</name> + <memory>1048576</memory> + <os> + <type>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <disk type='volume' device='disk'> + <driver name='qemu'/> + <source pool='pool1' volume='in-pool'/> + <target dev='vda' bus='virtio'/> + </disk> + </devices> + </domain> + + <pool type='dir'> + <name>pool1</name> + <uuid>12345678-1234-1234-1234-1234567890ab</uuid> + <target> + <path>@abs_builddir@/tmp</path> + </target> + + <volume type='file'> + <name>in-pool</name> + <capacity unit='bytes'>1048576</capacity> + <key>@abs_builddir@/tmp/in-pool</key> + <source> + </source> + <target> + <path>@abs_builddir@/tmp/in-pool</path> + </target> + </volume> + + </pool> + </node> -- 2.7.4
Richard W.M. Jones
2016-Sep-22 15:44 UTC
Re: [Libguestfs] [PATCH v2] libvirt: read disk paths from pools (RHBZ#1366049)
On Thu, Sep 22, 2016 at 05:40:25PM +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.All looks good now, ACK. Thanks, Rich.> src/libvirt-domain.c | 134 +++++++++++++++++++++++++---- > tests/disks/test-qemu-drive-libvirt.sh | 16 ++++ > tests/disks/test-qemu-drive-libvirt.xml.in | 37 ++++++++ > 3 files changed, 172 insertions(+), 15 deletions(-) > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > index d54814f..4d4142d 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,66 @@ for_each_disk (guestfs_h *g, > * TODO: secrets: ./auth/secret/@type, > * ./auth/secret/@usage || ./auth/secret/@uuid > */ > - } else > - continue; /* type <> "file", "block", or "network", skip it */ > + } 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]; > > - assert (xpfilename); > - assert (xpfilename->nodesetval); > - if (xpfilename->nodesetval->nodeNr > 0) { > - assert (xpfilename->nodesetval->nodeTab[0]); > - assert (xpfilename->nodesetval->nodeTab[0]->type => + /* 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) xpfilename->nodesetval->nodeTab[0]; > - filename = (char *) xmlNodeListGetString (doc, attr->children, 1); > - debug (g, "disk[%zu]: filename: %s", i, filename); > + 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 is not handled above, skip it */ > + > + /* Allow any of the code blocks above (handling a disk type) > + * to directly get the filename (setting '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 +832,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) \ > diff --git a/tests/disks/test-qemu-drive-libvirt.sh b/tests/disks/test-qemu-drive-libvirt.sh > index 215a99e..b2656ba 100755 > --- a/tests/disks/test-qemu-drive-libvirt.sh > +++ b/tests/disks/test-qemu-drive-libvirt.sh > @@ -47,6 +47,12 @@ export LIBGUESTFS_BACKEND=direct > export LIBGUESTFS_HV="${abs_srcdir}/debug-qemu.sh" > export DEBUG_QEMU_FILE="${abs_builddir}/test-qemu-drive-libvirt.out" > > +# Setup the fake pool. > +pool_dir=tmp > +rm -rf "$pool_dir" > +mkdir "$pool_dir" > +touch "$pool_dir/in-pool" > + > function check_output () > { > if [ ! -f "$DEBUG_QEMU_FILE" ]; then > @@ -104,8 +110,18 @@ check_output > grep -sq -- '-drive file=sheepdog:volume,' "$DEBUG_QEMU_FILE" || fail > rm "$DEBUG_QEMU_FILE" > > +# Local, stored in a pool. > + > +$guestfish -d pool1 run ||: > +check_output > +grep -sq -- "-drive file=$abs_builddir/tmp/in-pool" "$DEBUG_QEMU_FILE" || fail > +rm "$DEBUG_QEMU_FILE" > + > # To do: > > # HTTP - curl not yet supported by libvirt > > # SSH. > + > +# Clean up. > +rm -r "$pool_dir" > diff --git a/tests/disks/test-qemu-drive-libvirt.xml.in b/tests/disks/test-qemu-drive-libvirt.xml.in > index e8e6252..f0b7fe0 100644 > --- a/tests/disks/test-qemu-drive-libvirt.xml.in > +++ b/tests/disks/test-qemu-drive-libvirt.xml.in > @@ -132,4 +132,41 @@ > </devices> > </domain> > > + <domain type='test' xmlns:test='http://libvirt.org/schemas/domain/test/1.0'> > + <test:runstate>5</test:runstate> <!-- 5 == VIR_DOMAIN_SHUTOFF --> > + <name>pool1</name> > + <memory>1048576</memory> > + <os> > + <type>hvm</type> > + <boot dev='hd'/> > + </os> > + <devices> > + <disk type='volume' device='disk'> > + <driver name='qemu'/> > + <source pool='pool1' volume='in-pool'/> > + <target dev='vda' bus='virtio'/> > + </disk> > + </devices> > + </domain> > + > + <pool type='dir'> > + <name>pool1</name> > + <uuid>12345678-1234-1234-1234-1234567890ab</uuid> > + <target> > + <path>@abs_builddir@/tmp</path> > + </target> > + > + <volume type='file'> > + <name>in-pool</name> > + <capacity unit='bytes'>1048576</capacity> > + <key>@abs_builddir@/tmp/in-pool</key> > + <source> > + </source> > + <target> > + <path>@abs_builddir@/tmp/in-pool</path> > + </target> > + </volume> > + > + </pool> > + > </node> > -- > 2.7.4 > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs-- 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
Reasonably Related Threads
- [PATCH] libvirt: read disk paths from pools (RHBZ#1366049)
- [PATCH 1/2] libvirt: un-duplicate XPath code
- [PATCH 0/7] Various fixes for Ceph drives and parsing libvirt XML.
- [PATCH v2] lib: add support for disks with 4096 bytes sector size
- [PATCH 0/5] rbd improvements