Re: [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 04/20/2017 06:59 AM, Markus Armbruster wrote:
>
>> 
>> No objection to Alistair's idea to turn this into an enumeration.
>
> Question - should the enum be more than just 'guest' and 'host'?  For
> example, my patch proves that we have a lot of places that handle
> complimentary machine commands to reset and shutdown, and that whether
> 'reset' triggers a reset (and the guest keeps running as if rebooted) or
> a shutdown is then based on the command-line arguments given to qemu.
> So having the enum differentiate between 'guest-reset' and
> 'guest-shutdown' would be a possibility, if we want the enum to have
> additional states.

I don't know.  What I do know is that we better get the enum right:
while adding members is backwards-compatible, changing the member sent
for a specific trigger is not.  If we want to reserve the option to do
that anyway, we need suitable documentation.

>>> +++ b/vl.c
>>> @@ -1717,7 +1717,7 @@ void qemu_system_guest_panicked(GuestPanicInformation 
>>> *info)
>>>      if (!no_shutdown) {
>>>          qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
>>>                                         !!info, info, &error_abort);
>>> -        qemu_system_shutdown_request();
>>> +        qemu_system_shutdown_request(false);
>> 
>> Panicking is a guest action.  Whether the shutdown on panic should be
>> attributed to guest or host is perhaps debatable.
>
> And it relates to the idea that a guest request for a 'reset' turns into
> a qemu response of 'shutdown'.  After all, whether a guest panic results
> in a shutdown action is determined by command-line arguments to qemu.
> So if we DO want to differentiate between a guest panic and a normal
> guest shutdown, when both events are wired at the command line to cause
> the SHUTDOWN action, then that's another enum to add to my list.
>
>
>>> +++ b/replay/replay.c
>>> @@ -51,7 +51,10 @@ bool replay_next_event_is(int event)
>>>          switch (replay_state.data_kind) {
>>>          case EVENT_SHUTDOWN:
>>>              replay_finish_event();
>>> -            qemu_system_shutdown_request();
>>> +            /* TODO: track source of shutdown request, to replay a
>>> +             * guest-initiated request rather than always claiming to
>>> +             * be from the host? */
>>> +            qemu_system_shutdown_request(false);
>> 
>> This is what your "need a followup patch" refers to.  I'd like to have
>> an opinion from someone familiar with replay on what exactly we need
>> here.
>
> replay-internal.h has an enum ReplayEvents, which is a list of one-byte
> codes used in the replay data stream to record which event to replay. I
> don't know if it is easier to change the replay stream to add a 2-byte
> code (shutdown-with-cause, followed by an encoding of the cause enum),
> or a range of one-byte codes (one new code per number of enum members).
> I also don't know how easy or hard it is to extend the enum (are we free
> to add events in the middle, or are we worried about back-compat to an
> older replay stream that must still play correctly with a newer qemu,
> such that new events must be higher than all existing events).
>
> So yes, I'm hoping for feedback from someone familiar with replay.
>
>> 
>> Amazing number of ways to shut down or reset a machine.
>
> And as I said, it would be easier to submit a patch with less churn by
> having the uncommon case (host-triggered) call a new
> qemu_system_shutdown_request_reason(enum), while the common case
> (guest-triggered) be handled by having qemu_system_shutdown_request()
> with no arguments call
> qemu_system_shutdown_request_reason(SHUTDOWN_GUEST).  I'm just worried
> that doing it that way makes it easy for yet another new host shutdown
> method to use the wrong wrapper.

I don't mind the churn.  It does simplify review.

>> Looks sane on first glance.
>>