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 <[email protected]> > > --- > > 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?
signature.asc
Description: PGP signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
