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@redhat.com> --- 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 : if (jobStarted) { if (!virDomainObjIsActive()) qemuDomainRemoveInactive qemuDomainObjEndJob } else { if (!virDomainObjIsActive()) qemuDomainRemoveInactiveJob } Although there's a couple questions, either you make the change or can provide the explanation w/ regard to the design choices. So... Reviewed-by: John Ferlan <jferlan@redhat.com> John

virDomainObjEndAPI(&obj); virObjectUnref(conn); virObjectUnref(cfg); @@ -7065,7 +7065,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, */ qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, QEMU_ASYNC_JOB_NONE, 0); - qemuDomainRemoveInactive(src->driver, obj); + qemuDomainRemoveInactiveJob(src->driver, obj);

virDomainObjEndAPI(&obj); virNWFilterUnlockFilterUpdates();