From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37092) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gAhT1-0004R3-JG for qemu-devel@nongnu.org; Thu, 11 Oct 2018 16:21:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gAhSy-0000EG-EJ for qemu-devel@nongnu.org; Thu, 11 Oct 2018 16:21:55 -0400 Received: from mail-it1-x141.google.com ([2607:f8b0:4864:20::141]:39176) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gAhSy-0000Dc-7x for qemu-devel@nongnu.org; Thu, 11 Oct 2018 16:21:52 -0400 Received: by mail-it1-x141.google.com with SMTP id w200-v6so15147141itc.4 for ; Thu, 11 Oct 2018 13:21:52 -0700 (PDT) References: <20180918185246.18109-1-danielhb413@gmail.com> <20180918185246.18109-4-danielhb413@gmail.com> <871s8x7wp4.fsf@dusky.pond.sub.org> From: Daniel Henrique Barboza Message-ID: <75fbd1bc-26e8-55b0-e717-df82782a3390@gmail.com> Date: Thu, 11 Oct 2018 17:21:45 -0300 MIME-Version: 1.0 In-Reply-To: <871s8x7wp4.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Qemu-devel] [PATCH v9 3/3] qmp hmp: Make system_wakeup check wake-up support and run state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, ehabkost@redhat.com, mst@redhat.com, mdroth@linux.vnet.ibm.com, imammedo@redhat.com On 10/11/18 4:45 AM, Markus Armbruster wrote: > Daniel Henrique Barboza 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 has the capability to wake-up from suspend. If >> not, error out informing about the lack of support; >> >> - 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. >> >> 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 >> Signed-off-by: Daniel Henrique Barboza >> --- >> hmp.c | 5 ++++- >> include/sysemu/sysemu.h | 1 + >> qapi/misc.json | 5 ++++- >> qmp.c | 10 ++++++++++ >> vl.c | 2 +- >> 5 files changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/hmp.c b/hmp.c >> index 4975fa56b0..e98c24782f 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -1203,7 +1203,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/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >> index 0446adacc6..8d91c5ad5b 100644 >> --- a/include/sysemu/sysemu.h >> +++ b/include/sysemu/sysemu.h >> @@ -74,6 +74,7 @@ void qemu_exit_preconfig_request(void); >> void qemu_system_reset_request(ShutdownCause reason); >> void qemu_system_suspend_request(void); >> void qemu_register_suspend_notifier(Notifier *notifier); >> +bool qemu_wakeup_suspend_enabled(void); >> void qemu_system_wakeup_request(WakeupReason reason); >> void qemu_system_wakeup_enable(WakeupReason reason, bool enabled); >> void qemu_register_wakeup_notifier(Notifier *notifier); >> diff --git a/qapi/misc.json b/qapi/misc.json >> index b616d385a4..2f5995f45f 100644 >> --- a/qapi/misc.json >> +++ b/qapi/misc.json >> @@ -1230,7 +1230,10 @@ >> ## >> # @system_wakeup: >> # >> -# Wakeup guest from suspend. Does nothing in case the guest isn't suspended. >> +# If the guest has wake-up from suspend support enabled >> +# (wakeup-suspend-support flag from query-current-machine), wakeup guest >> +# from suspend if the guest is in SUSPENDED state. Returns an error >> +# otherwise. > Perhaps a note that versions older than 3.1 neglected to report errors > would be helpful. Ok! > >> # >> # Since: 1.1 >> # >> diff --git a/qmp.c b/qmp.c >> index e7c0a2fd60..44e619d1be 100644 >> --- a/qmp.c >> +++ b/qmp.c >> @@ -183,6 +183,16 @@ void qmp_cont(Error **errp) >> >> void qmp_system_wakeup(Error **errp) >> { >> + if (!qemu_wakeup_suspend_enabled()) { >> + error_setg(errp, >> + "wake-up from suspend is not supported by this guest"); >> + return; >> + } > Is this a new check? > > If not, where is the condition checked before this patch? This is a new check that concerns only the QMP callers, thus I chose not to bake it inside qemu_system_wakeup_request(). > >> + if (!runstate_check(RUN_STATE_SUSPENDED)) { >> + error_setg(errp, >> + "Unable to wake up: guest is not in suspended state"); >> + return; >> + } > Duplicates the check in qemu_system_wakeup_request(). Should that > function take an Error ** argument instead? Yes, it is saner than adding a duplicated check. Shouldn't hurt other callers as well. I'll respin the series with this change. > >> qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER); >> } >> >> diff --git a/vl.c b/vl.c >> index f78947f69c..35a33d61ba 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1759,7 +1759,7 @@ void qemu_register_wakeup_support(void) >> wakeup_suspend_enabled = true; >> } >> >> -static bool qemu_wakeup_suspend_enabled(void) >> +bool qemu_wakeup_suspend_enabled(void) >> { >> return wakeup_suspend_enabled; >> }