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

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

[Daniel Henrique Barboza]

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 <danielhb@linux.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---

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);
 }
 
 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

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 (danielhb@linux.ibm.com) 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 <danielhb@linux.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
> 
> 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 <dgilbert@redhat.com>

>  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 / dgilbert@redhat.com / Manchester, UK