Re: [Qemu-devel] [PATCH 2/2] Debug : Add error messages before a call to debug().

[Prerna <saxenap.ltc@gmail.com>]

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

Hi Eric,
Thank you for the review.

On Wed, Apr 27, 2016 at 9:30 PM, Eric Blake <eblake@redhat.com> wrote:

> On 04/14/2016 09:02 PM, Prerna Saxena wrote:
> > Qemu code has abort() calls in various places which raises a SIGABRT;
> > This patch adds error messages before (most)calls to abort(), so that
> > it is easier to determine why QEMU died.
>
> The subject line says you are adding messages before debug(), but the
> rest of the patch is adding message before abort(). You'll need to fix
> that.  Also, subject lines usually don't end in '.'
>

Sorry, the subject line shouldve said : "Add error messages before abort()".
Will resend.


>
> > +++ b/block.c
> > @@ -3725,6 +3725,7 @@ void
> bdrv_remove_aio_context_notifier(BlockDriverState *bs,
> >          }
> >      }
> >
> > +    error_report("Matching context notifier not found for removal.
> Aborting");
> >      abort();
>
> The "Aborting" part of the message is redundant; it's pretty obvious
> that qemu aborted.
>
>
Agree.


> I also wonder if you should be using g_assert_not_reached() instead of
> abort() in some (all?) of the places touched in this patch - at which
> point you don't have to worry about inventing a message that will never
> be printed.  The reason I suggest it is that g_assert_not_reached() is
> self-documenting, and prints a nicer message than abort() if it does
> accidentally get reached.
>
>
Ah, thanks for this suggestion. I will look up g_assert_not_reached() to
see how I can use it (at all places, possibly).

Regards,
Prerna


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

[-- Attachment #2: Type: text/html, Size: 2855 bytes --]