* [Qemu-devel] [PATCH 0/1] qmp-events: fix GUEST_PANICKED description formatting @ 2017-02-16 17:39 Anton Nefedov 2017-02-16 17:39 ` [Qemu-devel] [PATCH] " Anton Nefedov 0 siblings, 1 reply; 6+ messages in thread From: Anton Nefedov @ 2017-02-16 17:39 UTC (permalink / raw) To: qemu-devel; +Cc: eblake, pbonzini, armbru, den, anton.nefedov This patch is a follow-up for: http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03647.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting 2017-02-16 17:39 [Qemu-devel] [PATCH 0/1] qmp-events: fix GUEST_PANICKED description formatting Anton Nefedov @ 2017-02-16 17:39 ` Anton Nefedov 2017-02-16 19:36 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Anton Nefedov @ 2017-02-16 17:39 UTC (permalink / raw) To: qemu-devel; +Cc: eblake, pbonzini, armbru, den, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting 2017-02-16 17:39 ` [Qemu-devel] [PATCH] " Anton Nefedov @ 2017-02-16 19:36 ` Eric Blake 2017-02-20 18:10 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2017-02-16 19:36 UTC (permalink / raw) To: Anton Nefedov, qemu-devel; +Cc: pbonzini, armbru, den [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting 2017-02-16 19:36 ` Eric Blake @ 2017-02-20 18:10 ` Paolo Bonzini 2017-02-20 18:12 ` Denis V. Lunev 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2017-02-20 18:10 UTC (permalink / raw) To: Eric Blake, Anton Nefedov, qemu-devel; +Cc: armbru, den [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting 2017-02-20 18:10 ` Paolo Bonzini @ 2017-02-20 18:12 ` Denis V. Lunev 2017-02-20 19:49 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Denis V. Lunev @ 2017-02-20 18:12 UTC (permalink / raw) To: Paolo Bonzini, Eric Blake, Anton Nefedov, qemu-devel; +Cc: armbru 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp-events: fix GUEST_PANICKED description formatting 2017-02-20 18:12 ` Denis V. Lunev @ 2017-02-20 19:49 ` Eric Blake 0 siblings, 0 replies; 6+ messages in thread From: Eric Blake @ 2017-02-20 19:49 UTC (permalink / raw) To: Denis V. Lunev, Paolo Bonzini, Anton Nefedov, qemu-devel; +Cc: armbru [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-20 19:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-16 17:39 [Qemu-devel] [PATCH 0/1] qmp-events: fix GUEST_PANICKED description formatting Anton Nefedov 2017-02-16 17:39 ` [Qemu-devel] [PATCH] " Anton Nefedov 2017-02-16 19:36 ` Eric Blake 2017-02-20 18:10 ` Paolo Bonzini 2017-02-20 18:12 ` Denis V. Lunev 2017-02-20 19:49 ` Eric Blake
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).