| Message ID | 1487266760-80500-2-git-send-email-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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)
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(-)