8:57 a.m.
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
8:57 a.m.
New subject: [libvirt] [PATCH 1/3] virsh: detach-disk: Add --print-xml switch
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(a)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
8:57 a.m.
New subject: [libvirt] [PATCH 2/3] util: storage: Remove detected authentication data for backing chains
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(a)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
8:57 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: Remove <backingStore> sub-element in virshFindDisk
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(a)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
8:32 a.m.
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
2502
days inactive
2503
days old
4 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa