* [PATCH 1/5] arch/arm: Correct NULL test
@ 2009-12-27 21:26 Julia Lawall
2009-12-27 21:51 ` Uwe Kleine-König
0 siblings, 1 reply; 7+ messages in thread
From: Julia Lawall @ 2009-12-27 21:26 UTC (permalink / raw)
To: linux, linux-arm-kernel, linux-kernel, kernel-janitors
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/5] arch/arm: Correct NULL test
2009-12-27 21:26 [PATCH 1/5] arch/arm: Correct NULL test Julia Lawall
@ 2009-12-27 21:51 ` Uwe Kleine-König
2009-12-27 22:07 ` Julia Lawall
2009-12-27 22:25 ` Julia Lawall
0 siblings, 2 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2009-12-27 21:51 UTC (permalink / raw)
To: Julia Lawall; +Cc: linux, linux-arm-kernel, linux-kernel, kernel-janitors
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/ |
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/5] arch/arm: Correct NULL test
2009-12-27 21:51 ` Uwe Kleine-König
@ 2009-12-27 22:07 ` Julia Lawall
2009-12-28 16:24 ` H Hartley Sweeten
2009-12-27 22:25 ` Julia Lawall
1 sibling, 1 reply; 7+ messages in thread
From: Julia Lawall @ 2009-12-27 22:07 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux, linux-arm-kernel, linux-kernel, kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/5] arch/arm: Correct NULL test
2009-12-27 21:51 ` Uwe Kleine-König
2009-12-27 22:07 ` Julia Lawall
@ 2009-12-27 22:25 ` Julia Lawall
1 sibling, 0 replies; 7+ messages in thread
From: Julia Lawall @ 2009-12-27 22:25 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux, linux-arm-kernel, linux-kernel, kernel-janitors
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);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH 1/5] arch/arm: Correct NULL test
2009-12-27 22:07 ` Julia Lawall
@ 2009-12-28 16:24 ` H Hartley Sweeten
2009-12-28 16:51 ` Julia Lawall
0 siblings, 1 reply; 7+ messages in thread
From: H Hartley Sweeten @ 2009-12-28 16:24 UTC (permalink / raw)
To: Julia Lawall, Uwe Kleine-König
Cc: kernel-janitors, linux, linux-kernel, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/5] arch/arm: Correct NULL test
2009-12-28 16:24 ` H Hartley Sweeten
@ 2009-12-28 16:51 ` Julia Lawall
2009-12-28 17:09 ` H Hartley Sweeten
0 siblings, 1 reply; 7+ messages in thread
From: Julia Lawall @ 2009-12-28 16:51 UTC (permalink / raw)
To: H Hartley Sweeten
Cc: Uwe Kleine-König, kernel-janitors, linux, linux-kernel,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/5] arch/arm: Correct NULL test
2009-12-28 16:51 ` Julia Lawall
@ 2009-12-28 17:09 ` H Hartley Sweeten
0 siblings, 0 replies; 7+ messages in thread
From: H Hartley Sweeten @ 2009-12-28 17:09 UTC (permalink / raw)
To: Julia Lawall
Cc: Uwe Kleine-König, kernel-janitors, linux, linux-kernel,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-12-28 17:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-12-27 21:26 [PATCH 1/5] arch/arm: Correct NULL test Julia Lawall
2009-12-27 21:51 ` Uwe Kleine-König
2009-12-27 22:07 ` Julia Lawall
2009-12-28 16:24 ` H Hartley Sweeten
2009-12-28 16:51 ` Julia Lawall
2009-12-28 17:09 ` H Hartley Sweeten
2009-12-27 22:25 ` Julia Lawall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).