From 577a1f98fc84e4152c246695942502ef9a45c7f7 Mon Sep 17 00:00:00 2001 From: Jiri Denemark Date: Tue, 6 Aug 2019 14:19:35 +0200 Subject: [PATCH] qemu: Pass correct qemuCaps to virDomainDefParseNode Since qemuDomainDefPostParse callback requires qemuCaps, we need to make sure it gets the capabilities stored in the domain's private data if the domain is running. Passing NULL may cause QEMU capabilities probing to be triggered in case QEMU binary changed in the meantime. When this happens while a running domain object is locked, QMP event delivered to the domain before QEMU capabilities probing finishes will deadlock the event loop. Several general snapshot and checkpoint APIs were lazily passing NULL as the parseOpaque pointer instead of letting their callers pass the right data. This patch fixes all paths leading to virDomainDefParseNode. Signed-off-by: Jiri Denemark Reviewed-by: Michal Privoznik --- src/conf/checkpoint_conf.c | 9 ++++++--- src/conf/checkpoint_conf.h | 1 + src/conf/snapshot_conf.c | 11 ++++++++--- src/conf/snapshot_conf.h | 2 ++ src/esx/esx_driver.c | 2 +- src/qemu/qemu_driver.c | 18 +++++++++++++----- src/test/test_driver.c | 5 +++-- src/vbox/vbox_common.c | 4 ++-- tests/qemudomaincheckpointxml2xmltest.c | 2 +- tests/qemudomainsnapshotxml2xmltest.c | 2 +- 10 files changed, 38 insertions(+), 18 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 5ce4cc4853..b744e2b363 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -128,6 +128,7 @@ static virDomainCheckpointDefPtr virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, unsigned int flags) { virDomainCheckpointDefPtr ret = NULL; @@ -174,7 +175,7 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, return NULL; } def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode, - caps, xmlopt, NULL, + caps, xmlopt, parseOpaque, domainflags); if (!def->parent.dom) return NULL; @@ -207,6 +208,7 @@ virDomainCheckpointDefParseNode(xmlDocPtr xml, xmlNodePtr root, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; @@ -234,7 +236,7 @@ virDomainCheckpointDefParseNode(xmlDocPtr xml, } ctxt->node = root; - def = virDomainCheckpointDefParse(ctxt, caps, xmlopt, flags); + def = virDomainCheckpointDefParse(ctxt, caps, xmlopt, parseOpaque, flags); cleanup: xmlXPathFreeContext(ctxt); return def; @@ -244,6 +246,7 @@ virDomainCheckpointDefPtr virDomainCheckpointDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, unsigned int flags) { virDomainCheckpointDefPtr ret = NULL; @@ -253,7 +256,7 @@ virDomainCheckpointDefParseString(const char *xmlStr, if ((xml = virXMLParse(NULL, xmlStr, _("(domain_checkpoint)")))) { xmlKeepBlanksDefault(keepBlanksDefault); ret = virDomainCheckpointDefParseNode(xml, xmlDocGetRootElement(xml), - caps, xmlopt, flags); + caps, xmlopt, parseOpaque, flags); xmlFreeDoc(xml); } xmlKeepBlanksDefault(keepBlanksDefault); diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h index 03dd6b7bde..47ff69eb4d 100644 --- a/src/conf/checkpoint_conf.h +++ b/src/conf/checkpoint_conf.h @@ -75,6 +75,7 @@ virDomainCheckpointDefPtr virDomainCheckpointDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, unsigned int flags); virDomainCheckpointDefPtr diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 324901a560..7996589ad2 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -228,6 +228,7 @@ static virDomainSnapshotDefPtr virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, bool *current, unsigned int flags) { @@ -303,7 +304,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, goto cleanup; } def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode, - caps, xmlopt, NULL, domainflags); + caps, xmlopt, parseOpaque, + domainflags); if (!def->parent.dom) goto cleanup; } else { @@ -413,6 +415,7 @@ virDomainSnapshotDefParseNode(xmlDocPtr xml, xmlNodePtr root, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, bool *current, unsigned int flags) { @@ -443,7 +446,7 @@ virDomainSnapshotDefParseNode(xmlDocPtr xml, } ctxt->node = root; - def = virDomainSnapshotDefParse(ctxt, caps, xmlopt, current, flags); + def = virDomainSnapshotDefParse(ctxt, caps, xmlopt, parseOpaque, current, flags); cleanup: xmlXPathFreeContext(ctxt); return def; @@ -453,6 +456,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, bool *current, unsigned int flags) { @@ -463,7 +467,8 @@ virDomainSnapshotDefParseString(const char *xmlStr, if ((xml = virXMLParse(NULL, xmlStr, _("(domain_snapshot)")))) { xmlKeepBlanksDefault(keepBlanksDefault); ret = virDomainSnapshotDefParseNode(xml, xmlDocGetRootElement(xml), - caps, xmlopt, current, flags); + caps, xmlopt, parseOpaque, + current, flags); xmlFreeDoc(xml); } xmlKeepBlanksDefault(keepBlanksDefault); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index a0c87e7ade..216726fc16 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -106,12 +106,14 @@ unsigned int virDomainSnapshotFormatConvertXMLFlags(unsigned int flags); virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, bool *current, unsigned int flags); virDomainSnapshotDefPtr virDomainSnapshotDefParseNode(xmlDocPtr xml, xmlNodePtr root, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, bool *current, unsigned int flags); virDomainSnapshotDefPtr virDomainSnapshotDefNew(void); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index b98c72dc3f..d6d219c101 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4117,7 +4117,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; def = virDomainSnapshotDefParseString(xmlDesc, priv->caps, - priv->xmlopt, NULL, parse_flags); + priv->xmlopt, NULL, NULL, parse_flags); if (!def) return NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4da8b0e623..db6b00852f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -464,8 +464,12 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, int ret = -1; virCapsPtr caps = NULL; int direrr; + qemuDomainObjPrivatePtr priv; virObjectLock(vm); + + priv = vm->privateData; + if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to allocate memory for " @@ -504,7 +508,8 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, } def = virDomainSnapshotDefParseString(xmlStr, caps, - qemu_driver->xmlopt, &cur, + qemu_driver->xmlopt, + priv->qemuCaps, &cur, flags); if (def == NULL) { /* Nothing we can do here, skip this one */ @@ -579,8 +584,11 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm, int ret = -1; virCapsPtr caps = NULL; int direrr; + qemuDomainObjPrivatePtr priv; virObjectLock(vm); + priv = vm->privateData; + if (virAsprintf(&chkDir, "%s/%s", baseDir, vm->def->name) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to allocate memory for " @@ -620,6 +628,7 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm, def = virDomainCheckpointDefParseString(xmlStr, caps, qemu_driver->xmlopt, + priv->qemuCaps, flags); if (!def || virDomainCheckpointAlignDisks(def) < 0) { /* Nothing we can do here, skip this one */ @@ -15804,6 +15813,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } + priv = vm->privateData; cfg = virQEMUDriverGetConfig(driver); if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, vm->def, flags) < 0) @@ -15831,7 +15841,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE; if (!(def = virDomainSnapshotDefParseString(xmlDesc, caps, driver->xmlopt, - NULL, parse_flags))) + priv->qemuCaps, NULL, parse_flags))) goto cleanup; /* reject snapshot names containing slashes or starting with dot as @@ -15908,8 +15918,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE); - priv = vm->privateData; - if (redefine) { if (virDomainSnapshotRedefinePrep(domain, vm, &def, &snap, driver->xmlopt, @@ -17182,7 +17190,7 @@ qemuDomainCheckpointCreateXML(virDomainPtr domain, } if (!(def = virDomainCheckpointDefParseString(xmlDesc, caps, driver->xmlopt, - parse_flags))) + priv->qemuCaps, parse_flags))) goto cleanup; /* Unlike snapshots, the RNG schema already ensured a sane filename. */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 043d63b12a..2b5376ec28 100755 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -902,6 +902,7 @@ testParseDomainSnapshots(testDriverPtr privconn, def = virDomainSnapshotDefParseNode(ctxt->doc, node, privconn->caps, privconn->xmlopt, + NULL, &cur, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL | @@ -8207,7 +8208,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, if (!(def = virDomainSnapshotDefParseString(xmlDesc, privconn->caps, privconn->xmlopt, - NULL, + NULL, NULL, parse_flags))) goto cleanup; @@ -8668,7 +8669,7 @@ testDomainCheckpointCreateXML(virDomainPtr domain, } if (!(def = virDomainCheckpointDefParseString(xmlDesc, privconn->caps, - privconn->xmlopt, + privconn->xmlopt, NULL, parse_flags))) goto cleanup; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 6a4ef01e64..db3d940f85 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5505,7 +5505,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE; if (!(def = virDomainSnapshotDefParseString(xmlDesc, data->caps, - data->xmlopt, NULL, + data->xmlopt, NULL, NULL, parse_flags))) goto cleanup; @@ -6949,7 +6949,7 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) } def = virDomainSnapshotDefParseString(defXml, data->caps, - data->xmlopt, NULL, + data->xmlopt, NULL, NULL, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE); if (!def) { diff --git a/tests/qemudomaincheckpointxml2xmltest.c b/tests/qemudomaincheckpointxml2xmltest.c index 8a7c0922c7..9dabe92ab9 100644 --- a/tests/qemudomaincheckpointxml2xmltest.c +++ b/tests/qemudomaincheckpointxml2xmltest.c @@ -54,7 +54,7 @@ testCompareXMLToXMLFiles(const char *inxml, return -1; if (!(def = virDomainCheckpointDefParseString(inXmlData, driver.caps, - driver.xmlopt, + driver.xmlopt, NULL, parseflags))) { if (flags & TEST_INVALID) return 0; diff --git a/tests/qemudomainsnapshotxml2xmltest.c b/tests/qemudomainsnapshotxml2xmltest.c index 10ea9cf8d2..c84ee0bf7d 100644 --- a/tests/qemudomainsnapshotxml2xmltest.c +++ b/tests/qemudomainsnapshotxml2xmltest.c @@ -55,7 +55,7 @@ testCompareXMLToXMLFiles(const char *inxml, goto cleanup; if (!(def = virDomainSnapshotDefParseString(inXmlData, driver.caps, - driver.xmlopt, &cur, + driver.xmlopt, NULL, &cur, parseflags))) goto cleanup; if (cur) {