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?)...
> */
> 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@xxxxxxxxxx>
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();
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
