Re: [libvirt] [PATCH v2 1/2] qemu: Introduce and use qemuDomainRemoveInactiveJob

On 08/15/2017 03:53 AM, Michal Privoznik wrote: > At some places we either already have synchronous job or we just > released it. Also, some APIs might want to use this code without > having to release their job. Anyway, the job acquire code is > moved out to qemuDomainRemoveInactiveJob so that > qemuDomainRemoveInactive does just what it promises. Feels like this is a partial thought as to what's being adjusted here. I think essentially you're trying to state that RemoveInactiveJob is a wrapper to RemoveInactive for paths that don't already have a non async job. For paths with an async job, that job must first be ended before calling/using the new InactiveJob API. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_domain.c | 36 +++++++++++++++++++++++++++--------- > src/qemu/qemu_domain.h | 3 +++ > src/qemu/qemu_driver.c | 26 +++++++++++++------------- > src/qemu/qemu_migration.c | 10 +++++----- > src/qemu/qemu_process.c | 10 +++++----- > 5 files changed, 53 insertions(+), 32 deletions(-) > Should I assume you tested using the scenario from Martin's commit id 'b629c64e5'? > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 40608554c..2b19f841c 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -5187,14 +5187,16 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, > return rem.err; > } > > -/* > + > +/** > + * qemuDomainRemoveInactive: > + * > * The caller must hold a lock the vm. "hold a lock to the vm." And this should only be called if the caller has taken a non asynchronous job (right?)...

> */ > void > qemuDomainRemoveInactive(virQEMUDriverPtr driver, > virDomainObjPtr vm) > { > - bool haveJob = true; > char *snapDir; > virQEMUDriverConfigPtr cfg; > > @@ -5205,9 +5207,6 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, > > cfg = virQEMUDriverGetConfig(driver); > > - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) > - haveJob = false; > - > /* Remove any snapshot metadata prior to removing the domain */ > if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) { > VIR_WARN("unable to remove all snapshots for domain %s", > @@ -5240,13 +5239,32 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, > */ > virObjectLock(vm); > virObjectUnref(cfg); > - > - if (haveJob) > - qemuDomainObjEndJob(driver, vm); > - > virObjectUnref(vm); > } > > + > +/** > + * qemuDomainRemoveInactiveJob: > + * > + * Just like qemuDomainRemoveInactive but it tries to grab a > + * QEMU_JOB_MODIFY before. If it doesn't succeed in grabbing the s/before/first/ s/If it doesn't/Even though it doesn't/ > + * job the control carries with qemuDomainRemoveInactive though. s/job the control carries with/job, continue on with the/ s/though/call/ > + */ > +void > +qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver, > + virDomainObjPtr vm) > +{ > + bool haveJob; > + > + haveJob = qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) >= 0; Is perhaps the reason this was failing because we already had a job in some instances?

Since this is a void path on the path to destruction failure probably won't matter, although I suppose it could be logged in some VIR_DEBUG/WARN manner. Not a requirement, just a thought.

