From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avhPb-0006Cf-6k for qemu-devel@nongnu.org; Thu, 28 Apr 2016 04:35:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avhPX-0003xp-Vm for qemu-devel@nongnu.org; Thu, 28 Apr 2016 04:35:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34456) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avhPX-0003xj-Qd for qemu-devel@nongnu.org; Thu, 28 Apr 2016 04:34:59 -0400 From: Markus Armbruster References: <1460689374-9690-1-git-send-email-saxenap.ltc@gmail.com> <1460689374-9690-3-git-send-email-saxenap.ltc@gmail.com> <5720E204.1090101@redhat.com> Date: Thu, 28 Apr 2016 10:34:55 +0200 In-Reply-To: <5720E204.1090101@redhat.com> (Eric Blake's message of "Wed, 27 Apr 2016 10:00:04 -0600") Message-ID: <87oa8ut9lc.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/2] Debug : Add error messages before a call to debug(). List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Prerna Saxena , qemu-devel@nongnu.org Eric Blake 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.