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
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
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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
© 2016 - 2024 Red Hat, Inc.