[libvirt] [PATCH 0/3] Fix trouble with <backingStore> in virsh detach-disk
We've created malformed <auth> element for RBD images in backing store of a disk which failed parsing and broke detaching disks via virsh. Fix it by not parsing auth data at all from the disk backing images and remove the <backingStore> element when constructing the XML for disk detach. Peter Krempa (3): virsh: detach-disk: Add --print-xml switch util: storage: Remove detected authentication data for backing chains virsh: Remove <backingStore> sub-element in virshFindDisk src/util/virstoragefile.c | 8 ++++++++ tests/virstoragetest.c | 15 +++------------ tools/virsh-domain.c | 44 +++++++++++++++++++++++++++++++++----------- tools/virsh.pod | 4 ++++ 4 files changed, 48 insertions(+), 23 deletions(-) -- 2.15.0
Similarly to other commands add an argument which allows to check the XML which would be used to execute the operation instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 10 ++++++++++ tools/virsh.pod | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5a0e0c1b21..c10bf184f9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12432,6 +12432,10 @@ static const vshCmdOptDef opts_detach_disk[] = { VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = "print-xml", + .type = VSH_OT_BOOL, + .help = N_("print XML document rather than attach the interface") + }, {.name = NULL} }; @@ -12487,6 +12491,12 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + if (vshCommandOptBool(cmd, "print-xml")) { + vshPrint(ctl, "%s", disk_xml); + functionReturn = true; + goto cleanup; + } + if (flags != 0 || current) ret = virDomainDetachDeviceFlags(dom, disk_xml, flags); else diff --git a/tools/virsh.pod b/tools/virsh.pod index 69cc42338d..8f0e8d74b0 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3113,6 +3113,7 @@ I<--persistent>. =item B<detach-disk> I<domain> I<target> [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] +[I<--print-xml>] Detach a disk device from a domain. The I<target> is the device as seen from the domain. @@ -3130,6 +3131,9 @@ an offline domain, and like I<--live> I<--config> for a running domain. Note that older versions of virsh used I<--config> as an alias for I<--persistent>. +If B<--print-xml> is specified, then the XML which would be used to detach the +disk is printed instead. + =item B<detach-interface> I<domain> I<type> [I<--mac mac>] [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] -- 2.15.0
We can't really detect all the authentication data in a sane manner for disk backing chains. Since the old RBD parser parses it in some cases as the argv->XML convertor requires it, we can't just drop it. Instead clear any detected authentication data in the code paths related to disk backing chain lookup and fix the tests to cope with the change. https://bugzilla.redhat.com/show_bug.cgi?id=1544659 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 8 ++++++++ tests/virstoragetest.c | 15 +++------------ 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7f878039ba..440d2b3040 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3399,6 +3399,14 @@ virStorageSourceNewFromBackingAbsolute(const char *path) goto error; virStorageSourceNetworkAssignDefaultPorts(ret); + + /* Some of the legacy parsers parse authentication data since they are + * also used in other places. For backing store detection the + * authentication data would be invalid anyways, so we clear it */ + if (ret->auth) { + virStorageAuthDefFree(ret->auth); + ret->auth = NULL; + } } return ret; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 6eed7134ed..a39c00bab5 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -308,8 +308,7 @@ static const char testStorageChainFormat[] = "type:%d\n" "format:%d\n" "protocol:%s\n" - "hostname:%s\n" - "secret:%s\n"; + "hostname:%s\n"; static int testStorageChain(const void *args) @@ -374,8 +373,7 @@ testStorageChain(const void *args) data->files[i]->type, data->files[i]->format, virStorageNetProtocolTypeToString(data->files[i]->protocol), - NULLSTR(data->files[i]->hostname), - NULLSTR(data->files[i]->secret)) < 0 || + NULLSTR(data->files[i]->hostname)) < 0 || virAsprintf(&actual, testStorageChainFormat, i, NULLSTR(elt->path), @@ -386,8 +384,7 @@ testStorageChain(const void *args) elt->type, elt->format, virStorageNetProtocolTypeToString(elt->protocol), - NULLSTR(elt->nhosts ? elt->hosts[0].name : NULL), - NULLSTR(elt->auth ? elt->auth->username : NULL)) < 0) { + NULLSTR(elt->nhosts ? elt->hosts[0].name : NULL)) < 0) { VIR_FREE(expect); VIR_FREE(actual); goto cleanup; @@ -1361,9 +1358,6 @@ mymain(void) TEST_BACKING_PARSE("rbd:testshare:id=asdf:mon_host=example.com", "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" - " <auth username='asdf'>\n" - " <secret type='ceph'/>\n" - " </auth>\n" "</source>\n"); TEST_BACKING_PARSE("nbd:example.org:6000:exportname=blah", "<source protocol='nbd' name='blah'>\n" @@ -1529,9 +1523,6 @@ mymain(void) "}", "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" - " <auth username='asdf'>\n" - " <secret type='ceph'/>\n" - " </auth>\n" "</source>\n"); TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"rbd\"," "\"image\":\"test\"," -- 2.15.0
Previously we've removed the data only in virshUpdateDiskXML when changing the disk source for the CDROM since the backing store would be invalid. Move the code into a separate function and callit from virshFindDisk which is also used when detaching disk. The detaching code does not necessarily need to get the full backing chain since it will need to act on the one managed by libvirt anyways and this also takes care of problems when parts of the backing store were invalid due to buggy RBD detection code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c10bf184f9..d158327bd7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12151,6 +12151,26 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) return ret; } + +static void +virshDiskDropBackingStore(xmlNodePtr disk_node) +{ + xmlNodePtr tmp; + + for (tmp = disk_node->children; tmp; tmp = tmp->next) { + if (tmp->type != XML_ELEMENT_NODE) + continue; + + if (virXMLNodeNameEqual(tmp, "backingStore")) { + xmlUnlinkNode(tmp); + xmlFreeNode(tmp); + + return; + } + } +} + + typedef enum { VIRSH_FIND_DISK_NORMAL, VIRSH_FIND_DISK_CHANGEABLE, @@ -12228,6 +12248,8 @@ virshFindDisk(const char *doc, if (STREQ_NULLABLE(tmp, path)) { ret = xmlCopyNode(obj->nodesetval->nodeTab[i], 1); + /* drop backing store since they are not needed here */ + virshDiskDropBackingStore(ret); VIR_FREE(tmp); goto cleanup; } @@ -12266,7 +12288,6 @@ virshUpdateDiskXML(xmlNodePtr disk_node, { xmlNodePtr tmp = NULL; xmlNodePtr source = NULL; - xmlNodePtr backingStore = NULL; xmlNodePtr target_node = NULL; xmlNodePtr text_node = NULL; char *device_type = NULL; @@ -12307,22 +12328,13 @@ virshUpdateDiskXML(xmlNodePtr disk_node, if (virXMLNodeNameEqual(tmp, "target")) target_node = tmp; - if (virXMLNodeNameEqual(tmp, "backingStore")) - backingStore = tmp; - /* * We've found all we needed. */ - if (source && target_node && backingStore) + if (source && target_node) break; } - /* drop the <backingStore> subtree since it would become invalid */ - if (backingStore) { - xmlUnlinkNode(backingStore); - xmlFreeNode(backingStore); - } - if (type == VIRSH_UPDATE_DISK_XML_EJECT) { if (!source) { vshError(NULL, _("The disk device '%s' doesn't have media"), target); -- 2.15.0
On Wed, Feb 14, 2018 at 03:57:23PM +0100, Peter Krempa wrote:
We've created malformed <auth> element for RBD images in backing store of a disk which failed parsing and broke detaching disks via virsh.
Fix it by not parsing auth data at all from the disk backing images and remove the <backingStore> element when constructing the XML for disk detach.
Peter Krempa (3): virsh: detach-disk: Add --print-xml switch util: storage: Remove detected authentication data for backing chains virsh: Remove <backingStore> sub-element in virshFindDisk
src/util/virstoragefile.c | 8 ++++++++ tests/virstoragetest.c | 15 +++------------ tools/virsh-domain.c | 44 +++++++++++++++++++++++++++++++++----------- tools/virsh.pod | 4 ++++ 4 files changed, 48 insertions(+), 23 deletions(-)
ACK Jan
participants (2)
-
Ján Tomko -
Peter Krempa