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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list