Re: [libvirt] [PATCH 3/3] qemu: Clarify usage of DomainObjIsActive for SetTime

On Fri, Apr 15, 2016 at 10:00:14AM -0400, Cole Robinson wrote: > This converts SetTime to the common obj->agent->isactive pattern > (which adds a _third_ IsActive check), and documents the extra > checks I would not expect a commit with the summary 'Clarify usage' to change job handling. > --- > src/qemu/qemu_driver.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index f8dfa27..43f22f1 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -18725,9 +18725,9 @@ qemuDomainSetTime(virDomainPtr dom, > > priv = vm->privateData; > > - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) > - goto cleanup; > - This will let us report the capability error without even getting the job. > + /* We also perform this check further down after grabbing the > + job lock, but do it here too so we can throw an error for > + an invalid config, before trying to talk to the guest agent */ s/trying to talk to the guest agent/acquiring the job/ > if (!virDomainObjIsActive(vm)) { > virReportError(VIR_ERR_OPERATION_INVALID, > "%s", _("domain is not running")); > @@ -18746,9 +18746,18 @@ qemuDomainSetTime(virDomainPtr dom, > goto endjob; This goto would end the job before it was even started. > } > > + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) > + goto cleanup; > + > if (!qemuDomainAgentAvailable(vm, true)) > goto endjob; > > + if (!virDomainObjIsActive(vm)) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + "%s", _("domain is not running")); > + goto endjob; > + } > + > qemuDomainObjEnterAgent(vm); > rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync); > qemuDomainObjExitAgent(vm); > @@ -18756,6 +18765,7 @@ qemuDomainSetTime(virDomainPtr dom, > if (rv < 0) > goto endjob; > > + /* The VM may have shut down inbetween, check state again */ This pattern is so common that commenting it is pointless. Maybe the IsActive check could be moved inside qemuDomainObjExitAgent as I've done for qemuDomainObjExitMonitor.