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

[Dr. David Alan Gilbert]

* Daniel Henrique Barboza (address@hidden) 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. There is no way for the caller to
> differentiate between a successful wakeup or an error state caused
> when trying to wake up a guest that wasn't suspended. Silent
> failures should be interpreted as bugs and there is no API break
> in fixing this behavior - applications that didn't check the result
> will remain broken, the ones that check will have a chance to deal
> with it.
> 
> 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.
> 
> Signed-off-by: Daniel Henrique Barboza <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Dr. David Alan Gilbert <address@hidden>
> CC: Eric Blake <address@hidden>
> ---
> 
> Changes in v3:
> - improved commit message, as requested by Eric Blake
> - added Eric's R-b
> 
>  hmp.c | 4 +++-
>  qmp.c | 5 +++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 898e25f3e1..2c4c867cab 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1156,7 +1156,9 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
>  
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
>  {
> -    qmp_system_wakeup(NULL);
> +    Error *err = NULL;
> +    qmp_system_wakeup(&err);
> +    hmp_handle_error(mon, &err);
>  }

Fine from HMP side:


Acked-by: Dr. David Alan Gilbert <address@hidden>

>  void hmp_nmi(Monitor *mon, const QDict *qdict)
> diff --git a/qmp.c b/qmp.c
> index 25fdc9a5b2..9c1d0df1ab 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -205,6 +205,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);
>  }
>  
> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK