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')


Bjoern Walk (9):
  conf: add info to virDomainStateReason
  conf: set/retrieve state information
  lib: introduce virDomainGetStateParams function
  remote: implement remoteDomainGetStateParams
  qemu: implement qemuDomainGetStateParams
  qemu: set state information for guest panic event
  virsh: domstate: report detailed state if available
  news: add entry for virDomainGetStateParams
  qemu: fix order of S390 panic event information

 docs/news.xml                       | 11 ++++++
 include/libvirt/libvirt-domain.h    | 24 ++++++++++++
 src/conf/domain_conf.c              | 16 +++++++-
 src/conf/domain_conf.h              |  7 ++++
 src/driver-hypervisor.h             |  7 ++++
 src/libvirt-domain.c                | 56 ++++++++++++++++++++++++++
 src/libvirt_private.syms            |  1 +
 src/libvirt_public.syms             |  5 +++
 src/qemu/qemu_driver.c              | 61 ++++++++++++++++++++++++++++-
 src/qemu/qemu_monitor.c             | 39 ++++++++++++++++--
 src/qemu/qemu_monitor.h             |  1 +
 src/remote/remote_daemon_dispatch.c |  1 -
 src/remote/remote_driver.c          |  1 +
 src/remote/remote_protocol.x        | 20 +++++++++-
 src/remote_protocol-structs         | 11 ++++++
 tools/virsh-domain-monitor.c        | 31 ++++++++++++---
 tools/virsh.pod                     |  5 ++-
 17 files changed, 281 insertions(+), 16 deletions(-)

-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

    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

    int
    virDomainGetStateParams(virDomainPtr domain,
                            int *state,
                            int *reason,
                            virTypedParameterPtr *params,
                            int *nparams,
                            unsigned int flags)

I'd still keep the state and reason parameters directly to make the API
easier to use.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Jiri Denemark <jdenemar@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.

>     int
>     virDomainGetStateParams(virDomainPtr domain,
>                             int *state,
>                             int *reason,
>                             virTypedParameterPtr *params,
>                             int *nparams,
>                             unsigned int flags)
> 
> I'd still keep the state and reason parameters directly to make the API
> easier to use.
> 
> Jirka
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

-- 
IBM Systems
Linux on Z & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
------------------------------------------------------------------------
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Jul 11, 2018 at 01:40:31PM +0200, Bjoern Walk wrote:
>Jiri Denemark <jdenemar@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.--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Martin Kletzander <mkletzan@redhat.com> [2018-07-13, 10:50AM +0200]:
> 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.

Ok, I can see the appeal for this. I agree that parsing the string is
not optimal, it just didn't occur to me that we maybe want to have this
information in other places as well.

So then I would model the new API function like Jiri suggested:

    int
    virDomainGetStateParams(virDomainPtr domain,
                            int *state,
                            int *reason,
                            virTypedParameterPtr *params,
                            int *nparams,
                            unsigned int flags)

where each parameter in params holds a piece of the additional
information, i.e VIR_DOMAIN_STATE_PARAMS_REASON_PSW_ADDR and so on on
s390x.

However, this raises the question how this would be handled on the
client, who now receives an unknown set of parameters. How would for
example virsh go on and reconstruct the info string? Just iterate of all
returned parameters? Or should it introspect the parameters and perform
an explicit formatting?

> 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.

Yeah, in my QEMU-centric view I just assumed that this was an option.
For where this information is saved, if I understand you correctly, I am
not sure if I want to clutter this into the domain object where the
state and reasons (and, in this patch series, the info) is saved. I try
to find a way to retrieve this information from the hypervisor on the
fly.

I don't expect to much changes for the backend, so a general review of
the code if I am heading in the right direction.would be appreciated.

> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


-- 
IBM Systems
Linux on Z & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
------------------------------------------------------------------------
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list