From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752215AbYIANoi (ORCPT ); Mon, 1 Sep 2008 09:44:38 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751362AbYIANo2 (ORCPT ); Mon, 1 Sep 2008 09:44:28 -0400 Received: from ns2.suse.de ([195.135.220.15]:45995 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751344AbYIANo2 (ORCPT ); Mon, 1 Sep 2008 09:44:28 -0400 Date: Mon, 01 Sep 2008 15:44:26 +0200 Message-ID: From: Takashi Iwai To: Julia Lawall Cc: Julien Brunel , perex@perex.cz, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: Re: [PATCH] sound/arm: Bad NULL test In-Reply-To: References: <200809011059.54939.brunel@diku.dk> User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka) FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 Emacs/22.2 (x86_64-suse-linux-gnu) MULE/5.0 (SAKAKI) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Mon, 1 Sep 2008 15:23:25 +0200 (CEST), Julia Lawall wrote: > > On Mon, 1 Sep 2008, Takashi Iwai wrote: > > > At Mon, 1 Sep 2008 14:30:29 +0200 (CEST), > > Julia Lawall wrote: > > > > > > On Mon, 1 Sep 2008, Takashi Iwai wrote: > > > > > > > At Mon, 1 Sep 2008 10:59:54 +0200, > > > > Julien Brunel wrote: > > > > > > > > > > From: Julien Brunel > > > > > > > > > > In case of error, the function aaci_init_card returns an ERR pointer, > > > > > but never returns a NULL pointer. We have noticed a bad NULL test, > > > > > which comes after a call to this function. Rather than doing an IS_ERR > > > > > test, we suggest to duplicate the label out: one label for the case where > > > > > aaci_init_card returns a valid pointer, and another for the case where > > > > > aaci_init_card returns an ERR pointer. > > > > > > > > > > The semantic match that finds this problem is as follows: > > > > > (http://www.emn.fr/x-info/coccinelle/) > > > > > > > > > > // > > > > > @match_bad_null_test@ > > > > > expression x, E; > > > > > statement S1,S2; > > > > > @@ > > > > > x = aaci_init_card(...) > > > > > ... when != x = E > > > > > * if (x != NULL) > > > > > S1 else S2 > > > > > // > > > > > > > > > > Signed-off-by: Julien Brunel > > > > > Signed-off-by: Julia Lawall > > > > > > > > The fix below is simpler. Could you check whether it's OK? > > > > > > It is indeed simpler, and looks correct, but it seems a little odd to take > > > a value that can never be NULL and set it to NULL just to avoid changing a > > > test. Another alternative would be to leave the value as it is, and put > > > an IS_ERR test at the out label. But the value of the test is statically > > > determined by the goto that reaches it, so the original patch proposes > > > just getting rid of the test completely. > > > > OTOH, double labels are pretty ugly and hard to follow. > > Maybe the patch like below is a bit cleaner. > > OK, this seems like a reasonable compromise. The function aaci_init_card > is indeed only used in one place, and -ENOMEM is the only possible error > value. So perhaps it is just as easy to create that where it is called > rather than in aaci_init_card itself. OK, now I applied it to sound git tree. Thanks! Takashi > > julia > > > Takashi > > > > diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c > > index b0a4744..89096e8 100644 > > --- a/sound/arm/aaci.c > > +++ b/sound/arm/aaci.c > > @@ -999,7 +999,7 @@ static struct aaci * __devinit aaci_init_card(struct amba_device *dev) > > card = snd_card_new(SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, > > THIS_MODULE, sizeof(struct aaci)); > > if (card == NULL) > > - return ERR_PTR(-ENOMEM); > > + return NULL; > > > > card->private_free = aaci_free_card; > > > > @@ -1083,8 +1083,8 @@ static int __devinit aaci_probe(struct amba_device *dev, void *id) > > return ret; > > > > aaci = aaci_init_card(dev); > > - if (IS_ERR(aaci)) { > > - ret = PTR_ERR(aaci); > > + if (!aaci) { > > + ret = -ENOMEM; > > goto out; > > } > > > > >