Hello,
The convention for checking for NULL pointers is !ptr and not ptr == NULL.
This patch fixes such occurences in goldfish driver, it applies against next-20141212
Signed-off-by: Loic Pefferkorn <[email protected]>
---
drivers/staging/goldfish/goldfish_audio.c | 8 ++++----
drivers/staging/goldfish/goldfish_nand.c | 10 +++++-----
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/goldfish/goldfish_audio.c b/drivers/staging/goldfish/goldfish_audio.c
index f200359..7ab034b 100644
--- a/drivers/staging/goldfish/goldfish_audio.c
+++ b/drivers/staging/goldfish/goldfish_audio.c
@@ -273,19 +273,19 @@ static int goldfish_audio_probe(struct platform_device *pdev)
dma_addr_t buf_addr;
data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
- if (data == NULL)
+ if (!data)
return -ENOMEM;
spin_lock_init(&data->lock);
init_waitqueue_head(&data->wait);
platform_set_drvdata(pdev, data);
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (r == NULL) {
+ if (!r) {
dev_err(&pdev->dev, "platform_get_resource failed\n");
return -ENODEV;
}
data->reg_base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
- if (data->reg_base == NULL)
+ if (!data->reg_base)
return -ENOMEM;
data->irq = platform_get_irq(pdev, 0);
@@ -295,7 +295,7 @@ static int goldfish_audio_probe(struct platform_device *pdev)
}
data->buffer_virt = dmam_alloc_coherent(&pdev->dev,
COMBINED_BUFFER_SIZE, &buf_addr, GFP_KERNEL);
- if (data->buffer_virt == NULL) {
+ if (!data->buffer_virt) {
dev_err(&pdev->dev, "allocate buffer failed\n");
return -ENOMEM;
}
diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c
index d68f216..8e8e594 100644
--- a/drivers/staging/goldfish/goldfish_nand.c
+++ b/drivers/staging/goldfish/goldfish_nand.c
@@ -48,7 +48,7 @@ static u32 goldfish_nand_cmd_with_params(struct mtd_info *mtd,
struct cmd_params *cps = nand->cmd_params;
unsigned char __iomem *base = nand->base;
- if (cps == NULL)
+ if (!cps)
return -1;
switch (cmd) {
@@ -330,7 +330,7 @@ static int goldfish_nand_init_device(struct platform_device *pdev,
mtd->priv = nand;
name = devm_kzalloc(&pdev->dev, name_len + 1, GFP_KERNEL);
- if (name == NULL)
+ if (!name)
return -ENOMEM;
mtd->name = name;
@@ -379,11 +379,11 @@ static int goldfish_nand_probe(struct platform_device *pdev)
unsigned char __iomem *base;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (r == NULL)
+ if (!r)
return -ENODEV;
base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
- if (base == NULL)
+ if (!base)
return -ENOMEM;
version = readl(base + NAND_VERSION);
@@ -399,7 +399,7 @@ static int goldfish_nand_probe(struct platform_device *pdev)
nand = devm_kzalloc(&pdev->dev, sizeof(*nand) +
sizeof(struct mtd_info) * num_dev, GFP_KERNEL);
- if (nand == NULL)
+ if (!nand)
return -ENOMEM;
mutex_init(&nand->lock);
--
2.1.3
Loic,
On Sat, Dec 13, 2014 at 05:29:26PM +0100, Loic Pefferkorn wrote:
> Hello,
>
> The convention for checking for NULL pointers is !ptr and not ptr == NULL.
> This patch fixes such occurences in goldfish driver, it applies against next-20141212
>
Whose convention is this? I can't find any mention in
Documention/CodingStyle. checkpatch.pl doesn't complain about them.
And there are almost three thousand examples in staging which don't
use this convention.
linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
2844
>
> Signed-off-by: Loic Pefferkorn <[email protected]>
> ---
> drivers/staging/goldfish/goldfish_audio.c | 8 ++++----
> drivers/staging/goldfish/goldfish_nand.c | 10 +++++-----
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/goldfish/goldfish_audio.c b/drivers/staging/goldfish/goldfish_audio.c
> index f200359..7ab034b 100644
> --- a/drivers/staging/goldfish/goldfish_audio.c
> +++ b/drivers/staging/goldfish/goldfish_audio.c
> @@ -273,19 +273,19 @@ static int goldfish_audio_probe(struct platform_device *pdev)
> dma_addr_t buf_addr;
>
> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> - if (data == NULL)
> + if (!data)
> return -ENOMEM;
> spin_lock_init(&data->lock);
> init_waitqueue_head(&data->wait);
> platform_set_drvdata(pdev, data);
>
> r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (r == NULL) {
> + if (!r) {
> dev_err(&pdev->dev, "platform_get_resource failed\n");
> return -ENODEV;
> }
> data->reg_base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
> - if (data->reg_base == NULL)
> + if (!data->reg_base)
> return -ENOMEM;
>
[...]
--
- Jeremiah Mahler
> Whose convention is this? I can't find any mention in
> Documention/CodingStyle. checkpatch.pl doesn't complain about them.
> And there are almost three thousand examples in staging which don't
> use this convention.
>
> linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
> 2844
Hi Jeremiah,
Thanks for your feedback.
I have used checkpatch.pl with the --strict flag:
$ ./scripts/checkpatch.pl --strict -f drivers/staging/goldfish/goldfish_nand.c
CHECK: Comparison to NULL could be written "!cps"
#51: FILE: drivers/staging/goldfish/goldfish_nand.c:51:
+ if (cps == NULL)
CHECK: Comparison to NULL could be written "!name"
#333: FILE: drivers/staging/goldfish/goldfish_nand.c:333:
+ if (name == NULL)
CHECK: Comparison to NULL could be written "!r"
#382: FILE: drivers/staging/goldfish/goldfish_nand.c:382:
+ if (r == NULL)
CHECK: Comparison to NULL could be written "!base"
#386: FILE: drivers/staging/goldfish/goldfish_nand.c:386:
+ if (base == NULL)
CHECK: Comparison to NULL could be written "!nand"
#402: FILE: drivers/staging/goldfish/goldfish_nand.c:402:
+ if (nand == NULL)
total: 0 errors, 0 warnings, 5 checks, 442 lines checked
drivers/staging/goldfish/goldfish_nand.c has style problems, please review.
I have also found another commit having the same purpose: 7f376cd6dc1c9bfd14514c70765e6900a961c4b8
--
Cheers,
Loïc
On Sat, 13 Dec 2014 17:29:26 +0100
Loic Pefferkorn <[email protected]> wrote:
> Hello,
>
> The convention for checking for NULL pointers is !ptr and not ptr == NULL.
> This patch fixes such occurences in goldfish driver, it applies against next-20141212
>
>
> Signed-off-by: Loic Pefferkorn <[email protected]>
Pointless churn. It makes it less readable if anything, and it removes
the type safety as you are now checking against 0 not (void *)0
NAK
Alan
Loïc,
On Sat, Dec 13, 2014 at 07:22:38PM +0100, Loic Pefferkorn wrote:
> > Whose convention is this? I can't find any mention in
> > Documention/CodingStyle. checkpatch.pl doesn't complain about them.
> > And there are almost three thousand examples in staging which don't
> > use this convention.
> >
> > linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
> > 2844
>
> Hi Jeremiah,
>
> Thanks for your feedback.
>
> I have used checkpatch.pl with the --strict flag:
>
> $ ./scripts/checkpatch.pl --strict -f drivers/staging/goldfish/goldfish_nand.c
> CHECK: Comparison to NULL could be written "!cps"
> #51: FILE: drivers/staging/goldfish/goldfish_nand.c:51:
> + if (cps == NULL)
>
> CHECK: Comparison to NULL could be written "!name"
> #333: FILE: drivers/staging/goldfish/goldfish_nand.c:333:
> + if (name == NULL)
>
> CHECK: Comparison to NULL could be written "!r"
> #382: FILE: drivers/staging/goldfish/goldfish_nand.c:382:
> + if (r == NULL)
>
> CHECK: Comparison to NULL could be written "!base"
> #386: FILE: drivers/staging/goldfish/goldfish_nand.c:386:
> + if (base == NULL)
>
> CHECK: Comparison to NULL could be written "!nand"
> #402: FILE: drivers/staging/goldfish/goldfish_nand.c:402:
> + if (nand == NULL)
>
> total: 0 errors, 0 warnings, 5 checks, 442 lines checked
>
> drivers/staging/goldfish/goldfish_nand.c has style problems, please review.
>
> I have also found another commit having the same purpose: 7f376cd6dc1c9bfd14514c70765e6900a961c4b8
>
> --
> Cheers,
> Loïc
It looks like you're right. I must say I am surprised. I had no idea
checkpatch.pl could be even more pedantic than it already is :-)
--
- Jeremiah Mahler
On Sat, Dec 13, 2014 at 07:07:05PM +0000, One Thousand Gnomes wrote:
>
> Pointless churn. It makes it less readable if anything, and it removes
> the type safety as you are now checking against 0 not (void *)0
>
> NAK
>
> Alan
The type safety is an interesting point I was not aware of.
Just in case, do you have something for me on your todo list?
--
Cheers,
Loïc
On Sat, 13 Dec 2014 11:46:47 -0800
Jeremiah Mahler <[email protected]> wrote:
> Loïc,
>
> On Sat, Dec 13, 2014 at 07:22:38PM +0100, Loic Pefferkorn wrote:
> > > Whose convention is this? I can't find any mention in
> > > Documention/CodingStyle. checkpatch.pl doesn't complain about them.
> > > And there are almost three thousand examples in staging which don't
> > > use this convention.
> > >
> > > linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
> > > 2844
> >
> > Hi Jeremiah,
> >
> > Thanks for your feedback.
> >
> > I have used checkpatch.pl with the --strict flag:
checkpatch.pl is a bit dubious at the best of times - you can't automate
taste without an AI ;). With --strict it's a positive hazard.
Those kind of small cleanups really only make sense if you are doing big
changes to the code itself anyway and are doing testing and all the rest.
In this case I'd say checkpatch.pl is actually wrong because in the
general case it's better to compare with NULL in C
If you write
if (!x)
and accidentally use a non-pointer type you don't get a warning. If you
try and compare a non pointer type to NULL you usually do. So the NULL
comparison avoids accidents.
The historical reason for it being done in C was I think to avoid the
if (x = NULL)
bug, but gcc will shout at you for that these days.
Alan
On Sat, 13 Dec 2014 22:20:52 +0100
Loic Pefferkorn <[email protected]> wrote:
> On Sat, Dec 13, 2014 at 07:07:05PM +0000, One Thousand Gnomes wrote:
> >
> > Pointless churn. It makes it less readable if anything, and it removes
> > the type safety as you are now checking against 0 not (void *)0
> >
> > NAK
> >
> > Alan
>
> The type safety is an interesting point I was not aware of.
>
> Just in case, do you have something for me on your todo list?
Depends what you feel confident tackling. An interesting but
challenging one to look at as a newcomer might be
https://bugzilla.kernel.org/show_bug.cgi?id=87951
because it provides you with an example .iso file you can loopback mount
to see the crash, and the same fixes were done for a similar bug so can
be seen in the git log to work from.
commit 410dd3cf4c9b36f27ed4542ee18b1af5e68645a4
It's predictable, it's repeatable and it can be done crashing a virtual
machine not a real one. That usually makes things eaiser to fix.
For something easier perhaps
https://bugzilla.kernel.org/show_bug.cgi?id=75111
which just needs the configuration help fixing
Alan
I haven't seen any bugs caused by lack of type safety with "!foo"...
I prefer !foo because it is more common in the kernel and I think it's
easier to read but I don't feel strongly about this.
I kind of hate "if (foo != NULL) though, because it's a double negative.
But I really hate when people start adding the "!= 0" on to all their
conditions.
if (frob() != 0)
Also:
if (a + b != 0)
People do this all the time instead of "if (a || b)" and I don't know
why...
regards,
dan carpenter
On Mon, Dec 15, 2014 at 11:51:47AM +0000, One Thousand Gnomes wrote:
>
> Depends what you feel confident tackling. An interesting but
> challenging one to look at as a newcomer might be
>
> https://bugzilla.kernel.org/show_bug.cgi?id=87951
>
> because it provides you with an example .iso file you can loopback mount
> to see the crash, and the same fixes were done for a similar bug so can
> be seen in the git log to work from.
>
> commit 410dd3cf4c9b36f27ed4542ee18b1af5e68645a4
>
> It's predictable, it's repeatable and it can be done crashing a virtual
> machine not a real one. That usually makes things eaiser to fix.
>
> For something easier perhaps
>
> https://bugzilla.kernel.org/show_bug.cgi?id=75111
>
> which just needs the configuration help fixing
>
> Alan
>
Hi Alan,
Your answer is greatly appreciated, I'm going to do my best!
--
Cheers,
Loïc
On Mon, Dec 15, 2014 at 11:44:21AM +0000, One Thousand Gnomes wrote:
> On Sat, 13 Dec 2014 11:46:47 -0800
> Jeremiah Mahler <[email protected]> wrote:
>
> > Loïc,
> >
> > On Sat, Dec 13, 2014 at 07:22:38PM +0100, Loic Pefferkorn wrote:
> > > > Whose convention is this? I can't find any mention in
> > > > Documention/CodingStyle. checkpatch.pl doesn't complain about them.
> > > > And there are almost three thousand examples in staging which don't
> > > > use this convention.
> > > >
> > > > linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
> > > > 2844
> > >
> > > Hi Jeremiah,
> > >
> > > Thanks for your feedback.
> > >
> > > I have used checkpatch.pl with the --strict flag:
>
> checkpatch.pl is a bit dubious at the best of times - you can't automate
> taste without an AI ;). With --strict it's a positive hazard.
>
> Those kind of small cleanups really only make sense if you are doing big
> changes to the code itself anyway and are doing testing and all the rest.
>
> In this case I'd say checkpatch.pl is actually wrong because in the
> general case it's better to compare with NULL in C
>
> If you write
>
> if (!x)
>
> and accidentally use a non-pointer type you don't get a warning. If you
> try and compare a non pointer type to NULL you usually do. So the NULL
> comparison avoids accidents.
>
> The historical reason for it being done in C was I think to avoid the
>
> if (x = NULL)
>
> bug, but gcc will shout at you for that these days.
>
Or another way mentioned in K&R that produces a compile error
if (NULL = x)
> Alan
>
>
>
--
- Jeremiah Mahler
On Mon, Dec 15, 2014 at 05:03:46AM -0800, Jeremiah Mahler wrote:
>
> Or another way mentioned in K&R that produces a compile error
>
> if (NULL = x)
>
Yes. People used to write Yoda code back in the day. Don't ever do
this in the kernel.
1) It looks stupid.
2) GCC will catch most == vs = bugs as Alan pointed out.
3) There are still some that sneak through because people put double
parenthesis around everything like "if ((foo = NULL) || (...)) {",
but checkpatch.pl and Smatch will catch those.
regards,
dan carpenter
Dan,
On Mon, Dec 15, 2014 at 04:43:34PM +0300, Dan Carpenter wrote:
> On Mon, Dec 15, 2014 at 05:03:46AM -0800, Jeremiah Mahler wrote:
> >
> > Or another way mentioned in K&R that produces a compile error
> >
> > if (NULL = x)
> >
>
> Yes. People used to write Yoda code back in the day. Don't ever do
> this in the kernel.
>
> 1) It looks stupid.
Agreed :-)
> 2) GCC will catch most == vs = bugs as Alan pointed out.
> 3) There are still some that sneak through because people put double
> parenthesis around everything like "if ((foo = NULL) || (...)) {",
> but checkpatch.pl and Smatch will catch those.
>
> regards,
> dan carpenter
>
--
- Jeremiah Mahler
On Mon, 2014-12-15 at 14:59 +0300, Dan Carpenter wrote:
> I prefer !foo because it is more common in the kernel and I think it's
> easier to read but I don't feel strongly about this.
Me too. But I do prefer consistency.
fyi: for variants of:
"if (!foo)"
vs
"if (foo == NULL)"
a little cocci script shows a preference for "if (!foo)"
of >5:1 in drivers/net/
(actual: 11557:2145)
and a little less (>3:1) in net/
(actual: 10263:3045)
$ cat pointer_style.cocci
@@
type T;
T *a;
@@
* a == NULL
@@
type T;
T *a;
@@
* a != NULL
@@
type T;
T *a;
@@
* !a