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.nefe...@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
signature.asc
Description: OpenPGP digital signature
