From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54637) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a71x9-0005zS-Sw for qemu-devel@nongnu.org; Thu, 10 Dec 2015 09:12:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a71x4-0008Sq-TK for qemu-devel@nongnu.org; Thu, 10 Dec 2015 09:12:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38096) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a71x4-0008Sj-On for qemu-devel@nongnu.org; Thu, 10 Dec 2015 09:12:10 -0500 References: <1449743372-17169-1-git-send-email-armbru@redhat.com> <1449743372-17169-11-git-send-email-armbru@redhat.com> <5669691D.8040403@gmail.com> <87poyee9hv.fsf@blackfin.pond.sub.org> From: Marcel Apfelbaum Message-ID: <56698835.4000107@redhat.com> Date: Thu, 10 Dec 2015 16:12:05 +0200 MIME-Version: 1.0 In-Reply-To: <87poyee9hv.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 10/12] isa: Clean up inappropriate hw_error() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Marcel Apfelbaum Cc: "Michael S. Tsirkin" , Mark Cave-Ayland , qemu-devel@nongnu.org, =?UTF-8?Q?Herv=c3=a9_Poussineau?= , Aurelien Jarno , Richard Henderson On 12/10/2015 03:09 PM, Markus Armbruster wrote: > Marcel Apfelbaum writes: > >> On 12/10/2015 12:29 PM, Markus Armbruster wrote: >>> isa_bus_irqs(), isa_create() and isa_try_create() call hw_error() when >>> passed a null bus. Use of hw_error() has always been questionable, >>> because these are used only during machine initialization, and >>> printing CPU registers isn't useful there. >>> >>> Since the previous commit, passing a null bus is a programming error. >>> Drop the hw_error() and simply let it crash. >> >> Maybe we can be a little nicer add an assert ? :) > > assert(p) before dereferencing p only converts one kind of crash into > another one. I tend to do it only when the assert(p) does double-duty > as useful documentation. Or perhaps when I think there's a real risk of > running into !p in an environment where core dumps are off[*] and > reproducing the failure with a debugger attached could be hard. > > To use these three functions, you need an ISABus *. How could you end > up with a bad one? > > * You forget to create the ISA bus, and the compiler is too confused to > notice. You'll pass an unitialized ISABus, and asserting it's not > null is unlikely to help. > > * You create multiple ISA buses (that's the only way creating one can > fail) *and* forget to check for errors. If you pull that off, I'd > expect it to explode even in light testing. This is the scenario I was referring to. You are perfectly right that using assert will change a crash into another, but when I am "succeeding" to crash some code (and I am good at it :)), if I see a NULL pointer de-reference I am starting to wonder if *is possible* to have a NULL pointer there and the developer didn't take that into account. (it couldn't me my bug, right, it got be his :)) However, if I see an assert crash *I know* the developer did not expect a NULL pointer to be passed at all. For this specific scenario, (multiple ISA buses) maybe is such a strange case that we don't need to bother with an assert. Thanks for the detailed explanation, Marcel > > * Your pointer gets corrupted between correct initialization and use. > Asserting it's not null is unlikely to help. > > > [*] Switching them off on a development machine forfeits your > developer's license, as far as I'm concerned :) >