1. Patchew
  2. Libvirt
  3. View series

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

Bjoern Walk posted 1 patch 6 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180215115817.249749-1-bwalk@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(-)
[libvirt] [PATCH libvirt] qemu: log the crash information for S390
Posted by Bjoern Walk 6 years, 2 months ago 
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@linux.vnet.ibm.com>
Signed-off-by: Bjoern Walk <bwalk@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(-)

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");
+    if (!reason)
+        goto error;
+
+    if (VIR_STRDUP(ret->data.s390.reason, reason) < 0)
+        goto no_memory;
+
+    return ret;
+
+ error:
+    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed s390 panic data"));
+ no_memory:
+    qemuMonitorEventPanicInfoFree(ret);
+    return NULL;
+}
 
 static qemuMonitorEventPanicInfoPtr
 qemuMonitorJSONGuestPanicExtractInfo(virJSONValuePtr data)
@@ -584,6 +622,8 @@ qemuMonitorJSONGuestPanicExtractInfo(virJSONValuePtr data)
 
     if (STREQ_NULLABLE(type, "hyper-v"))
         return qemuMonitorJSONGuestPanicExtractInfoHyperv(data);
+    else if (STREQ_NULLABLE(type, "s390"))
+        return qemuMonitorJSONGuestPanicExtractInfoS390(data);
 
     virReportError(VIR_ERR_INTERNAL_ERROR,
                    _("unknown panic info type '%s'"), NULLSTR(type));
-- 
2.13.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH libvirt] qemu: log the crash information for S390
Posted by John Ferlan 6 years, 1 month ago 

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@linux.vnet.ibm.com>
> Signed-off-by: Bjoern Walk <bwalk@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...

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.

Need to wait until patch shows up in qemu master, but otherwise, things
seem fine for what I see...

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

> +    if (!reason)
> +        goto error;
> +
> +    if (VIR_STRDUP(ret->data.s390.reason, reason) < 0)
> +        goto no_memory;
> +
> +    return ret;
> +
> + error:
> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed s390 panic data"));
> + no_memory:
> +    qemuMonitorEventPanicInfoFree(ret);
> +    return NULL;
> +}
>  
>  static qemuMonitorEventPanicInfoPtr
>  qemuMonitorJSONGuestPanicExtractInfo(virJSONValuePtr data)
> @@ -584,6 +622,8 @@ qemuMonitorJSONGuestPanicExtractInfo(virJSONValuePtr data)
>  
>      if (STREQ_NULLABLE(type, "hyper-v"))
>          return qemuMonitorJSONGuestPanicExtractInfoHyperv(data);
> +    else if (STREQ_NULLABLE(type, "s390"))
> +        return qemuMonitorJSONGuestPanicExtractInfoS390(data);
>  
>      virReportError(VIR_ERR_INTERNAL_ERROR,
>                     _("unknown panic info type '%s'"), NULLSTR(type));
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH libvirt] qemu: log the crash information for S390
Posted by Jiri Denemark 6 years, 1 month ago 
On Tue, Feb 20, 2018 at 12:25:46 -0500, John Ferlan wrote:
> 
> 
> 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@linux.vnet.ibm.com>
> > Signed-off-by: Bjoern Walk <bwalk@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...
> 
> 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.

I think it should actually be changed that way so that the error message
is close to the check rather than being hidden in the error label.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH libvirt] qemu: log the crash information for S390
Posted by Bjoern Walk 6 years, 1 month ago 
John Ferlan <jferlan@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@linux.vnet.ibm.com>
> > Signed-off-by: Bjoern Walk <bwalk@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?

> Need to wait until patch shows up in qemu master, but otherwise, things
> seem fine for what I see...
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>

Thanks. While we wait for QEMU I can spin up a v2 with the news.xml and
the above.

-- 
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
Re: [libvirt] [PATCH libvirt] qemu: log the crash information for S390
Posted by Jiri Denemark 6 years, 1 month ago 
On Mon, Feb 26, 2018 at 08:25:11 +0100, Bjoern Walk wrote:
> John Ferlan <jferlan@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@linux.vnet.ibm.com>
> > > Signed-off-by: Bjoern Walk <bwalk@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

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.