On 02/06/2018 12:21 PM, Christian Borntraeger wrote:
+ CRASH_REASON_UNKNOWN, /* default value of 0 on reset */
+ CRASH_REASON_PGM,
+ CRASH_REASON_EXT,
+ CRASH_REASON_WAITPSW,
+ CRASH_REASON_OPEREXC,
...you have an internal enum for decoding some of those integer values into
specific human readable strings, but don't expose the enum over QAPI. Are we
sure that's the interface we want to go with? As long as you are okay with
that, then I can live with the interface change; I just want to make sure that
you are certain that the machine-based consumer of the QMP command does not
need to decode crash_reasons because you left it as an internal enum.
We might want to have temporary or intermediate crash reasons (e.g. emulation
failure or whatever),
so not requiring changes in both components might be less error prone. (the way
it is today)
But if there is a strong wish for an on-the-wire enum, we could do that as well.
I don't have a strong wish for an on-the-wire enum, so much as a request
to at least document in the commit message why you decided one was not
needed at this time. And in all reality, would you really have to keep
two different enums in sync, or if you expose something over the wire,
can't that just be your only enum type? If a temporary or intermediate
crash reason is useful enough to give a different string to the human
reader, why would it not be useful as also exposing as a different enum?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
