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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 12/5/18 5:35 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>
>> On 12/4/18 5:15 PM, Eduardo Habkost wrote:
>>> On Tue, Dec 04, 2018 at 04:36:56PM -0200, Daniel Henrique Barboza wrote:
>>>> On 11/29/18 4:55 PM, Markus Armbruster wrote:
>>>>> One more thing...
>>>>>
>>>>> Markus Armbruster <armbru@redhat.com> writes:
>>>>>
>>>>>> Daniel Henrique Barboza <danielhb413@gmail.com> 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 <bala24@linux.vnet.ibm.com>
>>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>>> ---
>>>>>>>     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.
>>>> Good point. I'll simply pass NULL to all callers that didn't care
>>>> for the error prior to this change.
>>> Most times I saw QEMU code ignoring errors, it actually didn't
>>> expect any errors to happen and was supposed to be using
>>> &error_abort instead.
>>>
>>> This makes NULL errp always look like a bug to be fixed.  If you
>>> are sure you really want to ignore an error, I'd recommend adding
>>> a comment making your intention explicit.
> I agree the "ignore errors" feature is widely abused.  I don't agree
> with adding "I mean it" comments.  I think the proper remedy is to flag
> abuses in patch review, and clean them up wherever we find them.
>
>> In this particular case, the existing code wouldn't be expecting
>> errors because
>> qemu_system_wakeup_request wasn't reporting any. Prior to this patch,
>> the function
>> would either change the runstate and notify the wake up event or fail
>> silently.
>>
>> The idea of the patch is to add the extra Error pointer into
>> qemu_system_wakeup_request,
>> so qmp_system_wakeup can propagate the runstate_check error back to
>> hmp_system_wakeup,
>> instead of duplicating this runstate verification inside
>> qmp_system_wakeup (like I was
>> doing in some earlier version). With this idea in mind, passing NULL
>> in the errp
>> of the remaining qemu_system_wakeup_request calls will not improve the
>> existing usage
>> or fix potential bugs, sure, but doesn't make it worse either.
>>
>> I don't see any problems with re-evaluating every existing
>> qemu_system_wakeup_request
>> call and judge f qemu should error_abort out of it in case of
>> error. It's just out of scope of
>> this patch series, IMO.
> Okay.  I asked you to eliminate the code duplication, and I don't want
> to punish your good deed there by demanding further cleanup.  That said,
> further cleanup is always welcome :)

I agree! Let's see if we can get this series pushed first, then we can
get back to all these new "ignore errors" we just added here.


Daniel