| Message ID | 89bc05f5a29055a7442e36166dec836ff65364f7.1460728797.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/19/2016 10:30 AM, Ján Tomko wrote: > 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. > Thanks for your comments, I'll look into that - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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; - + /* 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 */ if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -18746,9 +18746,18 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; } + 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 */ if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running"));