[libvirt] [PATCH] qemu: Check for domain being active on successful job acquire

[Michal Privoznik]

As this is needed. Although some functions check for domain
being active before obtaining job, we need to check it after,
because obtaining job unlocks domain object, during which
a state of domain can be changed.
---
 src/qemu/qemu_driver.c |   75 
 1 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ec01cd5..1efc66b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1948,9 +1948,18 @@ static int qemuDomainInjectNMI(virDomainPtr domain, 
unsigned int flags)
 
 if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
 goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+"%s", _("domain is not running"));
+goto endjob;
+}
+
 qemuDomainObjEnterMonitorWithDriver(driver, vm);
 ret = qemuMonitorInjectNMI(priv->mon);
 qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+endjob:
 if (qemuDomainObjEndJob(driver, vm) == 0) {
 vm = NULL;
 goto cleanup;
@@ -4387,7 +4396,7 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom,
 virDomainObjPtr vm;
 char *ret = NULL;
 unsigned long balloon;
-int err;
+int err = 0;
 
 /* Flags checked by virDomainDefFormat */
 
@@ -4413,9 +4422,17 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom,
 if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_QUERY) < 
0)
 goto cleanup;
 
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+"%s", _("domain is not running"));
+goto endjob;
+}
+
 qemuDomainObjEnterMonitorWithDriver(driver, vm);
 err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
 qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+endjob:
 if (qemuDomainObjEndJob(driver, vm) == 0) {
 vm = NULL;
 goto cleanup;
@@ -7153,6 +7170,12 @@ qemudDomainBlockStatsFlags (virDomainPtr dom,
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
 goto cleanup;
 
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+"%s", _("domain is not running"));
+goto endjob;
+}
+
 qemuDomainObjEnterMonitor(driver, vm);
 tmp = *nparams;
 ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);
@@ -8672,6 +8695,12 @@ qemuDomainMigrateSetMaxSpeed(virDomainPtr dom,
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MIGRATION_OP) < 0)
 goto cleanup;
 
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+"%s", _("domain is not running"));
+goto endjob;
+}
+
 VIR_DEBUG("Setting migration bandwidth to %luMbs", bandwidth);
 qemuDomainObjEnterMonitor(driver, vm);
 ret = qemuMonitorSetMigrationSpeed(priv->mon, bandwidth);
@@ -8680,6 +8709,7 @@ qemuDomainMigrateSetMaxSpeed(virDomainPtr dom,
 if (ret == 0)
 priv->migMaxBandwidth = bandwidth;
 
+endjob:
 if (qemuDomainObjEndJob(driver, vm) == 0)
 vm = NULL;
 } else {
@@ -8774,6 +8804,12 @@ qemuDomainSnapshotCreateActive(virConnectPtr conn,
 if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
 return -1;
 
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+"%s", _("domain is not running"));
+goto endjob;
+}
+
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
 /* savevm monitor command pauses the domain emitting an event which
  * confuses libvirt since it's not notified when qemu resumes the
@@ -8823,6 +8859,7 @@ cleanup:
 _("resuming after snapshot failed"));
 }
 
+endjob:
 if (vm && qemuDomainObjEndJob(driver, vm) == 0) {
 /* Only possible if a transient vm quit while our locks were down,
  * in which case we don't want to save snapshot metadata.  */
@@ -9045,6 +9082,13 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
 if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
 return -1;
 
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+"%s", _("domain is not running"));
+goto endjob;
+}
+
+
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
 /* In qemu, snapshot_blkdev on a single disk will pause cpus,
  * but this confuses libvirt since notifications are not given
@@ -9127,12 +9171,14 @@ cleanup:
 (persist &&
  virDomainSaveConfig(driver->configDir, vm->newDef) < 0))
 ret = -1;
-if (qemuDomainObjEnd

Re: [libvirt] [PATCH] qemu: Check for domain being active on successful job acquire

[Eric Blake]

On 10/12/2011 05:59 AM, Michal Privoznik wrote:

As this is needed. Although some functions check for domain
being active before obtaining job, we need to check it after,
because obtaining job unlocks domain object, during which
a state of domain can be changed.
---
  src/qemu/qemu_driver.c |   75 
  1 files changed, 69 insertions(+), 6 deletions(-)


ACK.  We've had patches like this in the past (commit 054d43f came to my 
mind).


It might be nicer to someday refactor the code to move the burden into 
the begin-job helper, instead of repeating it on all callers, but as 
that is a bigger patch, and would cause more merge conflicts across 
backports, I'm okay if it gets delayed until someone really has the time 
and motivation to tackle that project.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Check for domain being active on successful job acquire

[Michal Privoznik]

On 12.10.2011 23:35, Eric Blake wrote:
> On 10/12/2011 05:59 AM, Michal Privoznik wrote:
>> As this is needed. Although some functions check for domain
>> being active before obtaining job, we need to check it after,
>> because obtaining job unlocks domain object, during which
>> a state of domain can be changed.
>> ---
>>   src/qemu/qemu_driver.c |   75
>> 
>>   1 files changed, 69 insertions(+), 6 deletions(-)
> 
> ACK.  We've had patches like this in the past (commit 054d43f came to my
> mind).
> 
> It might be nicer to someday refactor the code to move the burden into
> the begin-job helper, instead of repeating it on all callers, but as
> that is a bigger patch, and would cause more merge conflicts across
> backports, I'm okay if it gets delayed until someone really has the time
> and motivation to tackle that project.
> 

Thanks, pushed.

I basically do agree, but we need to keep in mind that some APIs does
not need to check for domain being active, but in fact being not active
(e.g. create).

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list