[PATCH] sound/arm: Bad NULL test

[PATCH] sound/arm: Bad NULL test

[Julien Brunel]

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

Re: [PATCH] sound/arm: Bad NULL test

[Takashi Iwai]

At Mon, 1 Sep 2008 10:59:54 +0200,
Julien Brunel wrote:
> 
> 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>

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

Re: [PATCH] sound/arm: Bad NULL test

[Julia Lawall]

On Mon, 1 Sep 2008, Takashi Iwai wrote:

> At Mon, 1 Sep 2008 10:59:54 +0200,
> Julien Brunel wrote:
> > 
> > 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>
> 
> 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.

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

julia


> 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;
>  	}
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] sound/arm: Bad NULL test

[Takashi Iwai]

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

Re: [PATCH] sound/arm: Bad NULL test

[Julia Lawall]

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

Re: [PATCH] sound/arm: Bad NULL test

[Takashi Iwai]

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