From 900c5952499fb233a9c0d2e6d0a5fde84a99cf72 Mon Sep 17 00:00:00 2001 From: Jiri Denemark Date: Mon, 5 Aug 2019 16:05:20 +0200 Subject: [PATCH] qemu: Pass qemuCaps to qemuDomainDefFormatBufInternal 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 Reviewed-by: Michal Privoznik --- 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, -- GitLab