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

John Ferlan <jferlan(a)redhat.com&gt; [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(a)linux.vnet.ibm.com&gt; > > Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; > > --- > > 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?