On 08/17/2017 01:21 PM, John Ferlan wrote:
>
>
> 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@xxxxxxxxxx>
>> ---
>> 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?)...
Not really. Async jobs are orthogonal to this. But it may be true that
currently only APIs that also set an async job call this function. But
that's just a coincidence. Also, if you look at qemuDomainSaveInternal()
there's no async job held when calling RemoveInactive().
>
>> */
>> 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?
Yeah, I blindly trusted the code. Maybe we need to re-evaluate if this
is still true. But lets save that for another day.
> 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.
It already is. Look at qemuDomainObjBeginJobInternal at error label.
>
>> +
>> + 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?
Oh, that one should be swapped. We should call RemoveInactive() and then
ProcessEndJob(). It doesn't make sense to release a job just so that we
can acquire it again.
>
>> 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 :
Ah, good catch. I'll fix this.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
