[libvirt] [PATCH 0/9] extend virsh domstate to show additional information

[Martin Kletzander]

On Wed, Jul 11, 2018 at 01:40:31PM +0200, Bjoern Walk wrote:
>Jiri Denemark <jdenemar at redhat.com> [2018-07-11, 01:17PM +0200]:
>> On Wed, Jul 11, 2018 at 12:49:13 +0200, Bjoern Walk wrote:
>> > This patch series introduces the ability to save additional information
>> > for the domain state and exposes this information in virsh domstate.
>> >
>> > For example in the case of QEMU guest panic events, we can provide additional
>> > information like the crash reason or register state of the domain. This
>> > information usually gets logged in the domain log but for debugging it is
>> > useful to have it accessible from the client. Therefore, let's introduce a new
>> > public API function, virDomainGetStateParams, an extensible version of
>> > virDomainGetState, which returns the complete state of the domain, including
>> > newly introduced additional information.
>> >
>> > Let's also extend virsh domstate and introduce a new parameter --info to show
>> > the domain state, reason and additional information when available.
>> >
>> >     virsh # domstate --info guest-1
>> >     crashed (panicked: disabled-wait core='1' psw-mask='0x000000000010f146' \
>> >          psw-addr='0x0002000180000000')
>>
>> I was thinking you introduced a new API with typed parameters so that
>> you can return each part of the info string as a separate parameter with
>> a defined semantics. But I was wrong, you're just adding a new opaque
>> string with more details about the reason. In that case typed parameters
>> don't actually bring any additional value, they just complicate the
>> usage of the API. The following prototype would be much better
>
>The extensibility for this new API was more regarding for future
>additions of any state related information. Since libvirt does not have
>a deprecation model for APIs, whenever we would want to return
>additional information for the state (like in this case) a new API
>function has to be created.
>
>>     int
>>     virDomainGetState...(virDomainPtr domain,
>>                          int *state,
>>                          int *reason,
>>                          char **info,
>>                          unsigned int flags)
>>
>> On the other hand, is an opaque string really a good idea? It makes the
>> additional info usable only for being shown to the user rather than
>> being processed by an upper management layer. That's probably fine for
>> crashed state, but perhaps other states would want to return something
>> that is supposed to be processed. Maybe I'm just overthinking this, but
>> I'd like to avoid having to add yet another API later. So the API could
>> have the following prototype
>
>Sure, if machine readability is a goal this approach makes certainly
>more sense. On the other hand, the same information can be queried by a
>qemu-monitor-command call and retrieved in JSON format. This information
>here is aimed at human interaction, similar to the log output. It is
>also unclear which platforms/hypervisors would provide what information
>and mapping all of them to a virTypedParameter entry could result in a
>rather large list.
>
>Certainly no arguments against your objection, and if the overall
>concensus is that this is something we want, I can definitely rework the
>API.
>

I'm not very particularly opinionated on this, but I think APIs should be
machine-readable and CLI tools can convert that to human-readable format.
You'll never know when a program will like to access that and having to tell
anyone in the future that they need to parse a string is ugly IMHO.

Also from the monitor you can get that information only if there is any QEMU
running.  I presume the state you are returning is saved somewhere along with
the reason so that it can be provided later.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180713/54323241/attachment-0001.sig>