* Re: [PATCH] libxl: support ACPI shutdown flag [not found] <1398186736-32336-1-git-send-email-jfehlig@suse.com> @ 2014-04-28 17:24 ` Jim Fehlig [not found] ` <535E8EB3.2050707@suse.com> 2014-04-29 9:06 ` [libvirt] " Daniel P. Berrange 2 siblings, 0 replies; 7+ messages in thread From: Jim Fehlig @ 2014-04-28 17:24 UTC (permalink / raw) To: libvir-list; +Cc: xen-devel Any comments on this small patch? If possible, I'd like to push it for the 1.2.4 release. Thanks! Jim Jim Fehlig wrote: > Add support for VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag in > libxlDomainShutdownFlags(). Inspired by similar functionality > in the Xen xl client. > > Signed-off-by: Jim Fehlig <jfehlig@suse.com> > --- > > I considered invoking libxl_send_trigger() immediately when > VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag is specified, but in the > end decided to only honor the flag if a "normal" shutdown request > failed. This behavior is similar to xl and conforms to the > virDomainShutdownFlags() docs > > "If @flags is set to zero, then the hypervisor will choose the method > of shutdown it considers best. To have greater control pass one or > more of the virDomainShutdownFlagValues. The order in which the > hypervisor tries each shutdown method is undefined, and a hypervisor > is not required to support all methods." > > I'm certainly receptive to only invoking libxl_send_trigger() when > VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is specified if folks think that > is a better approach. > > src/libxl/libxl_driver.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index b3f8df6..2b30c08 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -868,7 +868,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) > int ret = -1; > libxlDomainObjPrivatePtr priv; > > - virCheckFlags(0, -1); > + virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN, -1); > > if (!(vm = libxlDomObjFromDomain(dom))) > goto cleanup; > @@ -883,17 +883,35 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) > } > > priv = vm->privateData; > - if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) { > + ret = libxl_domain_shutdown(priv->ctx, vm->def->id); > + if (ret == ERROR_NOPARAVIRT) { > + if (flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) { > + VIR_DEBUG("PV control interface not available, " > + "sending ACPI power button event."); > + ret = libxl_send_trigger(priv->ctx, vm->def->id, > + LIBXL_TRIGGER_POWER, 0); > + } else { > + virReportError(VIR_ERR_OPERATION_INVALID, > + "%s", _("PV control interface not available, " > + "preventing external graceful shutdown. " > + "Consider using an ACPI power event.")); > + ret = -1; > + goto cleanup; > + } > + } > + > + if (ret != 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Failed to shutdown domain '%d' with libxenlight"), > vm->def->id); > + ret = -1; > goto cleanup; > } > > - /* vm is marked shutoff (or removed from domains list if not persistent) > + /* ret == 0 == successful shutdown request > + * vm is marked shutoff (or removed from domains list if not persistent) > * in shutdown event handler. > */ > - ret = 0; > > cleanup: > if (vm) > ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <535E8EB3.2050707@suse.com>]
* Re: [PATCH] libxl: support ACPI shutdown flag [not found] ` <535E8EB3.2050707@suse.com> @ 2014-04-29 8:35 ` Ian Campbell 0 siblings, 0 replies; 7+ messages in thread From: Ian Campbell @ 2014-04-29 8:35 UTC (permalink / raw) To: Jim Fehlig; +Cc: libvir-list, xen-devel On Mon, 2014-04-28 at 11:24 -0600, Jim Fehlig wrote: > Any comments on this small patch? If possible, I'd like to push it for > the 1.2.4 release. FWIW your use of the libxl interfaces seems correct to me, so Ack in that regard. Ian. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag [not found] <1398186736-32336-1-git-send-email-jfehlig@suse.com> 2014-04-28 17:24 ` [PATCH] libxl: support ACPI shutdown flag Jim Fehlig [not found] ` <535E8EB3.2050707@suse.com> @ 2014-04-29 9:06 ` Daniel P. Berrange 2014-04-29 9:53 ` Ian Campbell [not found] ` <1398765180.4457.19.camel@kazak.uk.xensource.com> 2 siblings, 2 replies; 7+ messages in thread From: Daniel P. Berrange @ 2014-04-29 9:06 UTC (permalink / raw) To: Jim Fehlig; +Cc: libvir-list, xen-devel On Tue, Apr 22, 2014 at 11:12:16AM -0600, Jim Fehlig wrote: > Add support for VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag in > libxlDomainShutdownFlags(). Inspired by similar functionality > in the Xen xl client. > > Signed-off-by: Jim Fehlig <jfehlig@suse.com> > --- > > I considered invoking libxl_send_trigger() immediately when > VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag is specified, but in the > end decided to only honor the flag if a "normal" shutdown request > failed. This behavior is similar to xl and conforms to the > virDomainShutdownFlags() docs > > "If @flags is set to zero, then the hypervisor will choose the method > of shutdown it considers best. To have greater control pass one or > more of the virDomainShutdownFlagValues. The order in which the > hypervisor tries each shutdown method is undefined, and a hypervisor > is not required to support all methods." > > I'm certainly receptive to only invoking libxl_send_trigger() when > VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is specified if folks think that > is a better approach. The way I consider shutdown to operate is slightly different to how you've done it in your patch - flags == 0 -> arbitrary upto the hypervisor to decide. Here you invoke libxl_domain_shutdown() and raise error if this fails. This is a valid implementation, though I'd suggest you could choose to try libxl_send_trigger too in this scenario The QEMU driver considers flags == 0, to be equivalent to the union of all flags it supports. ie it effectivel changes flags==0 to be flags = (ACPI_POWER_BTN | GUEST_AGENT) - flags & VIR_DOMAIN_SHUTDOWN_NNNN -> exclusively try the choice specified by the user. When VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is passed, you are still trying libxl_domain_shutdown() before you try to run libxl_send_trigger(). IMHO this is a bug - it should try libxl_send_trigger exclusively if that was the only bit that was specified in the flags. IIUC libxl_domain_shutdown() will use the Xen paravirt channel to trigger a controlled shutdown in the guest. Currently we have the following flags defined VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */ None of those really map to the Xen paravirt shutdown so I think we should define a new flag in the API. VIR_DOMAIN_SHUTDOWN_PARAVIRT = (1 << 4), /* Use paravirt guest control */ Then I think the pseudo code makes sense - If flags == 0 flags = (PARAVIRT | ACPI_POWER_BTN) - If flags & PARAVIRT libxl_domain_shutdown() if (ret == 0) goto done if (ret != ERROR_NOPARAVIT) { virRaiseError(...) goto cleanup; } - If flags & ACPI_POWER_BTN libxl_send_trigger() if (ret == 0) goto done virRaiseError(...) goto cleanup; This gives users/apps full control over which methods are tried. In the flags==0 case it'll try as many as possible. If neccessary the user can force it to only do ACPI power, or only do paravirt. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag 2014-04-29 9:06 ` [libvirt] " Daniel P. Berrange @ 2014-04-29 9:53 ` Ian Campbell [not found] ` <1398765180.4457.19.camel@kazak.uk.xensource.com> 1 sibling, 0 replies; 7+ messages in thread From: Ian Campbell @ 2014-04-29 9:53 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: libvir-list, Jim Fehlig, xen-devel On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote: > IIUC libxl_domain_shutdown() will use the Xen paravirt channel > to trigger a controlled shutdown in the guest. Currently we have > the following flags defined > > VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ > VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ > VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ > VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ > VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */ > > None of those really map to the Xen paravirt shutdown It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if you consider the bit of the kernel which responds to the paravirt shutdown request as an agent of sorts. With Windows PV drivers I think this is even literally a userspace component. Not that having a more explicit type is a bad idea, but I thought I'd mention it. Ian. ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <1398765180.4457.19.camel@kazak.uk.xensource.com>]
* Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag [not found] ` <1398765180.4457.19.camel@kazak.uk.xensource.com> @ 2014-04-29 10:01 ` Daniel P. Berrange [not found] ` <20140429100106.GF13553@redhat.com> 1 sibling, 0 replies; 7+ messages in thread From: Daniel P. Berrange @ 2014-04-29 10:01 UTC (permalink / raw) To: Ian Campbell; +Cc: libvir-list, Jim Fehlig, xen-devel On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote: > On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote: > > IIUC libxl_domain_shutdown() will use the Xen paravirt channel > > to trigger a controlled shutdown in the guest. Currently we have > > the following flags defined > > > > VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ > > VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ > > VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ > > VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ > > VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */ > > > > None of those really map to the Xen paravirt shutdown > > It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if > you consider the bit of the kernel which responds to the paravirt > shutdown request as an agent of sorts. With Windows PV drivers I think > this is even literally a userspace component. > > Not that having a more explicit type is a bad idea, but I thought I'd > mention it. I guess my rationale for suggesting a new flag is that it is conceivable that Xen could support the virtio-console devices, in which case it would be possible for someone to configure a Xen guest with the QEMU (or another) guest agent provided shutdown facility. In such a scenario, it'd thus be desirable for the API to be able to distinguish Xen paravirt control from the guest agent. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <20140429100106.GF13553@redhat.com>]
* Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag [not found] ` <20140429100106.GF13553@redhat.com> @ 2014-04-29 10:02 ` Ian Campbell 2014-04-29 15:03 ` Jim Fehlig 1 sibling, 0 replies; 7+ messages in thread From: Ian Campbell @ 2014-04-29 10:02 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: libvir-list, Jim Fehlig, xen-devel On Tue, 2014-04-29 at 11:01 +0100, Daniel P. Berrange wrote: > On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote: > > On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote: > > > IIUC libxl_domain_shutdown() will use the Xen paravirt channel > > > to trigger a controlled shutdown in the guest. Currently we have > > > the following flags defined > > > > > > VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ > > > VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ > > > VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ > > > VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ > > > VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */ > > > > > > None of those really map to the Xen paravirt shutdown > > > > It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if > > you consider the bit of the kernel which responds to the paravirt > > shutdown request as an agent of sorts. With Windows PV drivers I think > > this is even literally a userspace component. > > > > Not that having a more explicit type is a bad idea, but I thought I'd > > mention it. > > I guess my rationale for suggesting a new flag is that it is conceivable > that Xen could support the virtio-console devices, in which case it would > be possible for someone to configure a Xen guest with the QEMU (or another) > guest agent provided shutdown facility. In such a scenario, it'd thus be > desirable for the API to be able to distinguish Xen paravirt control from > the guest agent. That makes sense. Ian. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag [not found] ` <20140429100106.GF13553@redhat.com> 2014-04-29 10:02 ` Ian Campbell @ 2014-04-29 15:03 ` Jim Fehlig 1 sibling, 0 replies; 7+ messages in thread From: Jim Fehlig @ 2014-04-29 15:03 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: libvir-list, Ian Campbell, xen-devel Daniel P. Berrange wrote: > On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote: > >> On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote: >> >>> IIUC libxl_domain_shutdown() will use the Xen paravirt channel >>> to trigger a controlled shutdown in the guest. Currently we have >>> the following flags defined >>> >>> VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ >>> VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ >>> VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ >>> VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ >>> VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */ >>> >>> None of those really map to the Xen paravirt shutdown >>> >> It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if >> you consider the bit of the kernel which responds to the paravirt >> shutdown request as an agent of sorts. With Windows PV drivers I think >> this is even literally a userspace component. >> >> Not that having a more explicit type is a bad idea, but I thought I'd >> mention it. >> > > I guess my rationale for suggesting a new flag is that it is conceivable > that Xen could support the virtio-console devices, in which case it would > be possible for someone to configure a Xen guest with the QEMU (or another) > guest agent provided shutdown facility. In such a scenario, it'd thus be > desirable for the API to be able to distinguish Xen paravirt control from > the guest agent. > Agreed. I'll rework the patch as suggested. Regards, Jim ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-04-29 15:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1398186736-32336-1-git-send-email-jfehlig@suse.com>
2014-04-28 17:24 ` [PATCH] libxl: support ACPI shutdown flag Jim Fehlig
[not found] ` <535E8EB3.2050707@suse.com>
2014-04-29 8:35 ` Ian Campbell
2014-04-29 9:06 ` [libvirt] " Daniel P. Berrange
2014-04-29 9:53 ` Ian Campbell
[not found] ` <1398765180.4457.19.camel@kazak.uk.xensource.com>
2014-04-29 10:01 ` Daniel P. Berrange
[not found] ` <20140429100106.GF13553@redhat.com>
2014-04-29 10:02 ` Ian Campbell
2014-04-29 15:03 ` Jim Fehlig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.