[Qemu-devel] [PATCH 0/1] qmp-events: fix GUEST_PANICKED description formatting

[Qemu-devel] [PATCH 0/1] qmp-events: fix GUEST_PANICKED description formatting

[Anton Nefedov]

This patch is a follow-up for:

http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03647.html

[Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting

[Anton Nefedov]

also remove a useless NULL check in the event reporting code

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 qapi/event.json |  4 ++--
 vl.c            | 22 ++++++++++------------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/qapi/event.json b/qapi/event.json
index 970ff02..e02852c 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -488,9 +488,9 @@
 #
 # @action: action that has been taken, currently always "pause"
 #
-# @info: optional information about a panic
+# @info: #optional information about a panic (since 2.9)
 #
-# Since: 1.5 (@info since 2.9)
+# Since: 1.5
 #
 # Example:
 #
diff --git a/vl.c b/vl.c
index 903c46d..f410e03 100644
--- a/vl.c
+++ b/vl.c
@@ -1710,6 +1710,15 @@ void qemu_system_reset(bool report)
 void qemu_system_guest_panicked(GuestPanicInformation *info)
 {
     qemu_log_mask(LOG_GUEST_ERROR, "Guest crashed\n");
+    if (info && info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
+        qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
+                      " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
+                      info->u.hyper_v.data->arg1,
+                      info->u.hyper_v.data->arg2,
+                      info->u.hyper_v.data->arg3,
+                      info->u.hyper_v.data->arg4,
+                      info->u.hyper_v.data->arg5);
+    }
 
     if (current_cpu) {
         current_cpu->crash_occurred = true;
@@ -1723,18 +1732,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
         qemu_system_shutdown_request();
     }
 
-    if (info) {
-        if (info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
-            qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
-                          " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
-                          info->u.hyper_v.data->arg1,
-                          info->u.hyper_v.data->arg2,
-                          info->u.hyper_v.data->arg3,
-                          info->u.hyper_v.data->arg4,
-                          info->u.hyper_v.data->arg5);
-        }
-        qapi_free_GuestPanicInformation(info);
-    }
+    qapi_free_GuestPanicInformation(info);
 }
 
 void qemu_system_reset_request(void)
-- 
2.7.4

Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3538 bytes --]

On 02/16/2017 11:39 AM, Anton Nefedov wrote:
> also remove a useless NULL check in the event reporting code
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  qapi/event.json |  4 ++--
>  vl.c            | 22 ++++++++++------------
>  2 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/qapi/event.json b/qapi/event.json
> index 970ff02..e02852c 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -488,9 +488,9 @@
>  #
>  # @action: action that has been taken, currently always "pause"
>  #
> -# @info: optional information about a panic
> +# @info: #optional information about a panic (since 2.9)
>  #
> -# Since: 1.5 (@info since 2.9)
> +# Since: 1.5

This part's fine, but

> +++ b/vl.c
> @@ -1710,6 +1710,15 @@ void qemu_system_reset(bool report)
>  void qemu_system_guest_panicked(GuestPanicInformation *info)
>  {
>      qemu_log_mask(LOG_GUEST_ERROR, "Guest crashed\n");
> +    if (info && info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
> +                      " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
> +                      info->u.hyper_v.data->arg1,
> +                      info->u.hyper_v.data->arg2,
> +                      info->u.hyper_v.data->arg3,
> +                      info->u.hyper_v.data->arg4,
> +                      info->u.hyper_v.data->arg5);
> +    }

This code motion doesn't seem to match up with the pull request...

>  
>      if (current_cpu) {
>          current_cpu->crash_occurred = true;
> @@ -1723,18 +1732,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
>          qemu_system_shutdown_request();
>      }
>  
> -    if (info) {
> -        if (info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
> -            qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
> -                          " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
> -                          info->u.hyper_v.data->arg1,
> -                          info->u.hyper_v.data->arg2,
> -                          info->u.hyper_v.data->arg3,
> -                          info->u.hyper_v.data->arg4,
> -                          info->u.hyper_v.data->arg5);
> -        }
> -        qapi_free_GuestPanicInformation(info);
> -    }

The pull request for 21/23 just had:

+
+    if (info) {
+        qapi_free_GuestPanicInformation(info);
+    }
 }

