Daniel Henrique Barboza <danielhb...@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 <arm...@redhat.com> writes:
>>>>
>>>>> Daniel Henrique Barboza <danielhb...@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 <bal...@linux.vnet.ibm.com>
>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb...@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 :)
