Eric Blake <eblake@xxxxxxxxxx> 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.
>>
