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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> 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 '.'
>
>> +++ 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.
>
> 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.

Printing elaborate messages before killing the program on programming
errors seems pointless.  These errors should never happen.  If they do,
something's seriously wrong with the program, and you need professional
help to debug it.  Pretty much the only generally useful clue the dying
program can provide for that is where the error is.  abort() provides
that clue (and more) if you let it dump core.  assert() additionally
provides that clue independently of the core.  If you find users need
more than that, chances are this error shouldn't abort() in the first
place.