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>
---
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(a)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();