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

[Marek Marczykowski-Górecki]

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;
> > +    }
> > +
> > +    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;
> > +    }
> > +
> > +    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).

> Regards,
> Jim
> 
> >       .domainGetOSType = libxlDomainGetOSType, /* 0.9.0 */
> >       .domainGetMaxMemory = libxlDomainGetMaxMemory, /* 0.9.0 */
> >       .domainSetMaxMemory = libxlDomainSetMaxMemory, /* 0.9.2 */
> > 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180906/c671b201/attachment-0001.sig>