Re: [Qemu-devel] [PATCH for-3.2 v10 3/3] qmp hmp: Make system_wakeup check wake-up support and run state

[Markus Armbruster]

One more thing...

Markus Armbruster <address@hidden> writes:

> Daniel Henrique Barboza <address@hidden> writes:
>
>> 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.
>>
>> This means that system_wakeup is silently failing, which can be
>> considered a bug. Adding error handling isn't an API break in this
>> case - applications that didn't check the result will remain broken,
>> the ones that check it will have a chance to deal with it.
>>
>> Adding to that, the commit before previous created a new QMP API called
>> query-current-machine, with a new flag called wakeup-suspend-support,
>> that indicates if the guest has the capability of waking up from suspended
>> state. Although such guest will never reach SUSPENDED state and erroring
>> it out in this scenario would suffice, it is more informative for the user
>> to differentiate between a failure because the guest isn't suspended versus
>> a failure because the guest does not have support for wake up at all.
>>
>> All this considered, this patch changes qmp_system_wakeup to check if
>> the guest is capable of waking up from suspend, and if it is suspended.
>> After this patch, this is the output of system_wakeup in a guest that
>> does not have wake-up from suspend support (ppc64):
>>
>> (qemu) system_wakeup
>> wake-up from suspend is not supported by this guest
>> (qemu)
>>
>> And this is the output of system_wakeup in a x86 guest that has the
>> support but isn't suspended:
>>
>> (qemu) system_wakeup
>> Unable to wake up: guest is not in suspended state
>> (qemu)
>>
>> Reported-by: Balamuruhan S <address@hidden>
>> Signed-off-by: Daniel Henrique Barboza <address@hidden>
>> ---
>>  hmp.c                   |  5 ++++-
>>  hw/acpi/core.c          |  4 +++-
>>  hw/char/serial.c        |  3 ++-
>>  hw/input/ps2.c          |  9 ++++++---
>>  hw/timer/mc146818rtc.c  |  3 ++-
>>  include/sysemu/sysemu.h |  3 ++-
>>  migration/migration.c   |  7 +++++--
>>  qapi/misc.json          |  8 +++++++-
>>  qmp.c                   | 13 ++++++++++++-
>>  vl.c                    |  6 ++++--
>>  10 files changed, 47 insertions(+), 14 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 7828f93a39..0f5d943413 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1220,7 +1220,10 @@ 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/hw/acpi/core.c b/hw/acpi/core.c
>> index 52e18d7810..a7073dd435 100644
>> --- a/hw/acpi/core.c
>> +++ b/hw/acpi/core.c
>> @@ -514,7 +514,9 @@ static uint32_t acpi_pm_tmr_get(ACPIREGS *ar)
>>  static void acpi_pm_tmr_timer(void *opaque)
>>  {
>>      ACPIREGS *ar = opaque;
>> -    qemu_system_wakeup_request(QEMU_WAKEUP_REASON_PMTIMER);
>> +    Error *err = NULL;
>> +
>> +    qemu_system_wakeup_request(QEMU_WAKEUP_REASON_PMTIMER, &err);
>>      ar->tmr.update_sci(ar);
>>  }

Leaks the error object when qemu_system_wakeup_request() fails.

If it cannot fail here, pass &error_abort.

If it can fail, but you want to ignore failure, pass NULL.

More of the same elsewhere.

[...]