From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754445AbYIANXu (ORCPT ); Mon, 1 Sep 2008 09:23:50 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751405AbYIANXj (ORCPT ); Mon, 1 Sep 2008 09:23:39 -0400 Received: from mgw2.diku.dk ([130.225.96.92]:52906 "EHLO mgw2.diku.dk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751099AbYIANXh (ORCPT ); Mon, 1 Sep 2008 09:23:37 -0400 Date: Mon, 1 Sep 2008 15:23:25 +0200 (CEST) From: Julia Lawall To: Takashi Iwai 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: Message-ID: References: <200809011059.54939.brunel@diku.dk> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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; > } > >