Acquiring job introduced in commit [1] to fix a race described in the commit. Actually it does not help because we get domain in create API before acuiring job. Then [2] fix the race but [1] was not reverted even is does not required by [2] to work properly. [1] commit b629c64e5e0a32ef439b8eeb3a697e2cd76f3248 Author: Martin Kletzander <mkletzan@redhat.com> Date: Thu Oct 30 14:38:35 2014 +0100 qemu: avoid rare race when undefining domain [2] commit c7d1c139ca3402e875002753952e80ce8054374e Author: Martin Kletzander <mkletzan@redhat.com> Date: Thu Dec 11 11:14:08 2014 +0100 qemu: avoid rare race when undefining domain Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@openvz.org> --- src/qemu/qemu_domain.c | 45 +-------------------------------------- src/qemu/qemu_domain.h | 10 ++++----- src/qemu/qemu_driver.c | 12 +++++------ src/qemu/qemu_migration.c | 10 ++++----- src/qemu/qemu_process.c | 12 ++++------- 5 files changed, 20 insertions(+), 69 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a4334de158..95134a3570 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7343,7 +7343,7 @@ qemuDomainRemoveInactive(virQEMUDriver *driver, * lock on driver->domains in order to call the remove obj * from locked list method. */ -static void +void qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, virDomainObj *vm) { @@ -7357,49 +7357,6 @@ qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, virDomainObjListRemoveLocked(driver->domains, vm); } -/** - * qemuDomainRemoveInactiveJob: - * - * Just like qemuDomainRemoveInactive but it tries to grab a - * VIR_JOB_MODIFY first. Even though it doesn't succeed in - * grabbing the job the control carries with - * qemuDomainRemoveInactive call. - */ -void -qemuDomainRemoveInactiveJob(virQEMUDriver *driver, - virDomainObj *vm) -{ - bool haveJob; - - haveJob = qemuDomainObjBeginJob(driver, vm, VIR_JOB_MODIFY) >= 0; - - qemuDomainRemoveInactive(driver, vm); - - if (haveJob) - qemuDomainObjEndJob(vm); -} - - -/** - * qemuDomainRemoveInactiveJobLocked: - * - * Similar to qemuDomainRemoveInactiveJob, except that the caller must - * also hold the lock @driver->domains - */ -void -qemuDomainRemoveInactiveJobLocked(virQEMUDriver *driver, - virDomainObj *vm) -{ - bool haveJob; - - haveJob = qemuDomainObjBeginJob(driver, vm, VIR_JOB_MODIFY) >= 0; - - qemuDomainRemoveInactiveLocked(driver, vm); - - if (haveJob) - qemuDomainObjEndJob(vm); -} - void qemuDomainSetFakeReboot(virDomainObj *vm, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f4d84ca43a..0415a34908 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -672,6 +672,10 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, void qemuDomainRemoveInactive(virQEMUDriver *driver, virDomainObj *vm); +void +qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, + virDomainObj *vm); + void qemuDomainSetFakeReboot(virDomainObj *vm, bool value); @@ -1036,12 +1040,6 @@ int qemuDomainDefNumaCPUsRectify(virDomainDef *def, virQEMUCaps *qemuCaps); -void qemuDomainRemoveInactiveJob(virQEMUDriver *driver, - virDomainObj *vm); - -void qemuDomainRemoveInactiveJobLocked(virQEMUDriver *driver, - virDomainObj *vm); - int virQEMUFileOpenAs(uid_t fallback_uid, gid_t fallback_gid, bool dynamicOwnership, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c0e36e9b2..ee0963c30d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1625,7 +1625,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START, flags) < 0) { - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); goto cleanup; } @@ -2751,7 +2751,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, } qemuDomainObjEndAsyncJob(vm); if (ret == 0) - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); cleanup: virQEMUSaveDataFree(data); @@ -3228,7 +3228,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, qemuDomainObjEndAsyncJob(vm); if (ret == 0 && flags & VIR_DUMP_CRASH) - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); cleanup: virDomainObjEndAPI(&vm); @@ -3536,7 +3536,7 @@ processGuestPanicEvent(virQEMUDriver *driver, endjob: qemuDomainObjEndAsyncJob(vm); if (removeInactive) - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); } @@ -5851,7 +5851,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, virFileWrapperFdFree(wrapperFd); virQEMUSaveDataFree(data); if (vm && ret < 0) - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); virDomainObjEndAPI(&vm); return ret; } @@ -6510,7 +6510,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, } else { /* Brand new domain. Remove it */ VIR_INFO("Deleting domain '%s'", vm->def->name); - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); } } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 30a6202d85..b735bdb391 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3100,7 +3100,7 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, virPortAllocatorRelease(priv->nbdPort); priv->nbdPort = 0; virDomainObjRemoveTransientDef(vm); - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); } virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); @@ -3517,7 +3517,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); } cleanup: @@ -5342,7 +5342,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); } virErrorRestore(&orig_err); @@ -5416,7 +5416,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); return ret; } @@ -5877,7 +5877,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, qemuMigrationJobFinish(vm); if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactiveJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); cleanup: g_clear_pointer(&jobData, virDomainJobDataFree); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2e11d24be2..8f0d1ae7e4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8979,14 +8979,10 @@ qemuProcessReconnect(void *opaque) driver->inhibitCallback(true, driver->inhibitOpaque); cleanup: - if (jobStarted) { - if (!virDomainObjIsActive(obj)) - qemuDomainRemoveInactive(driver, obj); + if (jobStarted) qemuDomainObjEndJob(obj); - } else { - if (!virDomainObjIsActive(obj)) - qemuDomainRemoveInactiveJob(driver, obj); - } + if (!virDomainObjIsActive(obj)) + qemuDomainRemoveInactive(driver, obj); virDomainObjEndAPI(&obj); virIdentitySetCurrent(NULL); return; @@ -9058,7 +9054,7 @@ qemuProcessReconnectHelper(virDomainObj *obj, */ qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, VIR_ASYNC_JOB_NONE, 0); - qemuDomainRemoveInactiveJobLocked(src->driver, obj); + qemuDomainRemoveInactiveLocked(src->driver, obj); virDomainObjEndAPI(&obj); g_clear_object(&data->identity); -- 2.35.1

[1] closes gap in virDomainObjListRemove so that concurrent thread can not step in and obtain the domain while domain is temporary unlocked. But there is another gap exist: thread B - executes create API thread C - executes undefine API - thread A executes some job on domain - threads B and C obtains domain from list and wait for job condition - thread A finishes its job and C grabs job condition, removes domain from list and finishes - thread B grabs job condition and start the domain, unfortunately is not in the list already [1] commit c7d1c139ca3402e875002753952e80ce8054374e Author: Martin Kletzander <mkletzan@redhat.com> Date: Thu Dec 11 11:14:08 2014 +0100 qemu: avoid rare race when undefining domain Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@openvz.org> --- src/qemu/qemu_domainjob.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index 1cddc7f2f0..cb20b798f7 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -849,6 +849,16 @@ qemuDomainObjBeginJobInternal(virQEMUDriver *driver, if (!nested && !qemuDomainNestedJobAllowed(&priv->job, job)) goto retry; + if (obj->removing) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(obj->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, obj->def->name); + goto cleanup; + } + ignore_value(virTimeMillisNow(&now)); if (job) { -- 2.35.1