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

[Eric Blake <eblake@xxxxxxxxxx>]

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.


>> +++ 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.

> 
> Looks sane on first glance.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel