[libvirt] [PATCH v2 0/3] libxl: implement virDomainPM* functions
Needed libxl_domain_suspend_only is supported in Xen >= 4.11. But wakeup should work with older versions. Marek Marczykowski-Górecki (3): libxl: send lifecycle event on suspend libxl: implement virDomainPM* functions libxl: initialize domain state with real data src/libxl/libxl_domain.c | 20 +++--- src/libxl/libxl_driver.c | 132 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 144 insertions(+), 8 deletions(-) base-commit: 3ad77f853230f870efa396636e008292c7f2b1c0 -- git-series 0.9.1
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 2ab78ac..b800bc9 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -520,6 +520,18 @@ libxlDomainShutdownThread(void *opaque) case VIR_DOMAIN_LIFECYCLE_ACTION_LAST: goto endjob; } + } else if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND) { + virDomainObjSetState(vm, VIR_DOMAIN_PMSUSPENDED, + VIR_DOMAIN_PMSUSPENDED_UNKNOWN); + + dom_event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_PMSUSPENDED, + VIR_DOMAIN_EVENT_PMSUSPENDED_MEMORY); + /* + * Similar to the xl implementation, ignore SUSPEND. Any actions needed + * after calling libxl_domain_suspend() are handled by it's callers. + */ + goto endjob; } else { VIR_INFO("Unhandled shutdown_reason %d", xl_reason); goto endjob; @@ -563,7 +575,6 @@ void libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) { libxlDriverPrivatePtr driver = data; - libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason; struct libxlShutdownThreadInfo *shutdown_info = NULL; virThread thread; libxlDriverConfigPtr cfg; @@ -574,13 +585,6 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) } /* - * Similar to the xl implementation, ignore SUSPEND. Any actions needed - * after calling libxl_domain_suspend() are handled by its callers. - */ - if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND) - goto error; - - /* * Start a thread to handle shutdown. We don't want to be tying up * libxl's event machinery by doing a potentially lengthy shutdown. */ -- git-series 0.9.1
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 --- 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; + + /* 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) +{ + 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; + + 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 */ .domainGetOSType = libxlDomainGetOSType, /* 0.9.0 */ .domainGetMaxMemory = libxlDomainGetMaxMemory, /* 0.9.0 */ .domainSetMaxMemory = libxlDomainSetMaxMemory, /* 0.9.2 */ -- git-series 0.9.1
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;
+ + /* 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.
+ + 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. Regards, Jim
.domainGetOSType = libxlDomainGetOSType, /* 0.9.0 */ .domainGetMaxMemory = libxlDomainGetMaxMemory, /* 0.9.0 */ .domainSetMaxMemory = libxlDomainSetMaxMemory, /* 0.9.2 */
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?
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@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
On Fri, Sep 07, 2018 at 11:10:14AM -0600, Jim Fehlig wrote:
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...
Perfect timing, I was just going to hit "send" on v3...
+ /* 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.
Not sure about qemu, but here it is done by libxl domain death event handler (libxlDomainShutdownThread in libxl_domain.c). See patch 1/3. -- 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?
When libvirtd is started, initialize domain objects state with its real state, not only RUNNING/SHUTOFF. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0a4e716..4b2d688 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -412,6 +412,17 @@ libxlReconnectDomain(virDomainObjPtr vm, vm->def, hostdev_flags) < 0) goto error; + if (d_info.shutdown && + d_info.shutdown_reason == LIBXL_SHUTDOWN_REASON_SUSPEND) + virDomainObjSetState(vm, VIR_DOMAIN_PMSUSPENDED, + VIR_DOMAIN_PMSUSPENDED_UNKNOWN); + else if (d_info.paused) + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, + VIR_DOMAIN_PAUSED_UNKNOWN); + else + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, + VIR_DOMAIN_RUNNING_UNKNOWN); + if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); -- git-series 0.9.1
participants (2)
-
Jim Fehlig -
Marek Marczykowski-Górecki