> + > + qemuDomainRemoveInactive(driver, vm); > + > + if (haveJob) > + qemuDomainObjEndJob(driver, vm); > +} > + > + > void > qemuDomainSetFakeReboot(virQEMUDriverPtr driver, > virDomainObjPtr vm, > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index 4c9050aff..f93b09b69 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h > @@ -611,6 +611,9 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, > void qemuDomainRemoveInactive(virQEMUDriverPtr driver, > virDomainObjPtr vm); > > +void qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver, > + virDomainObjPtr vm); > + > void qemuDomainSetFakeReboot(virQEMUDriverPtr driver, > virDomainObjPtr vm, > bool value); > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index e9f07c6e7..94c9c003f 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -1779,7 +1779,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, > def = NULL; > > if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START) < 0) { > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > goto cleanup; > } > > @@ -1789,7 +1789,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, > start_flags) < 0) { > virDomainAuditStart(vm, "booted", false); > qemuProcessEndJob(driver, vm); > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > goto cleanup; > } > > @@ -2259,9 +2259,9 @@ qemuDomainDestroyFlags(virDomainPtr dom, > > ret = 0; > endjob: > - qemuDomainObjEndJob(driver, vm); > if (ret == 0) > qemuDomainRemoveInactive(driver, vm); > + qemuDomainObjEndJob(driver, vm); > > cleanup: > virDomainObjEndAPI(&vm); > @@ -3396,7 +3396,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, > } > qemuDomainObjEndAsyncJob(driver, vm); > if (ret == 0) > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > > cleanup: > virObjectUnref(cookie); > @@ -3916,7 +3916,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, > > qemuDomainObjEndAsyncJob(driver, vm); > if (ret == 0 && flags & VIR_DUMP_CRASH) > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > > cleanup: > virDomainObjEndAPI(&vm); > @@ -4227,7 +4227,7 @@ processGuestPanicEvent(virQEMUDriverPtr driver, > endjob: > qemuDomainObjEndAsyncJob(driver, vm); > if (removeInactive) > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > > cleanup: > virObjectUnref(cfg); > @@ -4729,8 +4729,8 @@ processMonitorEOFEvent(virQEMUDriverPtr driver, > qemuDomainEventQueue(driver, event); > > endjob: > - qemuDomainObjEndJob(driver, vm); > qemuDomainRemoveInactive(driver, vm); > + qemuDomainObjEndJob(driver, vm); > } > > > @@ -6680,7 +6680,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, > VIR_FREE(xmlout); > virFileWrapperFdFree(wrapperFd); > if (vm && ret < 0) > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > virDomainObjEndAPI(&vm); > virNWFilterUnlockFilterUpdates(); > return ret; > @@ -7263,7 +7263,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, > /* Brand new domain. Remove it */ > VIR_INFO("Deleting domain '%s'", vm->def->name); > vm->persistent = 0; > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > } > goto cleanup; > } > @@ -7396,7 +7396,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, > */ > vm->persistent = 0; > if (!virDomainObjIsActive(vm)) > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > > ret = 0; > > @@ -15591,8 +15591,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > } > > if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) { > + qemuDomainRemoveInactive(driver, vm); > qemuProcessEndJob(driver, vm); > - qemuDomainRemoveInactive(driver, vm); Earlier in qemuDomainCreateXML we called qemuProcessEndJob first before using qemuDomainRemoveInactiveJob, but here and in the next hunk it's a different approach with a process job. Shouldn't these both be InactiveJob that go after the ProcessEndJob?

> goto cleanup; > } > if (config) > @@ -15613,8 +15613,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > start_flags); > virDomainAuditStart(vm, "from-snapshot", rc >= 0); > if (rc < 0) { > - qemuProcessEndJob(driver, vm); > qemuDomainRemoveInactive(driver, vm); > + qemuProcessEndJob(driver, vm); > goto cleanup; > } > detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; > @@ -15957,8 +15957,8 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, There's a hunk right above here calling qemuDomainRemoveInactive when qemuDomainObjBeginJob fails, that should be InactiveJob, right? > if (qemuProcessAttach(conn, driver, vm, pid, > pidfile, monConfig, monJSON) < 0) { > monConfig = NULL; > - qemuDomainObjEndJob(driver, vm); > qemuDomainRemoveInactive(driver, vm); > + qemuDomainObjEndJob(driver, vm); Not a problem here, but mostly a mental note that I'm sure to quickly forget! I find it ironic that there's a qemuProcessBeginStopJob without a qemuProcessEndStopJob, instead it's left as a "known" that the process begin start job takes a non async job. > goto cleanup; > } > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 056c051b3..b1f613430 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -2850,7 +2850,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); > priv->nbdPort = 0; > virDomainObjRemoveTransientDef(vm); > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > } > qemuMigrationParamsClear(&migParams); > virDomainObjEndAPI(&vm); > @@ -3291,7 +3291,7 @@ qemuMigrationConfirm(virConnectPtr conn, > virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); > vm->persistent = 0; > } > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > } > > cleanup: > @@ -4867,7 +4867,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, > virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); > vm->persistent = 0; > } > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > } > > if (orig_err) { > @@ -4947,7 +4947,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, > } > > if (!virDomainObjIsActive(vm)) > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > > cleanup: > virDomainObjEndAPI(&vm); > @@ -5388,7 +5388,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver, > > qemuMigrationJobFinish(driver, vm); > if (!virDomainObjIsActive(vm)) > - qemuDomainRemoveInactive(driver, vm); > + qemuDomainRemoveInactiveJob(driver, vm); > > cleanup: > VIR_FREE(jobInfo); > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index fed2bc588..b86ef3757 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -6652,10 +6652,10 @@ qemuProcessAutoDestroy(virDomainObjPtr dom, > VIR_DOMAIN_EVENT_STOPPED, > VIR_DOMAIN_EVENT_STOPPED_DESTROYED); > > - qemuDomainObjEndJob(driver, dom); > - > qemuDomainRemoveInactive(driver, dom); > > + qemuDomainObjEndJob(driver, dom); > + > qemuDomainEventQueue(driver, event); > > cleanup: > @@ -6987,10 +6987,10 @@ qemuProcessReconnect(void *opaque) > driver->inhibitCallback(true, driver->inhibitOpaque); > > cleanup: > - if (jobStarted) > - qemuDomainObjEndJob(driver, obj); > if (!virDomainObjIsActive(obj)) > qemuDomainRemoveInactive(driver, obj); > + if (jobStarted) > + qemuDomainObjEndJob(driver, obj); There would be a chance here that RemoveInactive is called without being in a job, perhaps this should be the rather ugly :