Re: [Qemu-devel] [PATCH RESEND 1/1] qmp.c: system_wakeup: adding RUN_STATE_SUSPENDED check before proceeding

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2539 bytes --]

On 01/02/2018 11:06 AM, Daniel Henrique Barboza wrote:
> The qmp/hmp command 'system_wakeup' is simply a direct call to
> 'qemu_system_wakeup_request' from vl.c. This function verifies if
> runstate is SUSPENDED and if the wake up reason is valid before
> proceeding. However, no error or warning is thrown if any of those
> pre-requirements isn't met.
> 
> This leads to situations such as the one described in
> https://github.com/open-power-host-os/qemu/issues/31, where one
> can induce the OS to be suspended by using pm-suspend (via
> dompmsuspend, for example) but for some reason the machine failed to
> go to the SUSPENDED runstate, staying at runstate RUNNING. The user
> then tries to wake up the guest using system_wakeup (or dompmwakeup),
> no error is thrown but nothing happened either because the wake up
> wasn't fired at all. In the end, the user is left with a guest that
> is dormant and believing that system_wakeup isn't working.
> 
> This patch changes qmp_system_wakeup to make the runstate verification
> before proceeding to call qemu_system_wakeup_request, firing up
> an error message if the user tries to wake up a machine that
> isn't in SUSPENDED state. The change isn't made inside
> qemu_system_wakeup_request because it is used in migration,
> ACPI and others where this usage might be valid. This patch
> by no means fixes the situation described above, but it can direct
> the user/management closer to the real problem.

Converting something that silently did nothing successfully into now
returning an error is somewhat backwards-incompatible.  Is this
something that needs to go through a deprecation cycle, to give people a
chance to fix QMP scripts that were relying on the previous no-op
behavior to now work with the new semantics?  Or should we add an
optional bool parameter to the command, where omitting the parameter
gives the old semantics, but new enough clients can pass the bool to
choose which behavior (no-op or error) they prefer?

> +++ b/qmp.c
> @@ -201,6 +201,11 @@ void qmp_cont(Error **errp)
>  
>  void qmp_system_wakeup(Error **errp)
>  {
> +    if (!runstate_check(RUN_STATE_SUSPENDED)) {
> +        error_setg(errp,
> +                   "Unable to wake up: guest is not in suspended state");
> +        return;
> +    }
>      qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>  }
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]