Re: [qemu-s390x] [PATCH v5 1/1] s390x/cpu: expose the guest crash information

[Eric Blake]

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