4:06 p.m.
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>
> ---
> 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?