where did the qemu_log_mask() stuff come from?  Oh, I see now - it was
in 22/23.

On that grounds, you already need the 'if (info)' for more than just the
free, so this code motion is no longer quite as important.  But now I'm
noticing that it looks weird because you are freeing an input parameter.
 Generally, transfer semantics like that are screwy - it's probably
better if the caller of qemu_system_guest_panicked() is the one freeing
info, rather than requiring that the caller pass in malloc'd memory that
gets freed as a side effect and must not be referenced afterwards in the
caller.  In other words, I think the code motion is unnecessary, but
that the qapi_free_GuestPanicInformation() call is probably in the wrong
function to begin with.

Sorry for not noticing sooner (it shows that I didn't read the full pull
request, but just the one patch that caught my eye because of the .json
edit).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]



On 16/02/2017 20:36, Eric Blake wrote:
> On that grounds, you already need the 'if (info)' for more than just the
> free, so this code motion is no longer quite as important.  But now I'm
> noticing that it looks weird because you are freeing an input parameter.
>  Generally, transfer semantics like that are screwy - it's probably
> better if the caller of qemu_system_guest_panicked() is the one freeing
> info, rather than requiring that the caller pass in malloc'd memory that
> gets freed as a side effect and must not be referenced afterwards in the
> caller.  In other words, I think the code motion is unnecessary, but
> that the qapi_free_GuestPanicInformation() call is probably in the wrong
> function to begin with.

Even better then would be to just pass a CPUState* and let
qemu_system_guest_panicked get the GuestPanicInformation via the QOM
property.

But for 2.9, we only need to change the union.  Eric, can you do that
for us since my QAPI-fu is limited?

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting

[Denis V. Lunev]

On 02/20/2017 07:10 PM, Paolo Bonzini wrote:
>
> On 16/02/2017 20:36, Eric Blake wrote:
>> On that grounds, you already need the 'if (info)' for more than just the
>> free, so this code motion is no longer quite as important.  But now I'm
>> noticing that it looks weird because you are freeing an input parameter.
>>  Generally, transfer semantics like that are screwy - it's probably
>> better if the caller of qemu_system_guest_panicked() is the one freeing
>> info, rather than requiring that the caller pass in malloc'd memory that
>> gets freed as a side effect and must not be referenced afterwards in the
>> caller.  In other words, I think the code motion is unnecessary, but
>> that the qapi_free_GuestPanicInformation() call is probably in the wrong
>> function to begin with.
> Even better then would be to just pass a CPUState* and let
> qemu_system_guest_panicked get the GuestPanicInformation via the QOM
> property.
>
> But for 2.9, we only need to change the union.  Eric, can you do that
> for us since my QAPI-fu is limited?
>
> Paolo
>
give me 5 minutes, I have patches for that, received them today.

Den

Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On 02/20/2017 12:12 PM, Denis V. Lunev wrote:

>> But for 2.9, we only need to change the union.  Eric, can you do that
>> for us since my QAPI-fu is limited?
>>
>> Paolo
>>
> give me 5 minutes, I have patches for that, received them today.

Yep, I've reviewed those patches.  Thanks for the fast followup.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

On 02/16/2017 10:30 AM, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  qapi/event.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/event.json b/qapi/event.json
> index 970ff02..e02852c 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -488,9 +488,9 @@
>  #
>  # @action: action that has been taken, currently always "pause"
>  #
> -# @info: optional information about a panic
> +# @info: #optional information about a panic (since 2.9)
>  #
> -# Since: 1.5 (@info since 2.9)
> +# Since: 1.5

My comment on v4 also pointed out a useless conditional that should be
fixed at the same time:

https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03361.html

And it's probably better to submit this patch as a top-level thread
rather than buried in-reply-to a pull request.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting

[Anton Nefedov]

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 qapi/event.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/event.json b/qapi/event.json
index 970ff02..e02852c 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -488,9 +488,9 @@
 #
 # @action: action that has been taken, currently always "pause"
 #
-# @info: optional information about a panic
+# @info: #optional information about a panic (since 2.9)
 #
-# Since: 1.5 (@info since 2.9)
+# Since: 1.5
 #
 # Example:
 #
-- 
2.7.4