sound/arm: Bad NULL test

From: Julien Brunel <[email protected]>

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 <[email protected]>

Signed-off-by Julia Lawall <[email protected]>

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

Quoted 24 lines · Julien Brunel

From: Julien Brunel <[email protected]>

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>

Quoted 2 lines

Signed-off-by Julien Brunel <[email protected]>

Signed-off-by Julia Lawall <[email protected]>

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

thanks,

Takashi

sound/arm/aaci.c +1 -1

index b0a4744..e46b7cb 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 29 lines · Takashi Iwai

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

Quoted 26 lines · Julien Brunel

From: Julien Brunel <[email protected]>

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 <[email protected]>

Signed-off-by Julia Lawall <[email protected]>

Quoted 1 line

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.

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

Quoted 32 lines · Julia Lawall

Quoted 29 lines · Takashi Iwai

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

Quoted 26 lines · Julien Brunel

From: Julien Brunel <[email protected]>

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 <[email protected]>

Signed-off-by Julia Lawall <[email protected]>

Quoted 8 lines

Quoted 1 line

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

sound/arm/aaci.c +3 -3

index b0a4744..89096e8 100644
@@ -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;
 	}
 

Quoted 43 lines · Takashi Iwai

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

Quoted 40 lines · Julia Lawall

Quoted 26 lines · Takashi Iwai

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

Quoted 24 lines · Julien Brunel

From: Julien Brunel <[email protected]>

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>

Quoted 4 lines

Quoted 2 lines

Signed-off-by Julien Brunel <[email protected]>

Signed-off-by Julia Lawall <[email protected]>

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.

Quoted 2 lines

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

Quoted 27 lines

Takashi

sound/arm/aaci.c +3 -3

index b0a4744..89096e8 100644
@@ -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;
 	}