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@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?