[libvirt] [PATCH v2 2/3] libxl: implement virDomainPM* functions

[Jim Fehlig]

On 09/06/2018 03:06 PM, Marek Marczykowski-Górecki wrote:

A few additional comments came to mind while looking at this patch again...

> On Thu, Sep 06, 2018 at 02:58:30PM -0600, Jim Fehlig wrote:
>> On 09/03/2018 04:09 PM, Marek Marczykowski-Górecki wrote:
>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
>>> ---
>>> Changes in v2:
>>>    - use virDomainObjEndAPI
>>>    - drop duplicated error reporting on virDomainObjIsActive
>>>    - bump version comment to 4.8.0
>>
>> You missed some other comments from V1. I'll repeat them here.
>>
>>> ---
>>>    src/libxl/libxl_driver.c | 121 ++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 121 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 5a5e792..0a4e716 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -1403,6 +1403,123 @@ libxlDomainDestroy(virDomainPtr dom)
>>>        return libxlDomainDestroyFlags(dom, 0);
>>>    }
>>> +#ifdef LIBXL_HAVE_DOMAIN_SUSPEND_ONLY
>>> +static int
>>> +libxlDomainPMSuspendForDuration(virDomainPtr dom,
>>> +                                unsigned int target,
>>> +                                unsigned long long duration,
>>> +                                unsigned int flags)
>>> +{
>>> +    virDomainObjPtr vm;
>>> +    int ret = -1;
>>> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
>>> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>>> +
>>> +    virCheckFlags(0, -1);
>>> +    if (target != VIR_NODE_SUSPEND_TARGET_MEM) {
>>> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
>>> +                _("PMSuspend type %d not supported by libxenlight driver"),
>>> +                target);
>>> +        return -1;
>>> +    }
>>> +
>>> +    if (duration != 0) {
>>> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>>> +                _("libxenlight driver supports only duration=0"));
>>> +        return -1;
>>> +    }

We could be more translator friendly and share the same error message as the 
qemu driver

     if (duration) {
         virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
                        _("Duration not supported. Use 0 for now"));
         return -1;
     }

>>> +
>>> +    if (!(vm = libxlDomObjFromDomain(dom)))
>>> +        goto cleanup;
>>> +
>>> +    if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if (!virDomainObjIsActive(vm))
>>> +        goto endjob;
>>
>> You still need to report the error if domain is inactive. To do that use
>> virDomainObjCheckActive(). E.g.
>>
>>      if (virDomainObjCheckActive(vm) < 0)
>>          goto endjob;
> 
> Ah, I've missed s/Is/Check/.
> 
>>> +
>>> +    /* Unlock virDomainObjPtr to not deadlock with even handler, which will try
>>> +     * to send lifecycle event
>>> +     */
>>> +    virObjectUnlock(vm);
>>> +    ret = libxl_domain_suspend_only(cfg->ctx, vm->def->id, NULL);
>>> +    virObjectLock(vm);
>>> +
>>> +    if (ret < 0) {
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                       _("Failed to suspend domain '%d'"), vm->def->id);
>>> +        goto endjob;
>>> +    }
>>> +

Should we create a lifecycle event and/or call virDomainObjSetState on 
successful suspend? Seems neither are done in the qemu driver, but might be an 
oversight there too.

>>> +    ret = 0;
>>> +
>>> + endjob:
>>> +    libxlDomainObjEndJob(driver, vm);
>>> +
>>> + cleanup:
>>> +    virDomainObjEndAPI(&vm);
>>> +    return ret;
>>> +}
>>> +#endif
>>> +
>>> +static int
>>> +libxlDomainPMWakeup(virDomainPtr dom,
>>> +                        unsigned int flags)
>>
>> Whitespace is off, but this line could probably be combined with the previous one.
>>
>>> +{
>>> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
>>> +    virDomainObjPtr vm;
>>> +    int ret = -1;
>>> +    virObjectEventPtr event = NULL;
>>> +    libxlDomainObjPrivatePtr priv;
>>> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>>> +
>>> +    virCheckFlags(0, -1);
>>> +
>>> +    if (!(vm = libxlDomObjFromDomain(dom)))
>>> +        goto cleanup;
>>> +
>>> +    if (virDomainPMWakeupEnsureACL(dom->conn, vm->def) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PMSUSPENDED) {
>>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>>> +                       "%s", _("Domain is not suspended"));
>>> +        goto endjob;
>>> +    }
>>> +
>>> +    event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED,
>>> +                                     VIR_DOMAIN_EVENT_STARTED_WAKEUP);
>>> +
>>> +    priv = vm->privateData;
>>> +    if (libxl_domain_resume(cfg->ctx, vm->def->id, 1, NULL) < 0) {
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                       _("Failed to resume domain '%d'"), vm->def->id);
>>> +        goto endjob;
>>> +    }
>>> +    virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_WAKEUP);
>>> +    /* reenable death event - libxl reports it only once */
>>> +    if (priv->deathW)
>>> +        libxl_evdisable_domain_death(cfg->ctx, priv->deathW);
>>> +    if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW))
>>> +        goto endjob;
>>
>> Is returning failure the right thing to do here? Should we kill off the
>> domain first? In libxlDomainStart we destroy the domain if enabling death
>> events fails.
> 
> That makes sense.
> 
>>> +
>>> +    ret = 0;
>>> +
>>> + endjob:
>>> +    libxlDomainObjEndJob(driver, vm);
>>> +
>>> + cleanup:
>>> +    virDomainObjEndAPI(&vm);
>>> +    virObjectEventStateQueue(driver->domainEventState, event);
>>> +    return ret;
>>> +}
>>> +
>>>    static char *
>>>    libxlDomainGetOSType(virDomainPtr dom)
>>>    {
>>> @@ -6385,6 +6502,10 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>>        .domainReboot = libxlDomainReboot, /* 0.9.0 */
>>>        .domainDestroy = libxlDomainDestroy, /* 0.9.0 */
>>>        .domainDestroyFlags = libxlDomainDestroyFlags, /* 0.9.4 */
>>> +#ifdef LIBXL_HAVE_DOMAIN_SUSPEND_ONLY
>>> +    .domainPMSuspendForDuration = libxlDomainPMSuspendForDuration, /* 4.8.0 */
>>> +#endif
>>> +    .domainPMWakeup = libxlDomainPMWakeup, /* 4.8.0 */
>>
>> Is there a reason to have domainPMWakeup without domainPMSuspendForDuration?
>> I'd think both of these should be conditional on
>> LIBXL_HAVE_DOMAIN_SUSPEND_ONLY.
> 
> Technically, you can suspend domain using other means in such a case (like
> direct write to xenstore).

Ok. I guess I'm on the fence about it. On one hand I don't like the asymmetry, 
but on the other I think it is comparable to wakeup on LAN. Packets arriving at 
the machine and waking it up from suspend had no involvement in suspending the 
machine in the first place.

Regards,
Jim