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 :|