From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55459) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RG8z7-0003w2-0h for qemu-devel@nongnu.org; Tue, 18 Oct 2011 08:41:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RG8yy-0006Qx-Pa for qemu-devel@nongnu.org; Tue, 18 Oct 2011 08:41:32 -0400 Received: from goliath.siemens.de ([192.35.17.28]:22311) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RG8yy-0006Qg-FO for qemu-devel@nongnu.org; Tue, 18 Oct 2011 08:41:24 -0400 Message-ID: <4E9D73F2.4010009@siemens.com> Date: Tue, 18 Oct 2011 14:41:22 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20111017134349.GD6406@redhat.com> <4E9C7EE3.9050603@web.de> <20111018120549.GH28776@redhat.com> <4E9D6FC1.9040504@siemens.com> <20111018123839.GM28776@redhat.com> In-Reply-To: <20111018123839.GM28776@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alex Williamson , Marcelo Tosatti , Avi Kivity , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" On 2011-10-18 14:38, Michael S. Tsirkin wrote: > On Tue, Oct 18, 2011 at 02:23:29PM +0200, Jan Kiszka wrote: >> On 2011-10-18 14:05, Michael S. Tsirkin wrote: >>> On Mon, Oct 17, 2011 at 09:15:47PM +0200, Jan Kiszka wrote: >>>> On 2011-10-17 15:43, Michael S. Tsirkin wrote: >>>>> On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote: >>>>>> diff --git a/hw/msi.c b/hw/msi.c >>>>>> index 3c7ebc3..9055155 100644 >>>>>> --- a/hw/msi.c >>>>>> +++ b/hw/msi.c >>>>>> @@ -40,6 +40,14 @@ >>>>>> /* Flag for interrupt controller to declare MSI/MSI-X support */ >>>>>> bool msi_supported; >>>>>> >>>>>> +static void msi_unsupported(MSIMessage *msg) >>>>>> +{ >>>>>> + /* If we get here, the board failed to register a delivery handler. */ >>>>>> + abort(); >>>>>> +} >>>>>> + >>>>>> +void (*msi_deliver)(MSIMessage *msg) = msi_unsupported; >>>>>> + >>>>> >>>>> How about we set this to NULL, and check it instead of the bool >>>>> flag? >>>>> >>>> >>>> Yeah. I will introduce >>>> >>>> bool msi_supported(void) >>>> { >>>> return msi_deliver != msi_unsupported; >>>> } >>>> >>>> OK? >>>> >>>> Jan >>>> >>> >>> Looks a bit weird ... >>> NULL is a pretty standard value for an invalid pointer, isn't it? >> >> Save us the runtime check and is equally expressive and readable IMHO. >> >> Jan > > Do we need to check? > NULL dereference leads to a crash just as surely... There is no NULL state of msi_deliver. A) it would execute msi_unsupported if all goes wrong (which will abort) and B) msi_supported() is supposed to protect us in the absence of bugs from ever executing msi_deliver() if it points to msi_unsupported. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux