Re: [PATCH 1/2] test_driver: implement virDomainPMSuspendForDuration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 9/3/19 2:00 PM, Daniel Henrique Barboza wrote:


On 8/13/19 1:19 PM, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@xxxxxxxxx>
---
  src/test/test_driver.c | 79 ++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 79 insertions(+)

[...]
+
+    virDomainObjSetState(vm, VIR_DOMAIN_PMSUSPENDED,
+                         VIR_DOMAIN_PMSUSPENDED_UNKNOWN);
+    event_suspend = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_PMSUSPENDED,
+ VIR_DOMAIN_EVENT_SUSPENDED_PAUSED);
+    virObjectEventStateQueue(privconn->eventState, event_suspend);
+
+    if (target == VIR_NODE_SUSPEND_TARGET_DISK) {
+        testDomainShutdownState(dom, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
+        event_shutdown = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_STOPPED,
+ VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
+        if (!vm->persistent)
+            virDomainObjListRemove(privconn->domains, vm);
+    }
+
+    ret = 0;
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    virObjectEventStateQueue(privconn->eventState, event_shutdown);


Unless you're OK with passing a NULL here, you need to check if
'event_shutdown' is NULL at this point, since there's no guarantee that
you set the var  with something else in the 'if target==VIR_NODE_SUSPEND_TARGET_DISK'
conditional.

I'll take a guess here and say that this is unintended, thus it's best to move this 'virObjectEventStateQueue()' call that uses 'event_shutdown' inside the "if"
right before the cleanup label.

Just saw that patch 2/2 did something similar and decided to get more
informed.

Checking what virObjectEventStateQueue() does, I see that it's a wrapper to
virObjectEventStateQueueRemote(), which is a no-op if the second
argument - in this case, event_shutdown - is NULL. This means that the
usage above does not harm, which is good.

I'd still prefer to avoid using virObjectEventStateQueue() like you're doing
here, especially when it's easier to simply move the function call inside
the "if" in which you defined a non-NULL value to event_shutdown.

However, I see this same use of virObjectEventStateQueue() across the
board in test_driver.c. That said, I'll assume that this question was already dealt with in the first time this code pattern was introduced in the code, and
this use is ok.

Rest of the code looks fine, so:


Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>









Thanks,


DHB

+    return ret;
+}
+
+
  #define TEST_TOTAL_CPUTIME 48772617035LL

  static int
@@ -9415,6 +9493,7 @@ static virHypervisorDriver testHypervisorDriver = {
      .domainSendKey = testDomainSendKey, /* 5.5.0 */
      .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */
      .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */
+    .domainPMSuspendForDuration = testDomainPMSuspendForDuration, /* 5.7.0 */
      .domainGetCPUStats = testDomainGetCPUStats, /* 5.6.0 */
      .domainSendProcessSignal = testDomainSendProcessSignal, /* 5.5.0 */       .connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */
--
2.22.0

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


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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux