Hi Eric,
Thank you for the review.
On Wed, Apr 27, 2016 at 9:30 PM, Eric Blake <ebl...@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
>
>
