From: Julia Lawall <[email protected]>
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 <[email protected]>
---
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);
On Sun, Dec 27, 2009 at 10:26:54PM +0100, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> 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 <[email protected]>
>
> ---
> 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/ |
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 <[email protected]>
> >
> > 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 <[email protected]>
> >
> > ---
> > 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
From: Julia Lawall <[email protected]>
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 <[email protected]>
---
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);
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
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
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