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.
--
Eduardo
