| Message ID | 20130109150427.GL3931@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 9, 2013 at 3:04 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > So, it seems there's some concensus building here, and it seems that > I've become the chosen victi^wvolunteer for this. So, here's a patch. > It's missing a Guns-supplied-by: tag though. > > From: Russell King <rmk+kernel@arm.linux.org.uk> > Subject: Mark IS_ERR_OR_NULL() deprecated > > IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much > thought about it's effects. Common errors include: > 1. checking the returned pointer for functions defined as only > returning errno-pointer values, rather than using IS_ERR(). > This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return > PTR_ERR(ptr); > 2. using it to check functions which only ever return NULL on error, > thereby leading to another zero-error value return. > In the case of debugfs functions, these return errno-pointer values when > debugfs is configured out, which means code which blindly checks using > IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for > something that's not implemented. > > Therefore, let's schedule it for removal in a few releases. > > Nicolas Pitre comments: >> I do agree with Russell here. Despite the original intentions behind >> IS_ERR_OR_NULL() which were certainly legitimate, the end result in >> practice is less reliable code with increased maintenance costs. >> Unlike other convenience macros in the kernel, this one is giving a >> false sense of correctness with too many people falling in the trap >> of using it just because it is available. >> >> I strongly think this macro should simply be removed from the source >> tree entirely and the code reverted to explicit tests against NULL >> when appropriate. > > Suggested-by: David Howells <dhowells@redhat.com> > Tape-measuring-service-offered-by: Will Deacon <will.deacon@arm.com> > Victim-for-firing-sqad: Russell King <rmk+kernel@arm.linux.org.uk> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> Acked-by: Grant Likely <grant.likely@secretlab.ca> I fully agree with doing this. While I'm not a fan of the PTR_ERR pattern, this does (hopefully) make users think just a little bit more about it. > --- > Ok, so I'm in the firing line for suggesting this, but it appears > several people wish this to happen. > > I'm not intending to push this patch forwards _just_ yet: we need to > sort out the existing users _first_ to prevent the kernel turning into > one hell of a mess of warnings. I currently see 355 users. That's a lot, but not inconceivable for an auditing effort for pulling them out instead of scheduling for future removal. g.
On Wednesday 09 January 2013, Grant Likely wrote: > > Suggested-by: David Howells <dhowells@redhat.com> > > Tape-measuring-service-offered-by: Will Deacon <will.deacon@arm.com> > > Victim-for-firing-sqad: Russell King <rmk+kernel@arm.linux.org.uk> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > Acked-by: Grant Likely <grant.likely@secretlab.ca> Acked-by: Arnd Bergmann <arnd@arndb.de>
On Wed, 9 Jan 2013, Russell King - ARM Linux wrote: > So, it seems there's some concensus building here, and it seems that > I've become the chosen victi^wvolunteer for this. So, here's a patch. > It's missing a Guns-supplied-by: tag though. Guns-supplied-by: NRA (obviously) > From: Russell King <rmk+kernel@arm.linux.org.uk> > Subject: Mark IS_ERR_OR_NULL() deprecated > > IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much > thought about it's effects. Common errors include: > 1. checking the returned pointer for functions defined as only > returning errno-pointer values, rather than using IS_ERR(). > This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return > PTR_ERR(ptr); > 2. using it to check functions which only ever return NULL on error, > thereby leading to another zero-error value return. > In the case of debugfs functions, these return errno-pointer values when > debugfs is configured out, which means code which blindly checks using > IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for > something that's not implemented. > > Therefore, let's schedule it for removal in a few releases. > > Nicolas Pitre comments: > > I do agree with Russell here. Despite the original intentions behind > > IS_ERR_OR_NULL() which were certainly legitimate, the end result in > > practice is less reliable code with increased maintenance costs. > > Unlike other convenience macros in the kernel, this one is giving a > > false sense of correctness with too many people falling in the trap > > of using it just because it is available. > > > > I strongly think this macro should simply be removed from the source > > tree entirely and the code reverted to explicit tests against NULL > > when appropriate. > > Suggested-by: David Howells <dhowells@redhat.com> > Tape-measuring-service-offered-by: Will Deacon <will.deacon@arm.com> > Victim-for-firing-sqad: Russell King <rmk+kernel@arm.linux.org.uk> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> Acked-by: Nicolas Pitre <nico@linaro.org> Anyone with good coccinelle skills around to deal with the users? > --- > Ok, so I'm in the firing line for suggesting this, but it appears > several people wish this to happen. > > I'm not intending to push this patch forwards _just_ yet: we need to > sort out the existing users _first_ to prevent the kernel turning into > one hell of a mess of warnings. > > include/linux/err.h | 17 ++++++++++++++++- > 1 files changed, 16 insertions(+), 1 deletions(-) > > diff --git a/include/linux/err.h b/include/linux/err.h > index f2edce2..d5a85df 100644 > --- a/include/linux/err.h > +++ b/include/linux/err.h > @@ -34,7 +34,22 @@ static inline long __must_check IS_ERR(const void *ptr) > return IS_ERR_VALUE((unsigned long)ptr); > } > > -static inline long __must_check IS_ERR_OR_NULL(const void *ptr) > +/* > + * IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much > + * thought about it's effects. Common errors include: > + * 1. checking the returned pointer for functions defined as only returning > + * errno-pointer values, rather than using IS_ERR(). > + * This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return PTR_ERR(ptr); > + * 2. using it to check functions which only ever return NULL on error, > + * thereby leading to another zero-error value return. > + * In the case of debugfs functions, these return errno-pointer values when > + * debugfs is configured out, which means code which blindly checks using > + * IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for > + * something that's not implemented. > + * > + * Therefore, let's schedule it for removal in a few releases. > + */ > +static inline long __must_check __deprecated IS_ERR_OR_NULL(const void *ptr) > { > return !ptr || IS_ERR_VALUE((unsigned long)ptr); > } >
On Wed, Jan 09, 2013 at 10:27:53AM -0500, Nicolas Pitre wrote:
> Anyone with good coccinelle skills around to deal with the users?
I'm not sure that's a solution.
For example:
err = gpio_request(en_vdd_1v05, "EN_VDD_1V05");
if (err) {
pr_err("%s: gpio_request failed: %d\n", __func__, err);
return err;
}
...
regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk");
if (IS_ERR_OR_NULL(regulator)) {
pr_err("%s: regulator_get failed: %d\n", __func__,
(int)PTR_ERR(regulator));
goto err_reg;
}
regulator_enable(regulator);
err = tegra_pcie_init(true, true);
...
err_reg:
gpio_free(en_vdd_1v05);
return err;
}
Now, regulator_get() returns error-pointers for real errors when it's
configured in. When regulator support is not configured, it returns
NULL.
So, one solution here would be:
if (IS_ERR(regulator)) {
err = PTR_ERR(regulator);
pr_err("%s: regulator_get failed: %d\n", __func__, err);
goto err_reg;
}
but leaves us with the question: is it safe to call tegra_pcie_init()
without regulator support?
The problem there is that "fixing" this causes a behavioural change in
the code, and we don't know what effect that change may have.
On Wed, 9 Jan 2013, Russell King - ARM Linux wrote: > On Wed, Jan 09, 2013 at 10:27:53AM -0500, Nicolas Pitre wrote: > > Anyone with good coccinelle skills around to deal with the users? > > I'm not sure that's a solution. Well, I was thinking that coccinelle could handle the majority of the 354 users when the "fix" is obvious enough to be automated. That said, if we want people to fix their code, it is probably necessary to merge your patch right away so the warnings are actually being seen, and revert it right before the final v3.8 release if the remaining warnings are still too numerous. Repeat with next cycle. > For example: > > err = gpio_request(en_vdd_1v05, "EN_VDD_1V05"); > if (err) { > pr_err("%s: gpio_request failed: %d\n", __func__, err); > return err; > } > ... > regulator = regulator_get(NULL, "vdd_ldo0,vddio_pex_clk"); > if (IS_ERR_OR_NULL(regulator)) { > pr_err("%s: regulator_get failed: %d\n", __func__, > (int)PTR_ERR(regulator)); > goto err_reg; > } > > regulator_enable(regulator); > > err = tegra_pcie_init(true, true); > ... > err_reg: > gpio_free(en_vdd_1v05); > > return err; > } > > Now, regulator_get() returns error-pointers for real errors when it's > configured in. When regulator support is not configured, it returns > NULL. > > So, one solution here would be: > > if (IS_ERR(regulator)) { > err = PTR_ERR(regulator); > pr_err("%s: regulator_get failed: %d\n", __func__, err); > goto err_reg; > } > > but leaves us with the question: is it safe to call tegra_pcie_init() > without regulator support? The best approach is to assume it is not, but unlike the current code, this should be fixed by returning an appropriate error code instead of 0. Nicolas
On Wed, Jan 9, 2013 at 4:04 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > So, it seems there's some concensus building here, and it seems that > I've become the chosen victi^wvolunteer for this. So, here's a patch. > It's missing a Guns-supplied-by: tag though. > > From: Russell King <rmk+kernel@arm.linux.org.uk> > Subject: Mark IS_ERR_OR_NULL() deprecated Acked-by: Linus Walleij <linus.walleij@linaro.org> Following the tradition of computer science an alternate title of the patch could be "IS_ERR_OR_NULL() considered harmful". Yours, Linus Walleij
diff --git a/include/linux/err.h b/include/linux/err.h index f2edce2..d5a85df 100644 --- a/include/linux/err.h +++ b/include/linux/err.h @@ -34,7 +34,22 @@ static inline long __must_check IS_ERR(const void *ptr) return IS_ERR_VALUE((unsigned long)ptr); } -static inline long __must_check IS_ERR_OR_NULL(const void *ptr) +/* + * IS_ERR_OR_NULL() attracts a lot of abuse: people use it without much + * thought about it's effects. Common errors include: + * 1. checking the returned pointer for functions defined as only returning + * errno-pointer values, rather than using IS_ERR(). + * This leads to: ptr = foo(); if (IS_ERR_OR_NULL(ptr)) return PTR_ERR(ptr); + * 2. using it to check functions which only ever return NULL on error, + * thereby leading to another zero-error value return. + * In the case of debugfs functions, these return errno-pointer values when + * debugfs is configured out, which means code which blindly checks using + * IS_ERR_OR_NULL() ends up returning errors, which is rather perverse for + * something that's not implemented. + * + * Therefore, let's schedule it for removal in a few releases. + */ +static inline long __must_check __deprecated IS_ERR_OR_NULL(const void *ptr) { return !ptr || IS_ERR_VALUE((unsigned long)ptr); }