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/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?  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.


Thanks,

Daniel

>
>> +++ b/qmp.c
>> @@ -201,6 +201,11 @@ void qmp_cont(Error **errp)
>>   
>>   void qmp_system_wakeup(Error **errp)
>>   {
>> +    if (!runstate_check(RUN_STATE_SUSPENDED)) {
>> +        error_setg(errp,
>> +                   "Unable to wake up: guest is not in suspended state");
>> +        return;
>> +    }
>>       qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>>   }
>>   
>>