From: Markus Elfring <[email protected]>
Date: Thu, 20 Jun 2019 19:12:53 +0200
The function “devm_ioremap_resource” contains appropriate error reporting.
Thus it can be questionable to present another error message
at other places.
Provide design options for the adjustment of affected source code
by the means of the semantic patch language (Coccinelle software).
Signed-off-by: Markus Elfring <[email protected]>
---
.../coccinelle/misc/redundant_dev_err.cocci | 53 +++++++++++++++++++
1 file changed, 53 insertions(+)
create mode 100644 scripts/coccinelle/misc/redundant_dev_err.cocci
diff --git a/scripts/coccinelle/misc/redundant_dev_err.cocci b/scripts/coccinelle/misc/redundant_dev_err.cocci
new file mode 100644
index 000000000000..aeb228280276
--- /dev/null
+++ b/scripts/coccinelle/misc/redundant_dev_err.cocci
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Reconsider a function call for redundant error reporting.
+//
+// Keywords: dev_err redundant device error messages
+// Confidence: Medium
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+@display depends on context@
+expression e;
+@@
+ e = devm_ioremap_resource(...);
+ if (IS_ERR(e))
+ {
+* dev_err(...);
+ return (...);
+ }
+
+@deletion depends on patch@
+expression e;
+@@
+ e = devm_ioremap_resource(...);
+ if (IS_ERR(e))
+-{
+- dev_err(...);
+ return (...);
+-}
+
+@or depends on org || report@
+expression e;
+position p;
+@@
+ e = devm_ioremap_resource(...);
+ if (IS_ERR(e))
+ {
+ dev_err@p(...);
+ return (...);
+ }
+
+@script:python to_do depends on org@
+p << or.p;
+@@
+coccilib.org.print_todo(p[0],
+ "WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.")
+
+@script:python reporting depends on report@
+p << or.p;
+@@
+coccilib.report.print_report(p[0],
+ "WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.")
--
2.22.0
On Thu, 20 Jun 2019, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Thu, 20 Jun 2019 19:12:53 +0200
>
> The function “devm_ioremap_resource” contains appropriate error reporting.
> Thus it can be questionable to present another error message
> at other places.
>
> Provide design options for the adjustment of affected source code
> by the means of the semantic patch language (Coccinelle software).
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> .../coccinelle/misc/redundant_dev_err.cocci | 53 +++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 scripts/coccinelle/misc/redundant_dev_err.cocci
>
> diff --git a/scripts/coccinelle/misc/redundant_dev_err.cocci b/scripts/coccinelle/misc/redundant_dev_err.cocci
> new file mode 100644
> index 000000000000..aeb228280276
> --- /dev/null
> +++ b/scripts/coccinelle/misc/redundant_dev_err.cocci
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/// Reconsider a function call for redundant error reporting.
> +//
> +// Keywords: dev_err redundant device error messages
> +// Confidence: Medium
> +
> +virtual patch
> +virtual context
> +virtual org
> +virtual report
> +
> +@display depends on context@
> +expression e;
> +@@
> + e = devm_ioremap_resource(...);
> + if (IS_ERR(e))
> + {
> +* dev_err(...);
> + return (...);
> + }
Why do you assume that there is exactly one dev_err and one return after
the test?
> +
> +@deletion depends on patch@
> +expression e;
> +@@
> + e = devm_ioremap_resource(...);
> + if (IS_ERR(e))
> +-{
> +- dev_err(...);
> + return (...);
> +-}
> +
> +@or depends on org || report@
> +expression e;
> +position p;
> +@@
> + e = devm_ioremap_resource(...);
> + if (IS_ERR(e))
> + {
> + dev_err@p(...);
> + return (...);
> + }
> +
> +@script:python to_do depends on org@
> +p << or.p;
> +@@
> +coccilib.org.print_todo(p[0],
> + "WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.")
"the previously called function" would be better as
"devm_ioremap_resource".
julia
> +
> +@script:python reporting depends on report@
> +p << or.p;
> +@@
> +coccilib.report.print_report(p[0],
> + "WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.")
> --
> 2.22.0
>
>
>> +@display depends on context@
>> +expression e;
>> +@@
>> + e = devm_ioremap_resource(...);
>> + if (IS_ERR(e))
>> + {
>> +* dev_err(...);
>> + return (...);
>> + }
>
> Why do you assume that there is exactly one dev_err and one return after
> the test?
I propose to start with the addition of a simple source code search pattern.
Would you prefer to clarify a more advanced approach?
>> +@script:python to_do depends on org@
>> +p << or.p;
>> +@@
>> +coccilib.org.print_todo(p[0],
>> + "WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.")
>
> "the previously called function" would be better as "devm_ioremap_resource".
Would you like to get the relevant function name dynamically determined?
Regards,
Markus
>> +@display depends on context@
>> +expression e;
>> +@@
>> + e = devm_ioremap_resource(...);
>> + if (IS_ERR(e))
>> + {
>> +* dev_err(...);
>> + return (...);
>> + }
>
> Why do you assume that there is exactly one dev_err and one return after
> the test?
I propose to start with the addition of a simple source code search pattern.
Would you prefer to clarify a more advanced approach?
>> +@script:python to_do depends on org@
>> +p << or.p;
>> +@@
>> +coccilib.org.print_todo(p[0],
>> + "WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.")
>
> "the previously called function" would be better as "devm_ioremap_resource".
Would you like to get the relevant function name dynamically determined?
Regards,
Markus
On Thu, 20 Jun 2019, Markus Elfring wrote:
> >> +@display depends on context@
> >> +expression e;
> >> +@@
> >> + e = devm_ioremap_resource(...);
> >> + if (IS_ERR(e))
> >> + {
> >> +* dev_err(...);
> >> + return (...);
> >> + }
> >
> > Why do you assume that there is exactly one dev_err and one return after
> > the test?
>
> I propose to start with the addition of a simple source code search pattern.
> Would you prefer to clarify a more advanced approach?
I think that something like
if (IS_ERR(e))
{
<+...
*dev_err(...)
...+>
}
would be more appropriate. Whether there is a return or not doesn't
really matter.
>
>
> >> +@script:python to_do depends on org@
> >> +p << or.p;
> >> +@@
> >> +coccilib.org.print_todo(p[0],
> >> + "WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.")
> >
> > "the previously called function" would be better as "devm_ioremap_resource".
>
> Would you like to get the relevant function name dynamically determined?
I have no idea what you consider "the relevant function name" to be. If
it is always devm_ioremap_resource then it would seem that it does not
need to be dynamically determined.
julia
>> Would you prefer to clarify a more advanced approach?
>
> I think that something like
>
> if (IS_ERR(e))
> {
> <+...
> *dev_err(...)
> ...+>
> }
>
> would be more appropriate.
This SmPL construct can be more powerful.
> Whether there is a return or not doesn't really matter.
Such an adjustment can be helpful for a few operation modes.
But the number of statements in the if branch will influence the possibility
for the deletion of curly braces together with redundant dev_err() calls
by the SmPL patch mode.
>> Would you like to get the relevant function name dynamically determined?
>
> I have no idea what you consider "the relevant function name" to be.
> If it is always devm_ioremap_resource then it would seem that it does not
> need to be dynamically determined.
Do other functions share the same error reporting strategy so that any more
collateral software evolution can happen?
Regards,
Markus
> I think that something like
>
> if (IS_ERR(e))
> {
> <+...
> *dev_err(...)
> ...+>
> }
>
> would be more appropriate. Whether there is a return or not doesn't
> really matter.
Do you find the following SmPL change specification useful and acceptable?
@deletion depends on patch@
expression e;
@@
e = devm_ioremap_resource(...);
if (IS_ERR(e))
(
-{
- dev_err(...);
return (...);
-}
|{
<+...
- dev_err(...);
...+>
}
)
Would this approach need a version check for the Coccinelle software?
Regards,
Markus
On Fri, 21 Jun 2019, Markus Elfring wrote:
> > I think that something like
> >
> > if (IS_ERR(e))
> > {
> > <+...
> > *dev_err(...)
> > ...+>
> > }
> >
> > would be more appropriate. Whether there is a return or not doesn't
> > really matter.
>
> Do you find the following SmPL change specification useful and acceptable?
>
>
> @deletion depends on patch@
> expression e;
> @@
> e = devm_ioremap_resource(...);
> if (IS_ERR(e))
> (
> -{
> - dev_err(...);
> return (...);
I still don't see the point of specifying return. Why not just S, where S
is a statement metavariable?
julia
> -}
> |{
I realize that you confuse conciseness with readability, but it would
really look better to have the | on a line by itself.
julia
> <+...
> - dev_err(...);
> ...+>
> }
> )
>
>
> Would this approach need a version check for the Coccinelle software?
Why would that be necessary?
> I still don't see the point of specifying return. Why not just S, where S
> is a statement metavariable?
Do you find the following SmPL change specification more appropriate?
@deletion depends on patch@
expression e;
statement s;
@@
e = devm_ioremap_resource(...);
if (IS_ERR(e))
(
-{
- dev_err(...);
s
-}
|
{
<+...
- dev_err(...);
...+>
}
)
Will any additional constraints become relevant?
>> Would this approach need a version check for the Coccinelle software?
>
> Why would that be necessary?
I guess that the application of SmPL disjunctions for if statements
can trigger development concerns.
Regards,
Markus
> I still don't see the point of specifying return. Why not just S, where S
> is a statement metavariable?
Do you find the following SmPL change specification more appropriate?
@deletion depends on patch@
expression e;
statement s;
@@
e = devm_ioremap_resource(...);
if (IS_ERR(e))
(
-{
- dev_err(...);
s
-}
|
{
<+...
- dev_err(...);
...+>
}
)
Will any additional constraints become relevant?
>> Would this approach need a version check for the Coccinelle software?
>
> Why would that be necessary?
I guess that the application of SmPL disjunctions for if statements
can trigger development concerns.
Regards,
Markus
On Fri, 21 Jun 2019, Markus Elfring wrote:
> > I still don't see the point of specifying return. Why not just S, where S
> > is a statement metavariable?
>
> Do you find the following SmPL change specification more appropriate?
It looks better.
>
> @deletion depends on patch@
> expression e;
> statement s;
> @@
> e = devm_ioremap_resource(...);
> if (IS_ERR(e))
> (
> -{
> - dev_err(...);
> s
> -}
> |
> {
> <+...
> - dev_err(...);
> ...+>
> }
> )
From: Markus Elfring <[email protected]>
Date: Mon, 1 Jul 2019 10:00:39 +0200
The function “devm_ioremap_resource” contains appropriate error reporting.
Thus it can be questionable to present another error message
at other places.
Provide design options for the adjustment of affected source code
by the means of the semantic patch language (Coccinelle software).
Signed-off-by: Markus Elfring <[email protected]>
---
v2:
Suggestions from Julia Lawall were integrated.
* Application of the SmPL construct “<+... … ...+>”
* Replacement of a return specification by a statement metavariable.
* Different coding style for a branch of a SmPL disjunction.
* Usage of a specific function name in two messages.
.../coccinelle/misc/redundant_dev_err.cocci | 62 +++++++++++++++++++
1 file changed, 62 insertions(+)
create mode 100644 scripts/coccinelle/misc/redundant_dev_err.cocci
diff --git a/scripts/coccinelle/misc/redundant_dev_err.cocci b/scripts/coccinelle/misc/redundant_dev_err.cocci
new file mode 100644
index 000000000000..7998defb04f3
--- /dev/null
+++ b/scripts/coccinelle/misc/redundant_dev_err.cocci
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Reconsider a function call for redundant error reporting.
+//
+// Keywords: dev_err redundant device error messages
+// Confidence: Medium
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+@display depends on context@
+expression e;
+@@
+ e = devm_ioremap_resource(...);
+ if (IS_ERR(e))
+ {
+ <+...
+* dev_err(...);
+ ...+>
+ }
+
+@deletion depends on patch@
+expression e;
+statement s;
+@@
+ e = devm_ioremap_resource(...);
+ if (IS_ERR(e))
+(
+-{
+- dev_err(...);
+ s
+-}
+|
+ {
+ <+...
+- dev_err(...);
+ ...+>
+ }
+)
+
+@or depends on org || report@
+expression e;
+position p;
+@@
+ e = devm_ioremap_resource(...);
+ if (IS_ERR(e))
+ {
+ <+... dev_err@p(...); ...+>
+ }
+
+@script:python to_do depends on org@
+p << or.p;
+@@
+coccilib.org.print_todo(p[0],
+ "WARNING: An error message is probably not needed here because the devm_ioremap_resource() function contains appropriate error reporting.")
+
+@script:python reporting depends on report@
+p << or.p;
+@@
+coccilib.report.print_report(p[0],
+ "WARNING: An error message is probably not needed here because the devm_ioremap_resource() function contains appropriate error reporting.")
--
2.22.0
On 01.07.19 10:10, Markus Elfring wrote:
Hi folks,
> +@script:python to_do depends on org@
> +p << or.p;
> +@@
> +coccilib.org.print_todo(p[0],
> + "WARNING: An error message is probably not needed here because the devm_ioremap_resource() function contains appropriate error reporting.")
> +
> +@script:python reporting depends on report@
> +p << or.p;
> +@@
> +coccilib.report.print_report(p[0],
> + "WARNING: An error message is probably not needed here because the devm_ioremap_resource() function contains appropriate error reporting.")
> --
By the way: do we have any mechanism for explicitly suppressing
individual warnings (some kind of annotation), when the maintainer is
sure that some particular case is a false-positive ?
(I'm thinking of something similar to certain #praga directives for
explicitly ignoring invididual warnings in specific lines of code)
I believe such a feature, so we don't get spammed with the same false
positives again and again.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
> By the way: do we have any mechanism for explicitly suppressing
> individual warnings (some kind of annotation),
I do not know such an accepted specification interface
(for the handling together with SmPL scripts) so far.
How would you identify possibly unwanted messages in a safe way?
> when the maintainer is sure that some particular case is a false-positive ?
Do you know any specific source code places already where you would get
concerned about the confidence and relevance of the provided information?
Regards,
Markus
On Mon, 1 Jul 2019, Enrico Weigelt, metux IT consult wrote:
> On 01.07.19 10:10, Markus Elfring wrote:
>
> Hi folks,
>
> > +@script:python to_do depends on org@
> > +p << or.p;
> > +@@
> > +coccilib.org.print_todo(p[0],
> > + "WARNING: An error message is probably not needed here because the devm_ioremap_resource() function contains appropriate error reporting.")
> > +
> > +@script:python reporting depends on report@
> > +p << or.p;
> > +@@
> > +coccilib.report.print_report(p[0],
> > + "WARNING: An error message is probably not needed here because the devm_ioremap_resource() function contains appropriate error reporting.")
> > --
>
> By the way: do we have any mechanism for explicitly suppressing
> individual warnings (some kind of annotation), when the maintainer is
> sure that some particular case is a false-positive ?
> (I'm thinking of something similar to certain #praga directives for
> explicitly ignoring invididual warnings in specific lines of code)
>
> I believe such a feature, so we don't get spammed with the same false
> positives again and again.
0-day takes care of it on its own. Probably other such bots do the same.
I'm not sure that it is a good idea to clutter the kernel code with such
annotations, especially since the whole point of Ccocinelle is that the
rules are easy to change. We also made a tool named Herodotos for
collecting identical reports over time, but it seems to be not so easy to
use.
julia