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
