Re: IS_ERR_OR_NULL - please STOP telling people to use this on a whim
On 10/17/2012 11:33 PM, Russell King - ARM Linux : On Wed, Oct 17, 2012 at 11:28:48PM +0300, Phil Carmody wrote: So, what to do? It can and has been used sensibly, so I don't think removing it is the best option. Well, the first thing that needs to be done is a full review of every user and fixes applied. The second thing is that we need eyes on code _and_ review comments, and educate those who are telling people to use IS_ERR_OR_NULL() whenever they see an IS_ERR() to think about the code a little more - that's kind of the purpose of my email. Unfortunately, it's going to take time to achieve a change, and if I end up being the only one who's spotting these errors, I'm going to get incredibly pissed off at doing so (because it will feel like I'm being ignored when there's a constant stream of it.) Another thing would be to work out whether we can get checkpatch to detect usage of IS_ERR_OR_NULL(p) followed by PTR_ERR(p) without any explicit NULL checks against p in the same block. That's probably going to be interesting to code up in perl. True that it would make sense to include in checkpatch to be able to block code beforehand. But for sure correction of existing code seems to be a work for Coccinelle. Best regards, -- Nicolas Ferre -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
IS_ERR_OR_NULL - please STOP telling people to use this on a whim
People,
This is not aimed at anyone specifically - but it is aimed at everyone
who reviews patches to make them aware of this issue, and to modify their
behaviour.
I'm geting sick and tired of telling people about this. I've just
floated the idea of removing IS_ERR_OR_NULL from the kernel tree because
it's one of the most incorrectly used and abused macros we have in the
source tree.
It would be one thing if this was only being done by people who are
submitting new code, but it's far worse than that. Reviewers who should
know better are telling people to use it _incorrectly_.
Reviewers really need to think about your review comments. Looking
through the kernel tree today, I see lots of uses of IS_ERR_OR_NULL(),
many of them are *buggy*.
Take a moment to think about this:
int error_value(struct device *dev, void *foo)
{
if (IS_ERR_OR_NULL(foo))
return PTR_ERR(foo);
return 0;
}
Consider the value this function returns for three arguments:
1. an errno encoded pointer
2. a NULL pointer.
3. a valid pointer.
If you can't see the problem, then *do* *not* tell anyone to use
IS_ERR_OR_NULL(), because you do *not* have the understanding necessary
to make that judgement yourself - you're probably telling people to
create buggy code.
Here's the list so far of what looks like buggy uses specific to ARM.
There _are_ others elsewhere in the kernel.
drivers/media/video/s5p-mfc/s5p_mfc.c: if (IS_ERR_OR_NULL(dev-alloc_ctx[0])) {
drivers/media/video/s5p-mfc/s5p_mfc.c- ret =
PTR_ERR(dev-alloc_ctx[0]);
drivers/media/video/s5p-mfc/s5p_mfc.c- goto err_res;
drivers/media/video/s5p-mfc/s5p_mfc.c- }
--
drivers/media/video/s5p-mfc/s5p_mfc.c: if (IS_ERR_OR_NULL(dev-alloc_ctx[1])) {
drivers/media/video/s5p-mfc/s5p_mfc.c- ret =
PTR_ERR(dev-alloc_ctx[1]);
drivers/media/video/s5p-mfc/s5p_mfc.c- goto err_mem_init_ctx_1;
drivers/media/video/s5p-mfc/s5p_mfc.c- }
--
drivers/staging/omapdrm/omap_dmm_tiler.c: if (IS_ERR_OR_NULL(txn))
drivers/staging/omapdrm/omap_dmm_tiler.c- return PTR_ERR(txn);
drivers/staging/omapdrm/omap_dmm_tiler.c-
drivers/staging/omapdrm/omap_dmm_tiler.c- tcm_for_each_slice(slice,
*area, area_s) {
--
drivers/staging/omap-thermal/omap-bandgap.c:if (IS_ERR_OR_NULL(bg_ptr)) {
drivers/staging/omap-thermal/omap-bandgap.c-dev_err(pdev-dev,
failed to fetch platform data\n);
drivers/staging/omap-thermal/omap-bandgap.c-return PTR_ERR(bg_ptr);
drivers/staging/omap-thermal/omap-bandgap.c-}
--
drivers/staging/omap-thermal/omap-thermal-common.c: if
(IS_ERR_OR_NULL(data-omap_thermal)) {
drivers/staging/omap-thermal/omap-thermal-common.c-
dev_err(bg_ptr-dev, thermal zone device is NULL\n);
drivers/staging/omap-thermal/omap-thermal-common.c- return
PTR_ERR(data-omap_thermal);
drivers/staging/omap-thermal/omap-thermal-common.c- }
--
drivers/staging/omap-thermal/omap-thermal-common.c: if
(IS_ERR_OR_NULL(freq_table)) {
drivers/staging/omap-thermal/omap-thermal-common.c-
dev_err(bg_ptr-dev,
drivers/staging/omap-thermal/omap-thermal-common.c- %s:
failed to get cpufreq table (%p)\n,
drivers/staging/omap-thermal/omap-thermal-common.c-
__func__, freq_table);
--
drivers/staging/omap-thermal/omap-thermal-common.c: if
(IS_ERR_OR_NULL(data-cool_dev)) {
drivers/staging/omap-thermal/omap-thermal-common.c-
dev_err(bg_ptr-dev,
drivers/staging/omap-thermal/omap-thermal-common.c- Failed
to register cpufreq cooling device\n);
drivers/staging/omap-thermal/omap-thermal-common.c- return
PTR_ERR(data-cool_dev);
--
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c: if (IS_ERR_OR_NULL(sgt)) {
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c- ret = PTR_ERR(sgt);
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c- goto err_buf_detach;
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c- }
--
drivers/gpu/drm/exynos/exynos_drm_fbdev.c: if (IS_ERR_OR_NULL(helper-fb))
{
drivers/gpu/drm/exynos/exynos_drm_fbdev.c- DRM_ERROR(failed to
create drm framebuffer.\n);
drivers/gpu/drm/exynos/exynos_drm_fbdev.c- ret =
PTR_ERR(helper-fb);
drivers/gpu/drm/exynos/exynos_drm_fbdev.c- goto out;
--
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IS_ERR_OR_NULL - please STOP telling people to use this on a whim
On 17/10/12 20:41 +0100, Russell King - ARM Linux wrote:
People,
This is not aimed at anyone specifically - but it is aimed at everyone
who reviews patches to make them aware of this issue, and to modify their
behaviour.
I'm geting sick and tired of telling people about this. I've just
floated the idea of removing IS_ERR_OR_NULL from the kernel tree because
it's one of the most incorrectly used and abused macros we have in the
source tree.
This makes me sad. I was responsible for its introduction, and my motive
was exactly yours in sending the above.
It would be one thing if this was only being done by people who are
submitting new code, but it's far worse than that. Reviewers who should
know better are telling people to use it _incorrectly_.
Reviewers really need to think about your review comments. Looking
through the kernel tree today, I see lots of uses of IS_ERR_OR_NULL(),
many of them are *buggy*.
Take a moment to think about this:
int error_value(struct device *dev, void *foo)
{
if (IS_ERR_OR_NULL(foo))
return PTR_ERR(foo);
return 0;
}
Consider the value this function returns for three arguments:
1. an errno encoded pointer
2. a NULL pointer.
3. a valid pointer.
If you can't see the problem, then *do* *not* tell anyone to use
IS_ERR_OR_NULL(), because you do *not* have the understanding necessary
to make that judgement yourself - you're probably telling people to
create buggy code.
The problem I saw was functions returning -ERRORs or NULL. There were
too many, and there was too much sloppy code inconsistently handling one
or either of the two, and not always both. I did consider trying to fix
some of the core functions that were returning -ERRORs or NULL to the
drivers I was involved in, but it seemed like there were too many, and
that would be too brave. I imagined that my macro would help catch
that undesirable situation, and permit people to map the error onto
whatever was most appropriate to propagate on.
The idea of them propagating the undesirable problem up further in the call
chain is the exact antithesis of what I intended.
Thank you for highlighting the issue I didn't foresee (neither did my
colleagues at Nokia, they made good use of it fairly quickly) and in
such unambiguous terms. Better to nip it in the bud, certainly.
Here's the list so far of what looks like buggy uses specific to ARM.
There _are_ others elsewhere in the kernel.
drivers/media/video/s5p-mfc/s5p_mfc.c: if
(IS_ERR_OR_NULL(dev-alloc_ctx[0])) {
drivers/media/video/s5p-mfc/s5p_mfc.c- ret =
PTR_ERR(dev-alloc_ctx[0]);
drivers/media/video/s5p-mfc/s5p_mfc.c- goto err_res;
drivers/media/video/s5p-mfc/s5p_mfc.c- }
...
:-(
So, what to do? It can and has been used sensibly, so I don't think removing
it is the best option.
Phil
pcarm...@nokia.com is no more, I'm now pc+l...@asdf.org
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IS_ERR_OR_NULL - please STOP telling people to use this on a whim
On Wed, Oct 17, 2012 at 11:28:48PM +0300, Phil Carmody wrote: So, what to do? It can and has been used sensibly, so I don't think removing it is the best option. Well, the first thing that needs to be done is a full review of every user and fixes applied. The second thing is that we need eyes on code _and_ review comments, and educate those who are telling people to use IS_ERR_OR_NULL() whenever they see an IS_ERR() to think about the code a little more - that's kind of the purpose of my email. Unfortunately, it's going to take time to achieve a change, and if I end up being the only one who's spotting these errors, I'm going to get incredibly pissed off at doing so (because it will feel like I'm being ignored when there's a constant stream of it.) Another thing would be to work out whether we can get checkpatch to detect usage of IS_ERR_OR_NULL(p) followed by PTR_ERR(p) without any explicit NULL checks against p in the same block. That's probably going to be interesting to code up in perl. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
