Re: [Qemu-devel] [PATCH v6 2/2] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions

[Daniel Henrique Barboza]

On 05/15/2018 12:35 PM, Markus Armbruster wrote:
> Daniel Henrique Barboza <address@hidden> writes:
>
> > This patch updates the descriptions of 'guest-suspend-ram' and
> > 'guest-suspend-hybrid' to mention that both commands relies now
> > on the existence of 'system_wakeup' and also on the proper support
> > for wake up from suspend, retrieved by the 'wakeup-suspend-support'
> > attribute of the 'query-target' QMP command.
> >
> > Reported-by: Balamuruhan S <address@hidden>
> > Signed-off-by: Daniel Henrique Barboza <address@hidden>
> > Reviewed-by: Michael Roth <address@hidden>
> > ---
> >   qga/qapi-schema.json | 14 ++++++++++----
> >   1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index 17884c7c70..e3fb8adfce 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -566,8 +566,11 @@
> >   # package installed in the guest.
> >   #
> >   # IMPORTANT: guest-suspend-ram requires QEMU to support the 'system_wakeup'
> > -# command.  Thus, it's *required* to query QEMU for the presence of the
> > -# 'system_wakeup' command before issuing guest-suspend-ram.
> > +# command and the guest to support wake up from suspend. Thus, it's
> > +# *required* to query QEMU for the presence of the 'system_wakeup' command
> > +# and to verify that wake up from suspend is enabled by checking the
> > +# 'wakeup-suspend-support' flag of 'query-target' QMP command, before issuing
> > +# guest-suspend-ram.
> Isn't checking for presence of system_wakeup redundant?
>
> When query-target tells us "system_wakeup works" by returning
> wakeup-suspend-support: true, we surely have system_wakeup (or else
> query-target would be lying to us).
>
> When it returns wakeup-suspend-support: false, it doesn't matter whether
> we have system_wakeup.
>
> Unless I'm wrong, we can simplify this to something like
>
>     # IMPORTANT: guest-suspend-ram requires working wakeup support in
>     # QEMU.  You *must* check QMP command query-target returns
>     # wakeup-suspend-support: true before issuing this command.

It is worth noticing that this API isn't checking for the existence of
system_wakeup. It is checking whether there are notifiers added in
the wakeup_notifiers QLIST.

However, I think we can simplify the text as you suggested because that
part seems outdated anyway. Is there any relevant scenario where
system_wakeup will not be present?



> >   #
> >   # This command does NOT return a response on success. There are two options
> >   # to check for success:
> > @@ -593,8 +596,11 @@
> >   # This command requires the pm-utils package to be installed in the guest.
> >   #
> >   # IMPORTANT: guest-suspend-hybrid requires QEMU to support the 'system_wakeup'
> > -# command.  Thus, it's *required* to query QEMU for the presence of the
> > -# 'system_wakeup' command before issuing guest-suspend-hybrid.
> > +# command and the guest to support wake up from suspend. Thus, it's
> > +# *required* to query QEMU for the presence of the 'system_wakeup' command
> > +# and to verify that wake up from suspend is enabled by checking the
> > +# 'wakeup-suspend-support' flag of 'query-target' QMP command, before issuing
> > +# guest-suspend-hybrid.
> >   #
> >   # This command does NOT return a response on success. There are two options
> >   # to check for success:
> Likewise.