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 <brunel@xxxxxxx> > > > > > > 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/) > > > > > > // <smpl> > > > @match_bad_null_test@ > > > expression x, E; > > > statement S1,S2; > > > @@ > > > x = aaci_init_card(...) > > > ... when != x = E > > > * if (x != NULL) > > > S1 else S2 > > > // </smpl> > > > > > > Signed-off-by: Julien Brunel <brunel@xxxxxxx> > > > Signed-off-by: Julia Lawall <julia@xxxxxxx> > > > > 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. 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; } -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
