[libvirt] [PATCH] qemu: Don't fail to shutdown domains with unresponsive agent

[Eric Blake]

On 02/25/2013 10:55 AM, Michal Privoznik wrote:
> Currently, qemuDomainShutdownFlags() chooses the agent method of
> shutdown whenever the agent is configured. However, this
> assumption is not enough as the guest agent may be unresponsive
> at the moment. So unless guest agent method has been explicitly
> requested, we should fall back to the ACPI method.
> ---
>  src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)

When Daniel added two new shutdown methods for LXC, I had the question
on whether shutdown methods should be mutually exclusive, or a bitmask
of all permissible things to attempt (where passing 0 leaves the
attempts up the hypervisor).  The consensus was that allowing the user
to pass more than one method makes sense (although the hypervisor then
gets to choose method priorities).  I'm not sure your logic matches the
goal of allowing the user to request both agent and acpi at the same time.

> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 435c37c..06b14ae 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1702,7 +1702,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) {
>      virDomainObjPtr vm;
>      int ret = -1;
>      qemuDomainObjPrivatePtr priv;
> -    bool useAgent = false;
> +    bool useAgent = false, agentRequested;
>  
>      virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN |
>                    VIR_DOMAIN_SHUTDOWN_GUEST_AGENT, -1);
> @@ -1719,23 +1719,32 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) {
>          goto cleanup;
>  
>      priv = vm->privateData;
> +    agentRequested = flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT;
>  
> -    if ((flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT) ||
> +    if (agentRequested ||
>          (!(flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) &&
>           priv->agent))
>          useAgent = true;

A better logic might be:

/* Explicitly requested agent, or no requests made and agent seems
possible */
if (agentRequested ||
    (!flags && priv->agent))

>  
>      if (useAgent) {
>          if (priv->agentError) {
> -            virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
> -                           _("QEMU guest agent is not "
> -                             "available due to an error"));
> -            goto cleanup;
> +            if (agentRequested) {
> +                virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
> +                               _("QEMU guest agent is not "
> +                                 "available due to an error"));
> +                goto cleanup;
> +            } else {
> +                useAgent = false;
> +            }
>          }

Here, if there is an agent error, but the user requested both agent and
acpi flags at the same time, then you want to fall back to acpi instead
of erroring out completely.

>          if (!priv->agent) {
> -            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> -                           _("QEMU guest agent is not configured"));
> -            goto cleanup;
> +            if (agentRequested) {
> +                virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                               _("QEMU guest agent is not configured"));
> +                goto cleanup;
> +            } else {
> +                useAgent = false;
> +            }
>          }
>      }
>  
> @@ -1752,7 +1761,12 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) {
>          qemuDomainObjEnterAgent(vm);
>          ret = qemuAgentShutdown(priv->agent, QEMU_AGENT_SHUTDOWN_POWERDOWN);
>          qemuDomainObjExitAgent(vm);
> -    } else {
> +    }
> +
> +    /* If we are not enforced to use just an agent, try ACPI
> +     * shutdown as well in case agent did not succeed */
> +    if (!useAgent ||
> +        ((ret < 0) && !agentRequested)) {

Redundant () around ret<0.

Here, the fallback seems like it should be:

/* Get here if agent request failed or was not attempted.  Try acpi
unless it was excluded from an explicit request */
if (ret < 0 &&
    (!flags || (flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN))) {


-- 
Eric Blake   eblake 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: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130225/f66f94fd/attachment-0001.sig>