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

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(a)invisiblethingslab.com&gt; >> --- >> 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).