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