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

[Ilias Stamatis]

On Tue, Sep 3, 2019 at 8:17 PM Daniel Henrique Barboza
<danielhb413 at gmail.com> wrote:
>
>
>
> 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 at gmail.com>
> >> ---
> >>   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.

I think not only in test_driver.c. I took a quick look and it's also
like that in some places in the QEMU driver. Plus I would say this
"pattern" is in general common in the codebase with all these free*()
functions on the cleanup sections which do the same thing - always
check for NULL first.

Additionally, if you want to check for NULL in that case you need to
do it explicitly because virDomainEventLifecycleNewFromObj might also
return NULL. This results in adding extra lines of code. Merely moving
the virObjectEventStateQueue above wouldn't change much. So, since
virObjectEventStateQueue already protects us against NULL I think
maybe it's fine to leave it like that?

In general I'm neutral to this, so feel free to alter it before merging.

Thanks for the review!
Ilias

>
> Rest of the code looks fine, so:
>
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>
>
>
>
>
>
> >
> >
> >
> > 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 at redhat.com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
> >
>