diff mbox

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

Message ID 89bc05f5a29055a7442e36166dec836ff65364f7.1460728797.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson April 15, 2016, 2 p.m. UTC
This converts SetTime to the common obj->agent->isactive pattern
(which adds a _third_ IsActive check), and documents the extra
checks
---
 src/qemu/qemu_driver.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

-- 
2.7.3

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

Comments

Cole Robinson April 19, 2016, 4:28 p.m. UTC | #1
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 mbox

Patch

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"));