[libvirt] [PATCH libvirt] qemu: log the crash information for S390

[Jiri Denemark]

On Mon, Feb 26, 2018 at 08:25:11 +0100, Bjoern Walk wrote:
> John Ferlan <jferlan at redhat.com> [2018-02-20, 12:25PM -0500]:
> > On 02/15/2018 06:58 AM, Bjoern Walk wrote:
> > > Since QEMU 2.12 guest crash information for S390 is available in the
> > > QEMU monitor, e.g.:
> > > 
> > >   {
> > >     "timestamp": {
> > >         "seconds": 1518004739,
> > >         "microseconds": 552563
> > >     },
> > >     "event": "GUEST_PANICKED",
> > >     "data": {
> > >         "action": "pause",
> > >         "info": {
> > >             "core": 0,
> > >             "psw-addr": 1102832,
> > >             "reason": "disabled-wait",
> > >             "psw-mask": 562956395872256,
> > >             "type": "s390"
> > >         }
> > >     }
> > >   }
> > > 
> > > Let's log this information into the domain log file, e.g.:
> > > 
> > >     2018-02-08 13:11:26.075+0000: panic s390: core='0' psw-mask='0x0002000180000000' psw-addr='0x000000000010f146' reason='disabled-wait'
> > > 
> > > Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> > > Signed-off-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
> > > ---
> > >  src/qemu/qemu_monitor.c      | 19 ++++++++++++++++++-
> > >  src/qemu/qemu_monitor.h      | 12 ++++++++++++
> > >  src/qemu/qemu_monitor_json.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 70 insertions(+), 1 deletion(-)
> > > 
> > 
> > Not sure the patch :
> > 
> > http://lists.nongnu.org/archive/html/qemu-devel/2018-02/msg02580.html
> > 
> > has quite made it to git master yet...  At least the pull I just didn't
> > have it...
> 
> Yeah, I sent it early because of vacation.
> 
> > In any case, is this news.xml worthy?
> 
> I don't know, probably. Still haven't figured out when to write an entry
> there.
> 
> > 
> > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > index 242b92ea..5c1f6836 100644
> > > --- a/src/qemu/qemu_monitor_json.c
> > > +++ b/src/qemu/qemu_monitor_json.c
> > > @@ -576,6 +576,44 @@ qemuMonitorJSONGuestPanicExtractInfoHyperv(virJSONValuePtr data)
> > >      return NULL;
> > >  }
> > >  
> > > +static qemuMonitorEventPanicInfoPtr
> > > +qemuMonitorJSONGuestPanicExtractInfoS390(virJSONValuePtr data)
> > > +{
> > > +    qemuMonitorEventPanicInfoPtr ret;
> > > +    int core;
> > > +    unsigned long long psw_mask, psw_addr;
> > > +    const char *reason = NULL;
> > > +
> > > +    if (VIR_ALLOC(ret) < 0)
> > > +        return NULL;
> > > +
> > > +    ret->type = QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390;
> > > +
> > > +    if (virJSONValueObjectGetNumberInt(data, "core", &core) < 0 ||
> > > +        virJSONValueObjectGetNumberUlong(data, "psw-mask", &psw_mask) < 0 ||
> > > +        virJSONValueObjectGetNumberUlong(data, "psw-addr", &psw_addr) < 0) {
> > > +        goto error;
> > > +    }
> > > +
> > > +    ret->data.s390.core = core;
> > > +    ret->data.s390.psw_mask = psw_mask;
> > > +    ret->data.s390.psw_addr = psw_addr;
> > > +
> > > +    reason = virJSONValueObjectGetString(data, "reason");
> > 
> > Probably could have gone with including this into the above condition,
> > 
> >    !(reason = virJSONValueObjectGetString(data, "reason"))
> > 
> > so that the error could be put in there to avoid the no_memory label..
> > 
> > This isn't a necessary change, but a "could be done" change if so desired.
> 
> I imagine having read somewhere here that we do want do discourage those
> assign-conditionals. But still, yes, this can be reordered to make it
> more compact and readable. I however don't see how we avoid the
> no_memory label, since we do the VIR_STRDUP, which is fundamentally
> different from the error when the QMP response is different. Any
> pointers?

Sure. The essential part is moving the virReportError(s) closer to the
condition which raise them and jump to the "error" label with the error
already set. In general, we don't use dedicated labels for each error,
we just report the error and jump to a common error label.

In other words:

    if (virJSON...() < 0 || ...) {
        virReportError("oops");
        goto error;
    }

    if (somethingElse < 0) {
        virReportError("oops");
        goto error;
    }

    if (VIR_STRDUP() < 0)
        goto error;

Jirka