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... In any case, is this news.xml worthy? > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 9b5ad72c..8c394583 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -4363,7 +4363,14 @@ qemuMonitorGuestPanicEventInfoFormatMsg(qemuMonitorEventPanicInfoPtr info) > info->data.hyperv.arg3, info->data.hyperv.arg4, > info->data.hyperv.arg5)); > break; > - > + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390: > + ignore_value(virAsprintf(&ret, "s390: core='%d' psw-mask='0x%016llx' " > + "psw-addr='0x%016llx' reason='%s'", > + info->data.s390.core, > + info->data.s390.psw_mask, > + info->data.s390.psw_addr, > + info->data.s390.reason)); > + break; > case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_NONE: > case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_LAST: > break; > @@ -4379,6 +4386,16 @@ qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info) > if (!info) > return; > > + switch (info->type) { > + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390: > + VIR_FREE(info->data.s390.reason); > + break; > + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_NONE: > + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_HYPERV: > + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_LAST: > + break; > + } > + > VIR_FREE(info); > } > > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index ea0c01ae..4b3a5e21 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -73,6 +73,7 @@ struct _qemuMonitorMessage { > typedef enum { > QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_NONE = 0, > QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_HYPERV, > + QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390, > > QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_LAST > } qemuMonitorEventPanicInfoType; > @@ -88,12 +89,23 @@ struct _qemuMonitorEventPanicInfoHyperv { > unsigned long long arg5; > }; > > +typedef struct _qemuMonitorEventPanicInfoS390 qemuMonitorEventPanicInfoS390; > +typedef qemuMonitorEventPanicInfoS390 *qemuMonitorEventPanicInfoS390Ptr; > +struct _qemuMonitorEventPanicInfoS390 { > + /* S390 specific guest panic information */ > + int core; > + unsigned long long psw_mask; > + unsigned long long psw_addr; > + char *reason; > +}; > + > typedef struct _qemuMonitorEventPanicInfo qemuMonitorEventPanicInfo; > typedef qemuMonitorEventPanicInfo *qemuMonitorEventPanicInfoPtr; > struct _qemuMonitorEventPanicInfo { > qemuMonitorEventPanicInfoType type; > union { > qemuMonitorEventPanicInfoHyperv hyperv; > + qemuMonitorEventPanicInfoS390 s390; > } data; > }; > > 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.

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?