Re: [Qemu-devel] [PATCH RESEND 1/1] qmp.c: system_wakeup: adding RUN_STATE_SUSPENDED check before proceeding

[Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>]

On 01/03/2018 11:41 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> writes:
>
>> On 01/02/2018 06:03 PM, Eric Blake wrote:
>>> On 01/02/2018 11:06 AM, Daniel Henrique Barboza wrote:
>>>> 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.
>>>>
>>>> This leads to situations such as the one described in
>>>> https://github.com/open-power-host-os/qemu/issues/31, where one
>>>> can induce the OS to be suspended by using pm-suspend (via
>>>> dompmsuspend, for example) but for some reason the machine failed to
>>>> go to the SUSPENDED runstate, staying at runstate RUNNING. The user
>>>> then tries to wake up the guest using system_wakeup (or dompmwakeup),
>>>> no error is thrown but nothing happened either because the wake up
>>>> wasn't fired at all. In the end, the user is left with a guest that
>>>> is dormant and believing that system_wakeup isn't working.
>>>>
>>>> This patch changes qmp_system_wakeup to 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. The change isn't made inside
>>>> qemu_system_wakeup_request because it is used in migration,
>>>> ACPI and others where this usage might be valid. This patch
>>>> by no means fixes the situation described above, but it can direct
>>>> the user/management closer to the real problem.
>>> Converting something that silently did nothing successfully into now
>>> returning an error is somewhat backwards-incompatible.  Is this
>>> something that needs to go through a deprecation cycle, to give people a
>>> chance to fix QMP scripts that were relying on the previous no-op
>>> behavior to now work with the new semantics?
> I doubt it.
>
> We're fixing a silent failure to be "loud".  How exactly could that
> upset applications?
>
> Applications that don't bother to check the result remain exactly as
> broken as before.

That's true.

>
> Applications that do check become aware of the fact that the command did
> not work as advertized.  I guess this risks breaking them differently.
>
> qmp-spec.txt section 5. gives us license to add errors: "Clients must
> not assume any particular [...]  errors generated by a command, that is,
> new errors can be added to any existing command in newer versions of the
> Server.
>
>>> behavior to now work with the new semantics?  Or should we add an
>>> optional bool parameter to the command, where omitting the parameter
>>> gives the old semantics, but new enough clients can pass the bool to
>>> choose which behavior (no-op or error) they prefer?
>> Good point. Back when I first coded this patch I made sure that no QEMU test
>> were broken, but I didn't consider that existing user scripts that uses
>> system_wakeup might now yield a different result.
>>
>> The extra optional parameter seems a good approach to take here IMO. The
>> current system_wakeup implementation works fine if the user is careful
>> enough to not call it if the guest isn't on SUSPENDED state, so I am unsure
>> if deprecating this behavior in favor of this new one is a bit too extreme.
> I'd say we call the silent failure to wake up the guest a bug, and your
> patch a bug fix.

I am fine with this approach, specially after your argument about an
application that doesn't check the result of system_wakeup is already
broken. This patch would simply make the error explicit.


Daniel

>
> I figure we could find precedence for turning broken successful behavior
> into a clean error if we looked for it.
>