[libvirt] [PATCH 00/10] Pass correct qemuCaps to qemuDomainDefPostParse
Since qemuDomain{Device,}DefPostParse callbacks require qemuCaps, we need to make sure they get 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 the cached capabilities are not valid anymore. 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. The funny part is that qemuDomainDeviceDefPostParse and qemuDomainDefPostParse are a callbacks for virDomainDeviceDefPostParse and virDomainDefPostParse respectively and we need to make sure to properly set the parseOpaque pointer in all paths which could eventually go through these general APIs. If you are interested, you can look at the called-by graph for virDomainDeviceDefPostParse at http://people.redhat.com/jdenemar/virDomainDeviceDefPostParse.svg Only a few of those paths were correct. Jiri Denemark (10): qemu: Pass qemuCaps to qemuDomainDefCopy qemu: Pass qemuCaps to qemuDomainDefFormatBufInternal qemu: Pass qemuCaps to qemuDomainSaveImageOpen qemu: Pass qemuCaps to qemuMigrationAnyPrepareDef qemu: Pass correct qemuCaps to virDomainDefParseString qemu: Pass qemuCaps to qemuMigrationCookieXMLParse qemu: Pass correct qemuCaps to virDomainDefCopy qemu: Pass correct qemuCaps to virDomainDefPostParse qemu: Pass correct qemuCaps to virDomainDefParseNode qemu: Pass correct qemuCaps to virDomainDeviceDefPostParse src/conf/checkpoint_conf.c | 9 +- src/conf/checkpoint_conf.h | 1 + src/conf/domain_conf.c | 41 ++++--- src/conf/domain_conf.h | 13 ++- src/conf/snapshot_conf.c | 11 +- src/conf/snapshot_conf.h | 2 + src/esx/esx_driver.c | 2 +- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 22 ++-- src/libxl/libxl_migration.c | 2 +- src/lxc/lxc_driver.c | 18 +-- src/lxc/lxc_process.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_domain.c | 53 +++++---- src/qemu/qemu_domain.h | 5 + src/qemu/qemu_driver.c | 149 +++++++++++++++--------- src/qemu/qemu_migration.c | 22 ++-- src/qemu/qemu_migration.h | 1 + src/qemu/qemu_migration_cookie.c | 16 ++- src/qemu/qemu_process.c | 13 ++- src/test/test_driver.c | 7 +- src/vbox/vbox_common.c | 8 +- tests/qemudomaincheckpointxml2xmltest.c | 2 +- tests/qemudomainsnapshotxml2xmltest.c | 2 +- tests/qemuhotplugtest.c | 4 +- 26 files changed, 256 insertions(+), 155 deletions(-) -- 2.22.0
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. This patch fixes all paths leading to qemuDomainDefCopy. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 16 ++++++++++------ src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 9 +++++---- src/qemu/qemu_migration.c | 4 ++-- 4 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0555caa6ab..dba5db68ec 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8361,6 +8361,7 @@ qemuDomainObjExitRemote(virDomainObjPtr obj, static virDomainDefPtr qemuDomainDefFromXML(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, const char *xml) { virCapsPtr caps; @@ -8369,7 +8370,7 @@ qemuDomainDefFromXML(virQEMUDriverPtr driver, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) return NULL; - def = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, + def = virDomainDefParseString(xml, caps, driver->xmlopt, qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); @@ -8380,6 +8381,7 @@ qemuDomainDefFromXML(virQEMUDriverPtr driver, virDomainDefPtr qemuDomainDefCopy(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr src, unsigned int flags) { @@ -8389,7 +8391,7 @@ qemuDomainDefCopy(virQEMUDriverPtr driver, if (!(xml = qemuDomainDefFormatXML(driver, src, flags))) return NULL; - ret = qemuDomainDefFromXML(driver, xml); + ret = qemuDomainDefFromXML(driver, qemuCaps, xml); VIR_FREE(xml); return ret; @@ -10616,6 +10618,7 @@ qemuDomainMigratableDefCheckABIStability(virQEMUDriverPtr driver, bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr src, virDomainDefPtr dst) { @@ -10623,8 +10626,8 @@ qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, virDomainDefPtr migratableDefDst = NULL; bool ret = false; - if (!(migratableDefSrc = qemuDomainDefCopy(driver, src, COPY_FLAGS)) || - !(migratableDefDst = qemuDomainDefCopy(driver, dst, COPY_FLAGS))) + if (!(migratableDefSrc = qemuDomainDefCopy(driver, qemuCaps, src, COPY_FLAGS)) || + !(migratableDefDst = qemuDomainDefCopy(driver, qemuCaps, dst, COPY_FLAGS))) goto cleanup; ret = qemuDomainMigratableDefCheckABIStability(driver, @@ -10643,14 +10646,15 @@ qemuDomainCheckABIStability(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDefPtr dst) { + qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr migratableSrc = NULL; virDomainDefPtr migratableDst = NULL; char *xml = NULL; bool ret = false; if (!(xml = qemuDomainFormatXML(driver, vm, COPY_FLAGS)) || - !(migratableSrc = qemuDomainDefFromXML(driver, xml)) || - !(migratableDst = qemuDomainDefCopy(driver, dst, COPY_FLAGS))) + !(migratableSrc = qemuDomainDefFromXML(driver, priv->qemuCaps, xml)) || + !(migratableDst = qemuDomainDefCopy(driver, priv->qemuCaps, dst, COPY_FLAGS))) goto cleanup; ret = qemuDomainMigratableDefCheckABIStability(driver, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b76d3cace9..94ff6e7bd3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -649,6 +649,7 @@ int qemuDomainObjExitRemote(virDomainObjPtr obj, ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; virDomainDefPtr qemuDomainDefCopy(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr src, unsigned int flags); @@ -863,6 +864,7 @@ int qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver, int asyncJob); bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr src, virDomainDefPtr dst); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ff83d1c024..4d6a933243 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6764,7 +6764,7 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto cleanup; - if (!(newdef_migr = qemuDomainDefCopy(driver, + if (!(newdef_migr = qemuDomainDefCopy(driver, NULL, newdef, QEMU_DOMAIN_FORMAT_LIVE_FLAGS | VIR_DOMAIN_XML_MIGRATABLE))) @@ -16553,7 +16553,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, switch ((virDomainSnapshotState) snapdef->state) { case VIR_DOMAIN_SNAPSHOT_RUNNING: case VIR_DOMAIN_SNAPSHOT_PAUSED: - + priv = vm->privateData; start_flags |= VIR_QEMU_PROCESS_START_PAUSED; /* Transitions 2, 3, 5, 6, 8, 9 */ @@ -16580,7 +16580,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (!(config->cpu = virCPUDefCopy(cookie->cpu))) goto endjob; - compatible = qemuDomainDefCheckABIStability(driver, vm->def, + compatible = qemuDomainDefCheckABIStability(driver, + priv->qemuCaps, + vm->def, config); } else { compatible = qemuDomainCheckABIStability(driver, vm, config); @@ -16624,7 +16626,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } } - priv = vm->privateData; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { /* Transitions 5, 6 */ if (qemuProcessStopCPUs(driver, vm, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 7c6be201b9..ec1cae8bec 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2381,7 +2381,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver, if (!newdef) goto cleanup; - if (!qemuDomainDefCheckABIStability(driver, *def, newdef)) { + if (!qemuDomainDefCheckABIStability(driver, NULL, *def, newdef)) { virDomainDefFree(newdef); goto cleanup; } @@ -3427,7 +3427,7 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, goto error; } else { virDomainDefPtr def = vm->newDef ? vm->newDef : vm->def; - if (!(persistDef = qemuDomainDefCopy(driver, def, + if (!(persistDef = qemuDomainDefCopy(driver, priv->qemuCaps, def, VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_MIGRATABLE))) goto error; -- 2.22.0
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. This patch fixes all paths leading to qemuDomainDefFormatBufInternal. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 37 ++++++++++++++++++++------------ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 23 +++++++++++++------- src/qemu/qemu_migration.c | 6 +++--- src/qemu/qemu_migration_cookie.c | 9 ++++++-- src/qemu/qemu_process.c | 9 ++++---- 6 files changed, 56 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dba5db68ec..a95c32d328 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8388,7 +8388,7 @@ qemuDomainDefCopy(virQEMUDriverPtr driver, virDomainDefPtr ret = NULL; char *xml; - if (!(xml = qemuDomainDefFormatXML(driver, src, flags))) + if (!(xml = qemuDomainDefFormatXML(driver, qemuCaps, src, flags))) return NULL; ret = qemuDomainDefFromXML(driver, qemuCaps, xml); @@ -8400,6 +8400,7 @@ qemuDomainDefCopy(virQEMUDriverPtr driver, static int qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr def, virCPUDefPtr origCPU, unsigned int flags, @@ -8408,7 +8409,6 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, int ret = -1; virDomainDefPtr copy = NULL; virCapsPtr caps = NULL; - virQEMUCapsPtr qemuCaps = NULL; virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, -1); @@ -8418,7 +8418,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, if (!(flags & (VIR_DOMAIN_XML_UPDATE_CPU | VIR_DOMAIN_XML_MIGRATABLE))) goto format; - if (!(copy = virDomainDefCopy(def, caps, driver->xmlopt, NULL, + if (!(copy = virDomainDefCopy(def, caps, driver->xmlopt, qemuCaps, flags & VIR_DOMAIN_XML_MIGRATABLE))) goto cleanup; @@ -8429,13 +8429,19 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, def->cpu && (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) { - if (!(qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - def->emulator, - def->os.machine))) - goto cleanup; + VIR_AUTOUNREF(virQEMUCapsPtr) qCaps = NULL; + + if (qemuCaps) { + qCaps = virObjectRef(qemuCaps); + } else { + if (!(qCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, + def->emulator, + def->os.machine))) + goto cleanup; + } if (virCPUUpdate(def->os.arch, def->cpu, - virQEMUCapsGetHostModel(qemuCaps, def->virtType, + virQEMUCapsGetHostModel(qCaps, def->virtType, VIR_QEMU_CAPS_HOST_CPU_MIGRATABLE)) < 0) goto cleanup; } @@ -8584,30 +8590,31 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, cleanup: virDomainDefFree(copy); virObjectUnref(caps); - virObjectUnref(qemuCaps); return ret; } int qemuDomainDefFormatBuf(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr def, unsigned int flags, virBufferPtr buf) { - return qemuDomainDefFormatBufInternal(driver, def, NULL, flags, buf); + return qemuDomainDefFormatBufInternal(driver, qemuCaps, def, NULL, flags, buf); } static char * qemuDomainDefFormatXMLInternal(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr def, virCPUDefPtr origCPU, unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (qemuDomainDefFormatBufInternal(driver, def, origCPU, flags, &buf) < 0) + if (qemuDomainDefFormatBufInternal(driver, qemuCaps, def, origCPU, flags, &buf) < 0) return NULL; return virBufferContentAndReset(&buf); @@ -8616,10 +8623,11 @@ qemuDomainDefFormatXMLInternal(virQEMUDriverPtr driver, char * qemuDomainDefFormatXML(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr def, unsigned int flags) { - return qemuDomainDefFormatXMLInternal(driver, def, NULL, flags); + return qemuDomainDefFormatXMLInternal(driver, qemuCaps, def, NULL, flags); } @@ -8638,11 +8646,12 @@ char *qemuDomainFormatXML(virQEMUDriverPtr driver, origCPU = priv->origCPU; } - return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags); + return qemuDomainDefFormatXMLInternal(driver, priv->qemuCaps, def, origCPU, flags); } char * qemuDomainDefFormatLive(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr def, virCPUDefPtr origCPU, bool inactive, @@ -8655,7 +8664,7 @@ qemuDomainDefFormatLive(virQEMUDriverPtr driver, if (compatible) flags |= VIR_DOMAIN_XML_MIGRATABLE; - return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags); + return qemuDomainDefFormatXMLInternal(driver, qemuCaps, def, origCPU, flags); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 94ff6e7bd3..2ba51929db 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -654,11 +654,13 @@ virDomainDefPtr qemuDomainDefCopy(virQEMUDriverPtr driver, unsigned int flags); int qemuDomainDefFormatBuf(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr vm, unsigned int flags, virBuffer *buf); char *qemuDomainDefFormatXML(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr vm, unsigned int flags); @@ -667,6 +669,7 @@ char *qemuDomainFormatXML(virQEMUDriverPtr driver, unsigned int flags); char *qemuDomainDefFormatLive(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virDomainDefPtr def, virCPUDefPtr origCPU, bool inactive, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4d6a933243..75d6b3a952 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3446,9 +3446,10 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainDefFree(def); goto endjob; } - xml = qemuDomainDefFormatLive(driver, def, NULL, true, true); + xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, def, NULL, true, true); } else { - xml = qemuDomainDefFormatLive(driver, vm->def, priv->origCPU, true, true); + xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def, + priv->origCPU, true, true); } if (!xml) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -7231,7 +7232,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, if (virDomainSaveImageGetXMLDescEnsureACL(conn, def) < 0) goto cleanup; - ret = qemuDomainDefFormatXML(driver, def, flags); + ret = qemuDomainDefFormatXML(driver, NULL, def, flags); cleanup: virQEMUSaveDataFree(data); @@ -7284,7 +7285,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, VIR_FREE(data->xml); - if (!(data->xml = qemuDomainDefFormatXML(driver, newdef, + if (!(data->xml = qemuDomainDefFormatXML(driver, NULL, newdef, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_MIGRATABLE))) @@ -7323,12 +7324,15 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainDefPtr def = NULL; int fd = -1; virQEMUSaveDataPtr data = NULL; + qemuDomainObjPrivatePtr priv; virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); if (!(vm = qemuDomObjFromDomain(dom))) return ret; + priv = vm->privateData; + if (virDomainManagedSaveGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -7345,7 +7349,7 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) false, NULL, false, false)) < 0) goto cleanup; - ret = qemuDomainDefFormatXML(driver, def, flags); + ret = qemuDomainDefFormatXML(driver, priv->qemuCaps, def, flags); cleanup: virQEMUSaveDataFree(data); @@ -15639,7 +15643,8 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, "snapshot", false)) < 0) goto cleanup; - if (!(xml = qemuDomainDefFormatLive(driver, vm->def, priv->origCPU, + if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, + vm->def, priv->origCPU, true, true)) || !(snapdef->cookie = (virObjectPtr) qemuDomainSaveCookieNew(vm))) goto cleanup; @@ -15897,7 +15902,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } else { /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ - if (!(xml = qemuDomainDefFormatLive(driver, vm->def, priv->origCPU, + if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, + vm->def, priv->origCPU, true, true)) || !(def->parent.dom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | @@ -16997,7 +17003,8 @@ qemuDomainCheckpointPrepare(virQEMUDriverPtr driver, virCapsPtr caps, /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ - if (!(xml = qemuDomainDefFormatLive(driver, vm->def, priv->origCPU, + if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, + vm->def, priv->origCPU, true, true)) || !(def->parent.dom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index ec1cae8bec..39e574ee30 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2083,9 +2083,9 @@ qemuMigrationSrcBeginPhase(virQEMUDriverPtr driver, if (!qemuDomainCheckABIStability(driver, vm, def)) goto cleanup; - rv = qemuDomainDefFormatLive(driver, def, NULL, false, true); + rv = qemuDomainDefFormatLive(driver, priv->qemuCaps, def, NULL, false, true); } else { - rv = qemuDomainDefFormatLive(driver, vm->def, priv->origCPU, + rv = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def, priv->origCPU, false, true); } @@ -2355,7 +2355,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver, char *xml; int hookret; - if (!(xml = qemuDomainDefFormatXML(driver, *def, + if (!(xml = qemuDomainDefFormatXML(driver, NULL, *def, VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_MIGRATABLE))) goto cleanup; diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 74b8575a91..74a12d1b03 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -781,6 +781,7 @@ qemuMigrationCookieCapsXMLFormat(virBufferPtr buf, static int qemuMigrationCookieXMLFormat(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, virBufferPtr buf, qemuMigrationCookiePtr mig) { @@ -822,6 +823,7 @@ qemuMigrationCookieXMLFormat(virQEMUDriverPtr driver, if ((mig->flags & QEMU_MIGRATION_COOKIE_PERSISTENT) && mig->persistent) { if (qemuDomainDefFormatBuf(driver, + qemuCaps, mig->persistent, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE | @@ -873,11 +875,12 @@ qemuMigrationCookieXMLFormat(virQEMUDriverPtr driver, static char * qemuMigrationCookieXMLFormatStr(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, qemuMigrationCookiePtr mig) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (qemuMigrationCookieXMLFormat(driver, &buf, mig) < 0) { + if (qemuMigrationCookieXMLFormat(driver, qemuCaps, &buf, mig) < 0) { virBufferFreeAndReset(&buf); return NULL; } @@ -1419,6 +1422,8 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig, int *cookieoutlen, unsigned int flags) { + qemuDomainObjPrivatePtr priv = dom->privateData; + if (!cookieout || !cookieoutlen) return 0; @@ -1462,7 +1467,7 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig, qemuMigrationCookieAddCaps(mig, dom, party) < 0) return -1; - if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig))) + if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, priv->qemuCaps, mig))) return -1; *cookieoutlen = strlen(*cookieout) + 1; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1ed56457b1..9345ecf1be 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4644,13 +4644,14 @@ qemuProcessStartHook(virQEMUDriverPtr driver, virHookQemuOpType op, virHookSubopType subop) { + qemuDomainObjPrivatePtr priv = vm->privateData; char *xml; int ret; if (!virHookPresent(VIR_HOOK_DRIVER_QEMU)) return 0; - if (!(xml = qemuDomainDefFormatXML(driver, vm->def, 0))) + if (!(xml = qemuDomainDefFormatXML(driver, priv->qemuCaps, vm->def, 0))) return -1; ret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, op, subop, @@ -7482,7 +7483,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, /* now that we know it's stopped call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); /* we can't stop the operation even if the script raised an error */ ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -7658,7 +7659,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); + char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); /* we can't stop the operation even if the script raised an error */ virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, @@ -8220,7 +8221,7 @@ qemuProcessReconnect(void *opaque) /* Run an hook to allow admins to do some magic */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, obj->def, 0); + char *xml = qemuDomainDefFormatXML(driver, priv->qemuCaps, obj->def, 0); int hookret; hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, obj->def->name, -- 2.22.0
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. This patch fixes all paths leading to qemuDomainSaveImageOpen. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 75d6b3a952..14a20029dd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6805,6 +6805,7 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver, /** * qemuDomainSaveImageOpen: * @driver: qemu driver data + * @qemuCaps: pointer to qemuCaps if the domain is running or NULL * @path: path of the save image * @ret_def: returns domain definition created from the XML stored in the image * @ret_data: returns structure filled with data from the image header @@ -6819,6 +6820,7 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver, */ static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) qemuDomainSaveImageOpen(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, const char *path, virDomainDefPtr *ret_def, virQEMUSaveDataPtr *ret_data, @@ -6943,7 +6945,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, } /* Create a domain from this XML */ - if (!(def = virDomainDefParseString(data->xml, caps, driver->xmlopt, NULL, + if (!(def = virDomainDefParseString(data->xml, caps, driver->xmlopt, qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto error; @@ -7126,7 +7128,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, virNWFilterReadLockFilterUpdates(); - fd = qemuDomainSaveImageOpen(driver, path, &def, &data, + fd = qemuDomainSaveImageOpen(driver, NULL, path, &def, &data, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, &wrapperFd, false, false); if (fd < 0) @@ -7223,7 +7225,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); - fd = qemuDomainSaveImageOpen(driver, path, &def, &data, + fd = qemuDomainSaveImageOpen(driver, NULL, path, &def, &data, false, NULL, false, false); if (fd < 0) @@ -7261,7 +7263,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, else if (flags & VIR_DOMAIN_SAVE_PAUSED) state = 0; - fd = qemuDomainSaveImageOpen(driver, path, &def, &data, + fd = qemuDomainSaveImageOpen(driver, NULL, path, &def, &data, false, NULL, true, false); if (fd < 0) @@ -7345,7 +7347,7 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; } - if ((fd = qemuDomainSaveImageOpen(driver, path, &def, &data, + if ((fd = qemuDomainSaveImageOpen(driver, priv->qemuCaps, path, &def, &data, false, NULL, false, false)) < 0) goto cleanup; @@ -7412,7 +7414,7 @@ qemuDomainObjRestore(virConnectPtr conn, virQEMUSaveDataPtr data = NULL; virFileWrapperFdPtr wrapperFd = NULL; - fd = qemuDomainSaveImageOpen(driver, path, &def, &data, + fd = qemuDomainSaveImageOpen(driver, NULL, path, &def, &data, bypass_cache, &wrapperFd, false, true); if (fd < 0) { if (fd == -3) -- 2.22.0
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. This patch fixes all paths leading to qemuMigrationAnyPrepareDef. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++------ src/qemu/qemu_migration.c | 8 ++++++-- src/qemu/qemu_migration.h | 1 + 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 14a20029dd..1139fde77a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12651,7 +12651,7 @@ qemuDomainMigratePrepareTunnel(virConnectPtr dconn, goto cleanup; } - if (!(def = qemuMigrationAnyPrepareDef(driver, dom_xml, dname, &origname))) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) goto cleanup; if (virDomainMigratePrepareTunnelEnsureACL(dconn, def) < 0) @@ -12712,7 +12712,7 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, goto cleanup; } - if (!(def = qemuMigrationAnyPrepareDef(driver, dom_xml, dname, &origname))) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) goto cleanup; if (virDomainMigratePrepare2EnsureACL(dconn, def) < 0) @@ -12951,7 +12951,7 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) goto cleanup; - if (!(def = qemuMigrationAnyPrepareDef(driver, dom_xml, dname, &origname))) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) goto cleanup; if (virDomainMigratePrepare3EnsureACL(dconn, def) < 0) @@ -13038,7 +13038,7 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, goto cleanup; } - if (!(def = qemuMigrationAnyPrepareDef(driver, dom_xml, dname, &origname))) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) goto cleanup; if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0) @@ -13092,7 +13092,7 @@ qemuDomainMigratePrepareTunnel3(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) goto cleanup; - if (!(def = qemuMigrationAnyPrepareDef(driver, dom_xml, dname, &origname))) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) goto cleanup; if (virDomainMigratePrepareTunnel3EnsureACL(dconn, def) < 0) @@ -13151,7 +13151,7 @@ qemuDomainMigratePrepareTunnel3Params(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) goto cleanup; - if (!(def = qemuMigrationAnyPrepareDef(driver, dom_xml, dname, &origname))) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) goto cleanup; if (virDomainMigratePrepareTunnel3ParamsEnsureACL(dconn, def) < 0) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 39e574ee30..d07482d9f2 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2861,6 +2861,7 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver, virDomainDefPtr qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, const char *dom_xml, const char *dname, char **origname) @@ -2878,7 +2879,8 @@ qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) return NULL; - if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, NULL, + if (!(def = virDomainDefParseString(dom_xml, caps, driver->xmlopt, + qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto cleanup; @@ -3422,7 +3424,9 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, if (flags & VIR_MIGRATE_PERSIST_DEST) { if (persist_xml) { - if (!(persistDef = qemuMigrationAnyPrepareDef(driver, persist_xml, + if (!(persistDef = qemuMigrationAnyPrepareDef(driver, + priv->qemuCaps, + persist_xml, NULL, NULL))) goto error; } else { diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index bea7b1e688..188ccfa7fd 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -115,6 +115,7 @@ qemuMigrationSrcBegin(virConnectPtr conn, virDomainDefPtr qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, const char *dom_xml, const char *dname, char **origname); -- 2.22.0
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. This patch fixes all paths leading to virDomainDefParseString. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1139fde77a..3004433aa5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3437,7 +3437,8 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, if (xmlin) { virDomainDefPtr def = NULL; - if (!(def = virDomainDefParseString(xmlin, caps, driver->xmlopt, NULL, + if (!(def = virDomainDefParseString(xmlin, caps, driver->xmlopt, + priv->qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) { goto endjob; @@ -15907,7 +15908,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def, priv->origCPU, true, true)) || - !(def->parent.dom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, + !(def->parent.dom = virDomainDefParseString(xml, caps, driver->xmlopt, + priv->qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob; @@ -17008,7 +17010,8 @@ qemuDomainCheckpointPrepare(virQEMUDriverPtr driver, virCapsPtr caps, if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def, priv->origCPU, true, true)) || - !(def->parent.dom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, + !(def->parent.dom = virDomainDefParseString(xml, caps, driver->xmlopt, + priv->qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto cleanup; -- 2.22.0
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. This patch fixes all paths leading to qemuMigrationCookieXMLParse. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration_cookie.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 74a12d1b03..da5bc8d05f 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -1197,6 +1197,7 @@ qemuMigrationCookieCapsXMLParse(xmlXPathContextPtr ctxt) static int qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, xmlDocPtr doc, xmlXPathContextPtr ctxt, unsigned int flags) @@ -1338,7 +1339,7 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, goto error; } mig->persistent = virDomainDefParseNode(doc, nodes[0], - caps, driver->xmlopt, NULL, + caps, driver->xmlopt, qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); @@ -1391,6 +1392,7 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, static int qemuMigrationCookieXMLParseStr(qemuMigrationCookiePtr mig, virQEMUDriverPtr driver, + virQEMUCapsPtr qemuCaps, const char *xml, unsigned int flags) { @@ -1403,7 +1405,7 @@ qemuMigrationCookieXMLParseStr(qemuMigrationCookiePtr mig, if (!(doc = virXMLParseStringCtxt(xml, _("(qemu_migration_cookie)"), &ctxt))) goto cleanup; - ret = qemuMigrationCookieXMLParse(mig, driver, doc, ctxt, flags); + ret = qemuMigrationCookieXMLParse(mig, driver, qemuCaps, doc, ctxt, flags); cleanup: xmlXPathFreeContext(ctxt); @@ -1505,6 +1507,7 @@ qemuMigrationEatCookie(virQEMUDriverPtr driver, if (cookiein && cookieinlen && qemuMigrationCookieXMLParseStr(mig, driver, + priv->qemuCaps, cookiein, flags) < 0) goto error; -- 2.22.0
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 functions from domain_conf.c were lazily passing NULL as the parseOpaque pointer instead of letting their callers pass the right data. This patch fixes all paths leading to virDomainDefCopy to do the right thing. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 18 +++++++++++------- src/conf/domain_conf.h | 9 ++++++--- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 10 +++++----- src/libxl/libxl_migration.c | 2 +- src/lxc/lxc_driver.c | 8 ++++---- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_driver.c | 29 +++++++++++++++++++++-------- src/qemu/qemu_migration.c | 4 +++- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 2 +- tests/qemuhotplugtest.c | 2 +- 12 files changed, 56 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0456369d55..c44ceba7d5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3674,7 +3674,8 @@ virDomainObjWaitUntil(virDomainObjPtr vm, int virDomainObjSetDefTransient(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - virDomainObjPtr domain) + virDomainObjPtr domain, + void *parseOpaque) { int ret = -1; @@ -3684,7 +3685,8 @@ virDomainObjSetDefTransient(virCapsPtr caps, if (domain->newDef) return 0; - if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, NULL, false))) + if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, + parseOpaque, false))) goto out; ret = 0; @@ -3723,10 +3725,11 @@ virDomainObjRemoveTransientDef(virDomainObjPtr domain) virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - virDomainObjPtr domain) + virDomainObjPtr domain, + void *parseOpaque) { if (virDomainObjIsActive(domain) && - virDomainObjSetDefTransient(caps, xmlopt, domain) < 0) + virDomainObjSetDefTransient(caps, xmlopt, domain, parseOpaque) < 0) return NULL; if (domain->newDef) @@ -29341,12 +29344,13 @@ virDomainDefCopy(virDomainDefPtr src, virDomainDefPtr virDomainObjCopyPersistentDef(virDomainObjPtr dom, virCapsPtr caps, - virDomainXMLOptionPtr xmlopt) + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { virDomainDefPtr cur; - cur = virDomainObjGetPersistentDef(caps, xmlopt, dom); - return virDomainDefCopy(cur, caps, xmlopt, NULL, false); + cur = virDomainObjGetPersistentDef(caps, xmlopt, dom, parseOpaque); + return virDomainDefCopy(cur, caps, xmlopt, parseOpaque, false); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 57ca2a8ad1..7b9732fbbc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2902,12 +2902,14 @@ void virDomainObjAssignDef(virDomainObjPtr domain, virDomainDefPtr *oldDef); int virDomainObjSetDefTransient(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - virDomainObjPtr domain); + virDomainObjPtr domain, + void *parseOpaque); void virDomainObjRemoveTransientDef(virDomainObjPtr domain); virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - virDomainObjPtr domain); + virDomainObjPtr domain, + void *parseOpaque); int virDomainObjUpdateModificationImpact(virDomainObjPtr vm, unsigned int *flags); @@ -2928,7 +2930,8 @@ virDomainDefPtr virDomainDefCopy(virDomainDefPtr src, bool migratable); virDomainDefPtr virDomainObjCopyPersistentDef(virDomainObjPtr dom, virCapsPtr caps, - virDomainXMLOptionPtr xmlopt); + virDomainXMLOptionPtr xmlopt, + void *parseOpaque); typedef enum { /* parse internal domain status information */ diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 2d8569e592..b60511a266 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1340,7 +1340,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, VIR_FREE(managed_save_path); } - if (virDomainObjSetDefTransient(cfg->caps, driver->xmlopt, vm) < 0) + if (virDomainObjSetDefTransient(cfg->caps, driver->xmlopt, vm, NULL) < 0) goto cleanup; /* Run an early hook to set-up missing devices */ diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index f430522b35..0e1bf2f0bb 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1607,7 +1607,7 @@ virDomainLiveConfigHelperMethod(virCapsPtr caps, return -1; if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!(*persistentDef = virDomainObjGetPersistentDef(caps, xmlopt, dom))) { + if (!(*persistentDef = virDomainObjGetPersistentDef(caps, xmlopt, dom, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Get persistent config failed")); return -1; @@ -2294,7 +2294,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - if (!(def = virDomainObjGetPersistentDef(cfg->caps, driver->xmlopt, vm))) + if (!(def = virDomainObjGetPersistentDef(cfg->caps, driver->xmlopt, vm, NULL))) goto endjob; maplen = VIR_CPU_MAPLEN(nvcpus); @@ -4126,7 +4126,7 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, /* Make a copy for updated domain. */ if (!(vmdef = virDomainObjCopyPersistentDef(vm, cfg->caps, - driver->xmlopt))) + driver->xmlopt, NULL))) goto endjob; if (libxlDomainAttachDeviceConfig(vmdef, dev) < 0) @@ -4216,7 +4216,7 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, /* Make a copy for updated domain. */ if (!(vmdef = virDomainObjCopyPersistentDef(vm, cfg->caps, - driver->xmlopt))) + driver->xmlopt, NULL))) goto endjob; if (libxlDomainDetachDeviceConfig(vmdef, dev) < 0) @@ -4303,7 +4303,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, /* Make a copy for updated domain. */ if (!(vmdef = virDomainObjCopyPersistentDef(vm, cfg->caps, - driver->xmlopt))) + driver->xmlopt, NULL))) goto cleanup; if ((ret = libxlDomainUpdateDeviceConfig(vmdef, dev)) < 0) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 76bcb66a11..a1021d499b 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -1343,7 +1343,7 @@ libxlDomainMigrationDstFinish(virConnectPtr dconn, vm->persistent = 1; if (!(vmdef = virDomainObjGetPersistentDef(cfg->caps, - driver->xmlopt, vm))) + driver->xmlopt, vm, NULL))) goto cleanup; if (virDomainSaveConfig(cfg->configDir, cfg->caps, vmdef) < 0) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index d0b6703101..a2180b197e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1913,7 +1913,7 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, if (persistentDef) { /* Make a copy for updated domain. */ - persistentDefCopy = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + persistentDefCopy = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt, NULL); if (!persistentDefCopy) goto endjob; } @@ -4735,7 +4735,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt, NULL); if (!vmdef) goto endjob; @@ -4840,7 +4840,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, goto endjob; /* Make a copy for updated domain. */ - if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt))) + if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt, NULL))) goto endjob; /* virDomainDefCompatibleDevice call is delayed until we know the @@ -4919,7 +4919,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt, NULL); if (!vmdef) goto endjob; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index a1d028b2e6..3d03086ea7 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1289,7 +1289,7 @@ int virLXCProcessStart(virConnectPtr conn, * report implicit runtime defaults in the XML, like vnc listen/socket */ VIR_DEBUG("Setting current domain def as transient"); - if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) + if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, NULL) < 0) goto cleanup; /* Run an early hook to set-up missing devices */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3004433aa5..f6e9c8672d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8826,6 +8826,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, const char *xml, unsigned int flags) { + qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr vmdef = NULL; virQEMUDriverConfigPtr cfg = NULL; virDomainDeviceDefPtr devConf = NULL; @@ -8847,7 +8848,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, * rely on the correct vm->def or vm->newDef being passed, so call the * device parse based on which definition is in use */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt, + priv->qemuCaps); if (!vmdef) goto cleanup; @@ -8965,6 +8967,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; + qemuDomainObjPrivatePtr priv; virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; @@ -8987,6 +8990,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; + priv = vm->privateData; + if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -9019,7 +9024,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt, + priv->qemuCaps); if (!vmdef) goto endjob; @@ -9077,6 +9083,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, const char *xml, unsigned int flags) { + qemuDomainObjPrivatePtr priv = vm->privateData; virCapsPtr caps = NULL; virQEMUDriverConfigPtr cfg = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; @@ -9115,7 +9122,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt, priv->qemuCaps); if (!vmdef) goto cleanup; @@ -9171,6 +9178,7 @@ qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver, const char *alias, unsigned int flags) { + qemuDomainObjPrivatePtr priv = vm->privateData; virCapsPtr caps = NULL; virQEMUDriverConfigPtr cfg = NULL; virDomainDefPtr def = NULL; @@ -9197,7 +9205,8 @@ qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver, if (persistentDef) { virDomainDeviceDef dev; - if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt))) + if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt, + priv->qemuCaps))) goto cleanup; if (virDomainDefFindDevice(vmdef, alias, &dev, true) < 0) @@ -10810,7 +10819,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (persistentDef) { /* Make a copy for updated domain. */ if (!(persistentDefCopy = virDomainObjCopyPersistentDef(vm, caps, - driver->xmlopt))) + driver->xmlopt, + priv->qemuCaps))) goto endjob; } @@ -16477,6 +16487,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (!(vm = qemuDomObjFromSnapshot(snapshot))) goto cleanup; + priv = vm->privateData; cfg = virQEMUDriverGetConfig(driver); if (virDomainRevertToSnapshotEnsureACL(snapshot->domain->conn, vm->def) < 0) @@ -16553,7 +16564,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * than inactive xml? */ if (snap->def->dom) { config = virDomainDefCopy(snap->def->dom, caps, - driver->xmlopt, NULL, true); + driver->xmlopt, priv->qemuCaps, true); if (!config) goto endjob; } @@ -16563,7 +16574,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, switch ((virDomainSnapshotState) snapdef->state) { case VIR_DOMAIN_SNAPSHOT_RUNNING: case VIR_DOMAIN_SNAPSHOT_PAUSED: - priv = vm->privateData; start_flags |= VIR_QEMU_PROCESS_START_PAUSED; /* Transitions 2, 3, 5, 6, 8, 9 */ @@ -22016,6 +22026,7 @@ qemuDomainGetFSInfo(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; + qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; qemuAgentPtr agent; virCapsPtr caps = NULL; @@ -22027,6 +22038,8 @@ qemuDomainGetFSInfo(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) return ret; + priv = vm->privateData; + if (virDomainGetFSInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -22042,7 +22055,7 @@ qemuDomainGetFSInfo(virDomainPtr dom, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto endjob; - if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, NULL, false))) + if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, priv->qemuCaps, false))) goto endjob; agent = qemuDomainObjEnterAgent(vm); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d07482d9f2..c2570f7303 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4853,6 +4853,7 @@ qemuMigrationDstPersist(virQEMUDriverPtr driver, bool ignoreSaveError) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv = vm->privateData; virCapsPtr caps = NULL; virDomainDefPtr vmdef; virDomainDefPtr oldDef = NULL; @@ -4867,7 +4868,8 @@ qemuMigrationDstPersist(virQEMUDriverPtr driver, oldDef = vm->newDef; vm->newDef = qemuMigrationCookieGetPersistent(mig); - if (!(vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, vm))) + if (!(vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, vm, + priv->qemuCaps))) goto error; if (virDomainSaveConfig(cfg->configDir, driver->caps, vmdef) < 0 && diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9345ecf1be..ffe4338854 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5637,7 +5637,7 @@ qemuProcessInit(virQEMUDriverPtr driver, * report implicit runtime defaults in the XML, like vnc listen/socket */ VIR_DEBUG("Setting current domain def as transient"); - if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) + if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, priv->qemuCaps) < 0) goto cleanup; /* Update qemu capabilities according to lists passed in via namespace */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6f18baa265..85cfcce79f 100755 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -713,7 +713,7 @@ testDomainStartState(testDriverPtr privconn, if (virDomainObjSetDefTransient(privconn->caps, privconn->xmlopt, - dom) < 0) { + dom, NULL) < 0) { goto cleanup; } diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 354068d748..016a3ed4b1 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -427,7 +427,7 @@ testQemuHotplugCpuPrepare(const char *test, /* create vm->newDef */ data->vm->persistent = true; - if (virDomainObjSetDefTransient(caps, driver.xmlopt, data->vm) < 0) + if (virDomainObjSetDefTransient(caps, driver.xmlopt, data->vm, NULL) < 0) goto error; priv = data->vm->privateData; -- 2.22.0
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. This patch fixes all paths leading to virDomainDefPostParse. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++++------- src/qemu/qemu_process.c | 2 +- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f6e9c8672d..4da8b0e623 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8326,6 +8326,7 @@ static int qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev, virCapsPtr caps, + virQEMUCapsPtr qemuCaps, unsigned int parse_flags, virDomainXMLOptionPtr xmlopt) { @@ -8517,7 +8518,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, return -1; } - if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt, NULL) < 0) + if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt, qemuCaps) < 0) return -1; return 0; @@ -8528,6 +8529,7 @@ static int qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev, virCapsPtr caps, + virQEMUCapsPtr qemuCaps, unsigned int parse_flags, virDomainXMLOptionPtr xmlopt) { @@ -8709,7 +8711,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, return -1; } - if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt, NULL) < 0) + if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt, qemuCaps) < 0) return -1; return 0; @@ -8719,6 +8721,7 @@ static int qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev, virCapsPtr caps, + virQEMUCapsPtr qemuCaps, unsigned int parse_flags, virDomainXMLOptionPtr xmlopt) { @@ -8814,7 +8817,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, return -1; } - if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt, NULL) < 0) + if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt, qemuCaps) < 0) return -1; return 0; @@ -8866,7 +8869,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, false) < 0) goto cleanup; - if (qemuDomainAttachDeviceConfig(vmdef, devConf, caps, + if (qemuDomainAttachDeviceConfig(vmdef, devConf, caps, priv->qemuCaps, parse_flags, driver->xmlopt) < 0) goto cleanup; @@ -9031,7 +9034,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, /* virDomainDefCompatibleDevice call is delayed until we know the * device we're going to update. */ - if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev, caps, + if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev, caps, priv->qemuCaps, parse_flags, driver->xmlopt)) < 0) goto endjob; @@ -9126,7 +9129,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, if (!vmdef) goto cleanup; - if (qemuDomainDetachDeviceConfig(vmdef, dev, caps, + if (qemuDomainDetachDeviceConfig(vmdef, dev, caps, priv->qemuCaps, parse_flags, driver->xmlopt) < 0) goto cleanup; @@ -9212,7 +9215,7 @@ qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver, if (virDomainDefFindDevice(vmdef, alias, &dev, true) < 0) goto cleanup; - if (qemuDomainDetachDeviceConfig(vmdef, &dev, caps, + if (qemuDomainDetachDeviceConfig(vmdef, &dev, caps, priv->qemuCaps, parse_flags, driver->xmlopt) < 0) goto cleanup; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ffe4338854..22ff4c42af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5615,7 +5615,7 @@ qemuProcessInit(virQEMUDriverPtr driver, if (vm->def->postParseFailed) { VIR_DEBUG("re-running the post parse callback"); - if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0) goto cleanup; } -- 2.22.0
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 <jdenemar@redhat.com> --- 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 85cfcce79f..7e87772434 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 | @@ -8168,7 +8169,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, if (!(def = virDomainSnapshotDefParseString(xmlDesc, privconn->caps, privconn->xmlopt, - NULL, + NULL, NULL, parse_flags))) goto cleanup; @@ -8629,7 +8630,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) { -- 2.22.0
Since qemuDomainDeviceDefPostParse 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. QEMU capabilities lookup (via domainPostParseDataAlloc callback) is hidden inside virDomainDeviceDefPostParseOne with no way to pass qemuCaps to virDomainDeviceDef* functions. This patch fixes all remaining paths leading to virDomainDeviceDefPostParse. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 23 ++++++++++++++--------- src/conf/domain_conf.h | 4 +++- src/libxl/libxl_driver.c | 12 ++++++------ src/lxc/lxc_driver.c | 10 +++++----- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 20 ++++++++++++-------- src/vbox/vbox_common.c | 4 ++-- tests/qemuhotplugtest.c | 2 +- 9 files changed, 45 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c44ceba7d5..a3b3e5938d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5454,22 +5454,24 @@ virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps, unsigned int flags, - virDomainXMLOptionPtr xmlopt) + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { - void *parseOpaque = NULL; + void *data = NULL; int ret; - if (xmlopt->config.domainPostParseDataAlloc) { + if (!parseOpaque && xmlopt->config.domainPostParseDataAlloc) { if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags, xmlopt->config.priv, - &parseOpaque) < 0) + &data) < 0) return -1; + parseOpaque = data; } ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque); - if (parseOpaque && xmlopt->config.domainPostParseDataFree) - xmlopt->config.domainPostParseDataFree(parseOpaque); + if (data && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(data); return ret; } @@ -16279,6 +16281,7 @@ virDomainDeviceDefParse(const char *xmlStr, const virDomainDef *def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, unsigned int flags) { xmlDocPtr xml; @@ -16441,7 +16444,8 @@ virDomainDeviceDefParse(const char *xmlStr, } /* callback to fill driver specific device aspects */ - if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0) + if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, + xmlopt, parseOpaque) < 0) goto error; /* validate the configuration */ @@ -29799,7 +29803,8 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, const virDomainDef *def, virCapsPtr caps, - virDomainXMLOptionPtr xmlopt) + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER; int flags = VIR_DOMAIN_DEF_FORMAT_INACTIVE | VIR_DOMAIN_DEF_FORMAT_SECURE; @@ -29887,7 +29892,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, return NULL; xmlStr = virBufferContentAndReset(&buf); - return virDomainDeviceDefParse(xmlStr, def, caps, xmlopt, + return virDomainDeviceDefParse(xmlStr, def, caps, xmlopt, parseOpaque, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7b9732fbbc..0be9f01b19 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2866,7 +2866,8 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def); virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, const virDomainDef *def, virCapsPtr caps, - virDomainXMLOptionPtr xmlopt); + virDomainXMLOptionPtr xmlopt, + void *parseOpaque); virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); void virDomainDeviceSetData(virDomainDeviceDefPtr device, void *devicedata); @@ -2998,6 +2999,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, const virDomainDef *def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, unsigned int flags); virDomainDiskDefPtr virDomainDiskDefParse(const char *xmlStr, const virDomainDef *def, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0e1bf2f0bb..b7a34352c5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4120,7 +4120,7 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, - cfg->caps, driver->xmlopt, + cfg->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto endjob; @@ -4137,7 +4137,7 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, /* If dev exists it was created to modify the domain config. Free it. */ virDomainDeviceDefFree(dev); if (!(dev = virDomainDeviceDefParse(xml, vm->def, - cfg->caps, driver->xmlopt, + cfg->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto endjob; @@ -4209,7 +4209,7 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, - cfg->caps, driver->xmlopt, + cfg->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob; @@ -4227,7 +4227,7 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, /* If dev exists it was created to modify the domain config. Free it. */ virDomainDeviceDefFree(dev); if (!(dev = virDomainDeviceDefParse(xml, vm->def, - cfg->caps, driver->xmlopt, + cfg->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob; @@ -4297,7 +4297,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, - cfg->caps, driver->xmlopt, + cfg->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto cleanup; @@ -4316,7 +4316,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, /* If dev exists it was created to modify the domain config. Free it. */ virDomainDeviceDefFree(dev); if (!(dev = virDomainDeviceDefParse(xml, vm->def, - cfg->caps, driver->xmlopt, + cfg->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a2180b197e..d8ae03c77a 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4716,7 +4716,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, goto endjob; dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, + caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE); if (dev == NULL) goto endjob; @@ -4728,7 +4728,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, * to CONFIG takes one instance. */ dev_copy = virDomainDeviceDefCopy(dev, vm->def, - caps, driver->xmlopt); + caps, driver->xmlopt, NULL); if (!dev_copy) goto endjob; } @@ -4835,7 +4835,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto endjob; - if (!(dev = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, + if (!(dev = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto endjob; @@ -4899,7 +4899,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, goto endjob; dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, + caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); if (dev == NULL) @@ -4912,7 +4912,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, * to CONFIG takes one instance. */ dev_copy = virDomainDeviceDefCopy(dev, vm->def, - caps, driver->xmlopt); + caps, driver->xmlopt, NULL); if (!dev_copy) goto endjob; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 39eeb2c12e..98db7fceec 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1962,7 +1962,7 @@ openvzDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; - dev = virDomainDeviceDefParse(xml, def, driver->caps, driver->xmlopt, + dev = virDomainDeviceDefParse(xml, def, driver->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE); if (!dev) goto cleanup; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index a4b440a24f..1638d527fd 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1691,7 +1691,7 @@ phypDomainAttachDeviceFlags(virDomainPtr domain, def->os.type = VIR_DOMAIN_OSTYPE_LINUX; - dev = virDomainDeviceDefParse(xml, def, phyp_driver->caps, NULL, + dev = virDomainDeviceDefParse(xml, def, phyp_driver->caps, NULL, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE); if (!dev) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index db6b00852f..a60231a658 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8866,7 +8866,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, goto cleanup; if (!(devConf = virDomainDeviceDefParse(xml, vmdef, caps, - driver->xmlopt, parse_flags))) + driver->xmlopt, priv->qemuCaps, + parse_flags))) goto cleanup; if (virDomainDeviceValidateAliasForHotplug(vm, devConf, @@ -8886,7 +8887,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (!(devLive = virDomainDeviceDefParse(xml, vm->def, caps, - driver->xmlopt, parse_flags))) + driver->xmlopt, priv->qemuCaps, + parse_flags))) goto cleanup; if (virDomainDeviceValidateAliasForHotplug(vm, devLive, @@ -9017,8 +9019,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, !(flags & VIR_DOMAIN_AFFECT_LIVE)) parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, + driver->xmlopt, priv->qemuCaps, parse_flags); if (dev == NULL) goto endjob; @@ -9029,7 +9031,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, * create a deep copy of device as adding * to CONFIG takes one instance. */ - dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); + dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, + driver->xmlopt, priv->qemuCaps); if (!dev_copy) goto endjob; } @@ -9115,8 +9118,8 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, !(flags & VIR_DOMAIN_AFFECT_LIVE)) parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, + driver->xmlopt, priv->qemuCaps, parse_flags); if (dev == NULL) goto cleanup; @@ -9127,7 +9130,8 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, * create a deep copy of device as adding * to CONFIG takes one instance. */ - dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); + dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, + driver->xmlopt, priv->qemuCaps); if (!dev_copy) goto cleanup; } diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index db3d940f85..49e657cdb7 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4313,7 +4313,7 @@ static int vboxDomainAttachDeviceImpl(virDomainPtr dom, def->os.type = VIR_DOMAIN_OSTYPE_HVM; - dev = virDomainDeviceDefParse(xml, def, data->caps, data->xmlopt, + dev = virDomainDeviceDefParse(xml, def, data->caps, data->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE); if (dev == NULL) goto cleanup; @@ -4432,7 +4432,7 @@ static int vboxDomainDetachDevice(virDomainPtr dom, const char *xml) def->os.type = VIR_DOMAIN_OSTYPE_HVM; - dev = virDomainDeviceDefParse(xml, def, data->caps, data->xmlopt, + dev = virDomainDeviceDefParse(xml, def, data->caps, data->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); if (dev == NULL) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 016a3ed4b1..6ad67c8902 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -274,7 +274,7 @@ testQemuHotplug(const void *data) device_parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; if (!(dev = virDomainDeviceDefParse(device_xml, vm->def, - caps, driver.xmlopt, + caps, driver.xmlopt, NULL, device_parse_flags))) goto cleanup; -- 2.22.0
On 8/8/19 4:26 PM, Jiri Denemark wrote:
Since qemuDomain{Device,}DefPostParse callbacks require qemuCaps, we need to make sure they get 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 the cached capabilities are not valid anymore. 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.
The funny part is that qemuDomainDeviceDefPostParse and qemuDomainDefPostParse are a callbacks for virDomainDeviceDefPostParse and virDomainDefPostParse respectively and we need to make sure to properly set the parseOpaque pointer in all paths which could eventually go through these general APIs.
If you are interested, you can look at the called-by graph for virDomainDeviceDefPostParse at http://people.redhat.com/jdenemar/virDomainDeviceDefPostParse.svg Only a few of those paths were correct.
Jiri Denemark (10): qemu: Pass qemuCaps to qemuDomainDefCopy qemu: Pass qemuCaps to qemuDomainDefFormatBufInternal qemu: Pass qemuCaps to qemuDomainSaveImageOpen qemu: Pass qemuCaps to qemuMigrationAnyPrepareDef qemu: Pass correct qemuCaps to virDomainDefParseString qemu: Pass qemuCaps to qemuMigrationCookieXMLParse qemu: Pass correct qemuCaps to virDomainDefCopy qemu: Pass correct qemuCaps to virDomainDefPostParse qemu: Pass correct qemuCaps to virDomainDefParseNode qemu: Pass correct qemuCaps to virDomainDeviceDefPostParse
src/conf/checkpoint_conf.c | 9 +- src/conf/checkpoint_conf.h | 1 + src/conf/domain_conf.c | 41 ++++--- src/conf/domain_conf.h | 13 ++- src/conf/snapshot_conf.c | 11 +- src/conf/snapshot_conf.h | 2 + src/esx/esx_driver.c | 2 +- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 22 ++-- src/libxl/libxl_migration.c | 2 +- src/lxc/lxc_driver.c | 18 +-- src/lxc/lxc_process.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_domain.c | 53 +++++---- src/qemu/qemu_domain.h | 5 + src/qemu/qemu_driver.c | 149 +++++++++++++++--------- src/qemu/qemu_migration.c | 22 ++-- src/qemu/qemu_migration.h | 1 + src/qemu/qemu_migration_cookie.c | 16 ++- src/qemu/qemu_process.c | 13 ++- src/test/test_driver.c | 7 +- src/vbox/vbox_common.c | 8 +- tests/qemudomaincheckpointxml2xmltest.c | 2 +- tests/qemudomainsnapshotxml2xmltest.c | 2 +- tests/qemuhotplugtest.c | 4 +- 26 files changed, 256 insertions(+), 155 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Jiri Denemark -
Michal Privoznik