2:52 a.m.
This is v2 to:
https://www.redhat.com/archives/libvir-list/2017-August/msg00222.html
diff to v1:
- Split qemuDomainRemoveInactive into two functions so that its safe to call it
from places where we already have a job.
Michal Privoznik (2):
qemu: Introduce and use qemuDomainRemoveInactiveJob
qemuDomainUndefineFlags: Grab QEMU_JOB_MODIFY
src/qemu/qemu_domain.c | 36 +++++++++++++++++++++++++++---------
src/qemu/qemu_domain.h | 3 +++
src/qemu/qemu_driver.c | 47 ++++++++++++++++++++++++++---------------------
src/qemu/qemu_migration.c | 10 +++++-----
src/qemu/qemu_process.c | 10 +++++-----
5 files changed, 66 insertions(+), 40 deletions(-)
--
2.13.0
2:53 a.m.
New subject: [libvirt] [PATCH v2 1/2] qemu: Introduce and use qemuDomainRemoveInactiveJob
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.
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(-)
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.
*/
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
+ * job the control carries with qemuDomainRemoveInactive though.
+ */
+void
+qemuDomainRemoveInactiveJob(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
+{
+ bool haveJob;
+
+ haveJob = qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) >= 0;
+
+ 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);
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,
if (qemuProcessAttach(conn, driver, vm, pid,
pidfile, monConfig, monJSON) < 0) {
monConfig = NULL;
- qemuDomainObjEndJob(driver, vm);
qemuDomainRemoveInactive(driver, vm);
+ qemuDomainObjEndJob(driver, vm);
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);
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();
--
2.13.0
6:21 a.m.
New subject: [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>
---
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();
4:32 a.m.
New subject: [libvirt] [PATCH v2 1/2] qemu: Introduce and use qemuDomainRemoveInactiveJob
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(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?)...
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
2:53 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Grab QEMU_JOB_MODIFY
This API is definitely modifying state of @vm. Therefore it
should grab a job.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 94c9c003f..3f7909c21 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7325,10 +7325,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (virDomainUndefineFlagsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!vm->persistent) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cannot undefine transient domain"));
- goto cleanup;
+ goto endjob;
}
if (!virDomainObjIsActive(vm) &&
@@ -7338,15 +7341,15 @@ qemuDomainUndefineFlags(virDomainPtr dom,
_("cannot delete inactive domain with %d "
"snapshots"),
nsnapshots);
- goto cleanup;
+ goto endjob;
}
if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0)
- goto cleanup;
+ goto endjob;
}
name = qemuDomainManagedSavePath(driver, vm);
if (name == NULL)
- goto cleanup;
+ goto endjob;
if (virFileExists(name)) {
if (flags & VIR_DOMAIN_UNDEFINE_MANAGED_SAVE) {
@@ -7354,13 +7357,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Failed to remove domain managed "
"save image"));
- goto cleanup;
+ goto endjob;
}
} else {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("Refusing to undefine while domain managed "
"save image exists"));
- goto cleanup;
+ goto endjob;
}
}
@@ -7372,17 +7375,17 @@ qemuDomainUndefineFlags(virDomainPtr dom,
virReportSystemError(errno,
_("failed to remove nvram: %s"),
vm->def->os.loader->nvram);
- goto cleanup;
+ goto endjob;
}
} else if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot delete inactive domain with nvram"));
- goto cleanup;
+ goto endjob;
}
}
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
- goto cleanup;
+ goto endjob;
event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_UNDEFINED,
@@ -7396,9 +7399,11 @@ qemuDomainUndefineFlags(virDomainPtr dom,
*/
vm->persistent = 0;
if (!virDomainObjIsActive(vm))
- qemuDomainRemoveInactiveJob(driver, vm);
+ qemuDomainRemoveInactive(driver, vm);
ret = 0;
+ endjob:
+ qemuDomainObjEndJob(driver, vm);
cleanup:
VIR_FREE(name);
--
2.13.0
6:24 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Grab QEMU_JOB_MODIFY
On 08/15/2017 03:53 AM, Michal Privoznik wrote:
This API is definitely modifying state of @vm. Therefore it
should grab a job.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
4:33 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Grab QEMU_JOB_MODIFY
On 08/17/2017 01:24 PM, John Ferlan wrote:
On 08/15/2017 03:53 AM, Michal Privoznik wrote:
> This API is definitely modifying state of @vm. Therefore it
> should grab a job.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks, pushed.
Michal
2645
days inactive
2659
days old
6 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik