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

[Alistair Francis <alistair.francis@xilinx.com>]

On Thu, Apr 20, 2017 at 12:05 PM, Eric Blake <eblake@redhat.com> wrote:
> On 04/20/2017 11:18 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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.
>
> Or even this idea:
>
> { 'enum': 'ShutdownCause', 'data': [ 'shutdown', 'reset', 'panic' ] }
> { 'event': 'SHUTDOWN',
>   'data': { 'guest': 'bool', '*cause': 'ShutdownCause' } }
>
> where the enum can grow as we come up with ever more reasons worth
> exposing (maybe even 'qmp', 'gui' and 'interrupt' are reasonable causes
> for a host shutdown).  Our promise would be that 'guest' never changes
> for an existing shutdown reason, but that 'cause' may become more
> refined over time if someone expresses a need for having the distinction.
>
> Thoughts?

I'm not a fan of the 'guest' bool. I do see that it helps with
maintaining backwards compatibility but I think we would be better off
just getting the reasons right in the first place.

What about something that can grow in the future? We start with a
general guest shutdown that is always there and then as we add new
reasons things can be moved to use the new method or continue to use
the general one.

SHUTDOWN_HOST
SHUTDOWN_HOST_GUI
/* This is always a backwards compatible fall-back
 * Maybe this could be SHUTDOWN_GUEST_UNKNOWN instead?
 */
SHUTDOWN_GUEST_GENERAL
SHUTDOWN_GUEST_HALT
SHUTDOWN_GUEST_RESET
...

That way we can guarantee the base coverage but still expand more
specific reasons in the future.

I guess the only problem is that then the reasons aren't always
reliable then as we could introduce a new reason and something gets
stuck using the general fall back.

Thanks,

Alistair

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