From: Julien Brunel <brunel@diku.dk>

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@diku.dk>
Signed-off-by:  Julia Lawall <julia@diku.dk>

---
 sound/arm/aaci.c |    13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff -u -p a/sound/arm/aaci.c b/sound/arm/aaci.c
--- a/sound/arm/aaci.c
+++ b/sound/arm/aaci.c
@@ -1091,7 +1091,7 @@ static int __devinit aaci_probe(struct a
 	aaci->base = ioremap(dev->res.start, SZ_4K);
 	if (!aaci->base) {
 		ret = -ENOMEM;
-		goto out;
+		goto out_free_aaci;
 	}
 
 	/*
@@ -1119,7 +1119,7 @@ static int __devinit aaci_probe(struct a
 
 	ret = aaci_probe_ac97(aaci);
 	if (ret)
-		goto out;
+		goto out_free_aaci;
 
 	/*
 	 * Size the FIFOs (must be multiple of 16).
@@ -1129,12 +1129,12 @@ static int __devinit aaci_probe(struct a
 		printk(KERN_WARNING "AACI: fifosize = %d not supported\n",
 		       aaci->fifosize);
 		ret = -ENODEV;
-		goto out;
+		goto out_free_aaci;
 	}
 
 	ret = aaci_init_pcm(aaci);
 	if (ret)
-		goto out;
+		goto out_free_aaci;
 
 	snd_card_set_dev(aaci->card, &dev->dev);
 
@@ -1145,10 +1145,9 @@ static int __devinit aaci_probe(struct a
 		amba_set_drvdata(dev, aaci->card);
 		return ret;
 	}
-
+ out_free_aaci:
+	snd_card_free(aaci->card);
  out:
-	if (aaci)
-		snd_card_free(aaci->card);
 	amba_release_regions(dev);
 	return ret;
 }
--

[ message continues ]

At Mon, 1 Sep 2008 10:59:54 +0200,

The fix below is simpler.  Could you check whether it's OK?

thanks,

Takashi


diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
index b0a4744..e46b7cb 100644
--- a/sound/arm/aaci.c
+++ b/sound/arm/aaci.c
@@ -1085,6 +1085,7 @@ static int __devinit aaci_probe(struct amba_device *dev, void *id)
 	aaci = aaci_init_card(dev);
 	if (IS_ERR(aaci)) {
 		ret = PTR_ERR(aaci);
+		aaci = NULL;
 		goto out;
 	}
 
--

[ message continues ]

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.

I guess it depends on which sort of solution is preferred.

--

[ message continues ]

At Mon, 1 Sep 2008 14:30:29 +0200 (CEST),

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;
 	}
 
--

[ message continues ]

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.

--

[ message continues ]

At Mon, 1 Sep 2008 15:23:25 +0200 (CEST),

OK, now I applied it to sound git tree.

Thanks!

--

[ message continues ]