Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag
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(a)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 :|