[libvirt] [PATCH v2 3/4] Wire up QEMU agent to reboot/shutdown APIs

[Eric Blake]

On 01/23/2012 07:48 AM, Michal Privoznik wrote:

[for some reason, this one didn't get threaded properly - bug in git
send-email?]

> This makes use of the QEMU guest agent to implement the
> virDomainShutdownFlags and virDomainReboot APIs. With
> no flags specified, it will prefer to use the agent, but
> fallback to ACPI. Explicit choice can be made by using
> a suitable flag
> 
> * src/qemu/qemu_driver.c: Wire up use of agent
> ---
>  src/qemu/qemu_driver.c |  131 ++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 106 insertions(+), 25 deletions(-)
> 

> -static int qemuDomainShutdown(virDomainPtr dom) {
> +static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) {
>      struct qemud_driver *driver = dom->conn->privateData;
>      virDomainObjPtr vm;
>      int ret = -1;
>      qemuDomainObjPrivatePtr priv;
> +    bool useAgent = false;
> +
> +    virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN |
> +                  VIR_DOMAIN_SHUTDOWN_GUEST_AGENT, -1);
> +
> +    if (flags &
> +        (VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN &
> +         VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) {

Won't work.  That statement is always false.

> +        qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> +                        _("Only one flag supported"));
> +        return -1;
> +    }

Back to my comment in 2/4, I'd prefer that you move this into libvirt.c
(so drivers don't have to duplicate it), where it should look like:

    /* At most one of these two flags should be set.  */
    if ((flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) &&
        (flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) {
        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
        goto error;
    }

> +
> +    if (flags &
> +        (VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN &
> +         VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) {
> +        qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> +                        _("Only one flag supported"));
> +        return -1;
> +    }

Another instance of broken logic that will always be false, and which I
think belongs better in libvirt.c.

I didn't see anything else wrong in the patch, so if you fix patch 2,
then delete the two no-op hunks from patch 3, you have my ACK.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120123/5936521b/attachment-0001.sig>