[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN
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. >> _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |