[PATCH 1/5] arch/arm: Correct NULL test

[PATCH 1/5] arch/arm: Correct NULL test

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Test the just-allocated value for NULL rather than some other value.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression x,y;
statement S;
@@

x = \(kmalloc\|kcalloc\|kzalloc\)(...);
(
if ((x) == NULL) S
|
if (
-   y
+   x
       == NULL)
 S
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/arm/plat-pxa/dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/arch/arm/plat-pxa/dma.c b/arch/arm/plat-pxa/dma.c
--- a/arch/arm/plat-pxa/dma.c
+++ b/arch/arm/plat-pxa/dma.c
@@ -244,7 +244,7 @@ static void pxa_dma_init_debugfs(void)
 
 	dbgfs_chan = kmalloc(sizeof(*dbgfs_state) * num_dma_channels,
 			     GFP_KERNEL);
-	if (!dbgfs_state)
+	if (!dbgfs_chan)
 		goto err_alloc;
 
 	chandir = debugfs_create_dir("channels", dbgfs_root);

Re: [PATCH 1/5] arch/arm: Correct NULL test

[Uwe Kleine-König]

On Sun, Dec 27, 2009 at 10:26:54PM +0100, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> Test the just-allocated value for NULL rather than some other value.
> 
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> ....
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  arch/arm/plat-pxa/dma.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -u -p a/arch/arm/plat-pxa/dma.c b/arch/arm/plat-pxa/dma.c
> --- a/arch/arm/plat-pxa/dma.c
> +++ b/arch/arm/plat-pxa/dma.c
> @@ -244,7 +244,7 @@ static void pxa_dma_init_debugfs(void)
>  
>  	dbgfs_chan = kmalloc(sizeof(*dbgfs_state) * num_dma_channels,
>  			     GFP_KERNEL);
> -	if (!dbgfs_state)
> +	if (!dbgfs_chan)
>  		goto err_alloc;
Shouldn't the malloc line read:

	... = kmalloc(sizeof(*dbgfs_chan) * ...)
	                      ^^^^^^^^^^

?

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH 1/5] arch/arm: Correct NULL test

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1079 bytes --]

On Sun, 27 Dec 2009, Uwe Kleine-König wrote:

> On Sun, Dec 27, 2009 at 10:26:54PM +0100, Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Test the just-allocated value for NULL rather than some other value.
> > 
> > The semantic patch that makes this change is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > ....
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  arch/arm/plat-pxa/dma.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff -u -p a/arch/arm/plat-pxa/dma.c b/arch/arm/plat-pxa/dma.c
> > --- a/arch/arm/plat-pxa/dma.c
> > +++ b/arch/arm/plat-pxa/dma.c
> > @@ -244,7 +244,7 @@ static void pxa_dma_init_debugfs(void)
> >  
> >  	dbgfs_chan = kmalloc(sizeof(*dbgfs_state) * num_dma_channels,
> >  			     GFP_KERNEL);
> > -	if (!dbgfs_state)
> > +	if (!dbgfs_chan)
> >  		goto err_alloc;
> Shouldn't the malloc line read:
> 
> 	... = kmalloc(sizeof(*dbgfs_chan) * ...)
> 	                      ^^^^^^^^^^
> 
> ?

Good point, thanks.  I will send a revised patch.

julia

RE: [PATCH 1/5] arch/arm: Correct NULL test

[H Hartley Sweeten]

On Sunday, December 27, 2009 3:08 PM, Julia Lawall wrote:
>>>  	dbgfs_chan = kmalloc(sizeof(*dbgfs_state) * num_dma_channels,
>>>  			     GFP_KERNEL);
>>> -	if (!dbgfs_state)
>>> +	if (!dbgfs_chan)
>>>  		goto err_alloc;
>> Shouldn't the malloc line read:
>> 
>> 	... = kmalloc(sizeof(*dbgfs_chan) * ...)
>> 	                      ^^^^^^^^^^
>> 
>> ?
>
> Good point, thanks.  I will send a revised patch.

Wouldn't this be clearer?

	... = kmalloc(sizeof(struct dentry) * ...)

Regards,
Hartley

RE: [PATCH 1/5] arch/arm: Correct NULL test

[Julia Lawall]

On Mon, 28 Dec 2009, H Hartley Sweeten wrote:

> On Sunday, December 27, 2009 3:08 PM, Julia Lawall wrote:
> >>>  	dbgfs_chan = kmalloc(sizeof(*dbgfs_state) * num_dma_channels,
> >>>  			     GFP_KERNEL);
> >>> -	if (!dbgfs_state)
> >>> +	if (!dbgfs_chan)
> >>>  		goto err_alloc;
> >> Shouldn't the malloc line read:
> >> 
> >> 	... = kmalloc(sizeof(*dbgfs_chan) * ...)
> >> 	                      ^^^^^^^^^^
> >> 
> >> ?
> >
> > Good point, thanks.  I will send a revised patch.
> 
> Wouldn't this be clearer?
> 
> 	... = kmalloc(sizeof(struct dentry) * ...)

Documentation/CodingStyle thinks otherwise:

"The preferred form for passing a size of a struct is the following:

	p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability 
and introduces an opportunity for a bug when the pointer variable type is 
changed but the corresponding sizeof that is passed to a memory allocator 
is not."

And actually, in this case, sizeof(struct dentry) would be wrong, because 
the type of dbgfs_chan is struct dentry **, not struct dentry *.  What is 
wanted is an array of pointers.

julia

RE: [PATCH 1/5] arch/arm: Correct NULL test

[H Hartley Sweeten]

On Monday, December 28, 2009 9:51 AM, Julia Lawall wrote:
> On Mon, 28 Dec 2009, H Hartley Sweeten wrote:
>
>> On Sunday, December 27, 2009 3:08 PM, Julia Lawall wrote:
>>>>>  	dbgfs_chan = kmalloc(sizeof(*dbgfs_state) * num_dma_channels,
>>>>>  			     GFP_KERNEL);
>>>>> -	if (!dbgfs_state)
>>>>> +	if (!dbgfs_chan)
>>>>>  		goto err_alloc;
>>>> Shouldn't the malloc line read:
>>>> 
>>>> 	... = kmalloc(sizeof(*dbgfs_chan) * ...)
>>>> 	                      ^^^^^^^^^^
>>>> 
>>>> ?
>>>
>>> Good point, thanks.  I will send a revised patch.
>> 
>> Wouldn't this be clearer?
>> 
>> 	... = kmalloc(sizeof(struct dentry) * ...)
>
> Documentation/CodingStyle thinks otherwise:
>
> "The preferred form for passing a size of a struct is the following:
>
> 	p = kmalloc(sizeof(*p), ...);
>
> The alternative form where struct name is spelled out hurts readability 
> and introduces an opportunity for a bug when the pointer variable type is 
> changed but the corresponding sizeof that is passed to a memory allocator 
> is not."
>
> And actually, in this case, sizeof(struct dentry) would be wrong, because 
> the type of dbgfs_chan is struct dentry **, not struct dentry *.  What is 
> wanted is an array of pointers.

Ah, missed that.  Thanks for pointing it out.

Regards,
Hartley

Re: [PATCH 1/5] arch/arm: Correct NULL test

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Test the just-allocated value for NULL rather than some other value.
Adjust the size of the allocated array as well, according to the location
storing the result.

The semantic patch that makes the first change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression x,y;
statement S;
@@

x = \(kmalloc\|kcalloc\|kzalloc\)(...);
(
if ((x) == NULL) S
|
if (
-   y
+   x
       == NULL)
 S
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/arm/plat-pxa/dma.c             |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-pxa/dma.c b/arch/arm/plat-pxa/dma.c
index 2975798..fb28dc9 100644
--- a/arch/arm/plat-pxa/dma.c
+++ b/arch/arm/plat-pxa/dma.c
@@ -242,9 +242,9 @@ static void pxa_dma_init_debugfs(void)
 	if (!dbgfs_state)
 		goto err_state;
 
-	dbgfs_chan = kmalloc(sizeof(*dbgfs_state) * num_dma_channels,
+	dbgfs_chan = kmalloc(sizeof(*dbgfs_chan) * num_dma_channels,
 			     GFP_KERNEL);
-	if (!dbgfs_state)
+	if (!dbgfs_chan)
 		goto err_alloc;
 
 	chandir = debugfs_create_dir("channels", dbgfs_root);