[cocci] [PATCH] scripts/coccinelle/free: add NULL test before dev_{put, hold} functions

[cocci] [PATCH] scripts/coccinelle/free: add NULL test before dev_{put, hold} functions

[Ziyang Xuan]

Since commit b37a46683739 ("netdevice: add the case if dev is NULL"),
NULL check before dev_{put, hold} functions is not needed.

Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 scripts/coccinelle/free/ifnulldev_put.cocci | 54 +++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 scripts/coccinelle/free/ifnulldev_put.cocci

diff --git a/scripts/coccinelle/free/ifnulldev_put.cocci b/scripts/coccinelle/free/ifnulldev_put.cocci
new file mode 100644
index 000000000000..7ff36e6212ba
--- /dev/null
+++ b/scripts/coccinelle/free/ifnulldev_put.cocci
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// Since commit b37a46683739 ("netdevice: add the case if dev is NULL"),
+/// NULL check before dev_{put, hold} functions is not needed.
+///
+/// Based on ifnullfree.cocci by Fabian Frederick.
+///
+// Copyright: (C) 2022 Ziyang Xuan.
+// Comments: -
+// Options: --no-includes --include-headers
+
+virtual patch
+virtual org
+virtual report
+virtual context
+
+@r2 depends on patch@
+expression E;
+@@
+- if (E != NULL)
+(
+  __dev_put(E);
+|
+  dev_put(E);
+|
+  dev_put_track(E, ...);
+|
+  __dev_hold(E);
+|
+  dev_hold(E);
+|
+  dev_hold_track(E, ...);
+)
+
+@r depends on context || report || org @
+expression E;
+position p;
+@@
+
+* if (E != NULL)
+*	\(__dev_put@p\|dev_put@p\|dev_put_track@p\|__dev_hold@p\|dev_hold@p\|
+*         dev_hold_track@p\)(E, ...);
+
+@script:python depends on org@
+p << r.p;
+@@
+
+cocci.print_main("NULL check before dev_{put, hold} functions is not needed", p)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg = "WARNING: NULL check before dev_{put, hold} functions is not needed."
+coccilib.report.print_report(p[0], msg)
-- 
2.25.1

Re: [cocci] [PATCH] scripts/coccinelle/free: add NULL test before dev_{put, hold} functions

[Markus Elfring]

I find the patch subject improvable.
You would like to detect the remaining usage of redundant null pointer checks
before selected function calls.
Thus it would be nice if the corresponding description can become clearer
another bit, wouldn't it?


> Since commit b37a46683739 ("netdevice: add the case if dev is NULL"),
> NULL check before dev_{put, hold} functions is not needed.
>
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
>  scripts/coccinelle/free/ifnulldev_put.cocci | 54 +++++++++++++++++++++


How do you think about to achieve further collateral evolution?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b37a466837393af72fe8bcb8f1436410f3f173f3

Will it become feasible to combine any more source code analysis approaches?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/ifnullfree.cocci?id=8c23f235a6a8ae43abea215812eb9d8cf4dd165e#n2


> +// Comments: -


Can such a line be omitted?



> +@r2 depends on patch@
> +expression E;
> +@@
> +- if (E != NULL)
> +(
> +  __dev_put(E);
> +|
> +  dev_put(E);
> +|
> +  dev_put_track(E, ...);
> +|
> +  __dev_hold(E);
> +|
> +  dev_hold(E);
> +|
> +  dev_hold_track(E, ...);
> +)


Did you order the case distinctions in the SmPL disjunction according to
the call frequencies of the mentioned function names?

Otherwise, I imagine that another software design option can be considered for
the application of nested disjunctions by the means of the semantic patch language.

Code variant:

@r2 depends on patch@
expression E;
@@
-if (E != NULL)
(
(__dev_put
|dev_put
|__dev_hold
|dev_hold
)(E)
|
(dev_put_track
|dev_hold_track
)(E, ...)
)
;


Regards,
Markus

Re: [cocci] [PATCH] scripts/coccinelle/free: add NULL test before dev_{put, hold} functions

[Ziyang Xuan (William)]

> I find the patch subject improvable.
> You would like to detect the remaining usage of redundant null pointer checks
> before selected function calls.
> Thus it would be nice if the corresponding description can become clearer
> another bit, wouldn't it?

Sorry, I did not get your mean. Did you mean I should point out the selected function?

And the following is one warning, what would be like as your suggestion?

./drivers/infiniband/core/lag.c:98:2-10: WARNING: NULL check before dev_{put, hold} functions is not needed.

>
>
>> Since commit b37a46683739 ("netdevice: add the case if dev is NULL"),
>> NULL check before dev_{put, hold} functions is not needed.
>>
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>> ---
>>  scripts/coccinelle/free/ifnulldev_put.cocci | 54 +++++++++++++++++++++
>
> How do you think about to achieve further collateral evolution?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b37a466837393af72fe8bcb8f1436410f3f173f3
>
> Will it become feasible to combine any more source code analysis approaches?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/ifnullfree.cocci?id=8c23f235a6a8ae43abea215812eb9d8cf4dd165e#n2
>
>
>> +// Comments: -
>
> Can such a line be omitted?
>
It's ok.
>
>> +@r2 depends on patch@
>> +expression E;
>> +@@
>> +- if (E != NULL)
>> +(
>> +  __dev_put(E);
>> +|
>> +  dev_put(E);
>> +|
>> +  dev_put_track(E, ...);
>> +|
>> +  __dev_hold(E);
>> +|
>> +  dev_hold(E);
>> +|
>> +  dev_hold_track(E, ...);
>> +)
>
> Did you order the case distinctions in the SmPL disjunction according to
> the call frequencies of the mentioned function names?

No, no any special, just list the related functions.

>
> Otherwise, I imagine that another software design option can be considered for
> the application of nested disjunctions by the means of the semantic patch language.
>
> Code variant:
>
> @r2 depends on patch@
> expression E;
> @@
> -if (E != NULL)
> (
> (__dev_put
> |dev_put
> |__dev_hold
> |dev_hold
> )(E)
> |
> (dev_put_track
> |dev_hold_track
> )(E, ...)
> )
> ;
It's ok.
>
> Regards,
> Markus
>
> .

Re: [cocci] scripts/coccinelle/free: add NULL test before dev_{put, hold} functions

[Markus Elfring]

>> I find the patch subject improvable.
>> You would like to detect the remaining usage of redundant null pointer checks
>> before selected function calls.
>> Thus it would be nice if the corresponding description can become clearer
>> another bit, wouldn't it?
> Sorry, I did not get your mean.


How do you think about a wording like the following?

Commit subject:
[PATCH] coccinelle: Add detection for redundant null pointer checks before dev_{put,hold} function calls

Commit description:
The information “Add the case if dev is NULL in dev_{put, hold},
so the caller doesn't need to care whether dev is NULL or not.”
was provided by the commit b37a466837393af72fe8bcb8f1436410f3f173f3
("netdevice: add the case if dev is NULL").
Thus extend source code analyses and corresponding transformations
by the means of the semantic patch language so that null pointer checks
which became unnecessary at other places because of the mentioned input
parameter validation.


>> Did you order the case distinctions in the SmPL disjunction according to
>> the call frequencies of the mentioned function names?
> No, no any special, just list the related functions.

Will this view influence the selection which SmPL code variant
will be preferred finally?


Do you get further software development ideas from a data processing approach
which I published before?
https://build.opensuse.org/package/show/home:elfring:semantic_patching:Deletion_of_checks_before_specific_function_calls/Deletion_of_checks_before_specific_function_calls_in_Linux_source_files

Regards,
Markus

Re: [cocci] scripts/coccinelle/free: add NULL test before dev_{put, hold} functions

[Ziyang Xuan (William)]

>>> I find the patch subject improvable.
>>> You would like to detect the remaining usage of redundant null pointer checks
>>> before selected function calls.
>>> Thus it would be nice if the corresponding description can become clearer
>>> another bit, wouldn't it?
>> Sorry, I did not get your mean.
>
> How do you think about a wording like the following?
>
> Commit subject:
> [PATCH] coccinelle: Add detection for redundant null pointer checks before dev_{put,hold} function calls
>
> Commit description:
> The information “Add the case if dev is NULL in dev_{put, hold},
> so the caller doesn't need to care whether dev is NULL or not.”
> was provided by the commit b37a466837393af72fe8bcb8f1436410f3f173f3
> ("netdevice: add the case if dev is NULL").
> Thus extend source code analyses and corresponding transformations
> by the means of the semantic patch language so that null pointer checks
> which became unnecessary at other places because of the mentioned input
> parameter validation.

Patch title looks like better. And I think the main destination of the description is

to tell others what you want to do. If it can, I think it's OK. After all, everyone's style

is different. But I will refer to your suggestions. Thank you!

>
>>> Did you order the case distinctions in the SmPL disjunction according to
>>> the call frequencies of the mentioned function names?
>> No, no any special, just list the related functions.
> Will this view influence the selection which SmPL code variant
> will be preferred finally?
>
>
> Do you get further software development ideas from a data processing approach
> which I published before?
> https://build.opensuse.org/package/show/home:elfring:semantic_patching:Deletion_of_checks_before_specific_function_calls/Deletion_of_checks_before_specific_function_calls_in_Linux_source_files

Sorry, I didn't learn about these. I learn about coccinelle recently by coccinelle scripts in Linux

kernel and coccinelle official website. Now I get more resources. Thank you!


I will give the v2 patch with your suggestions. Thank you!

> Regards,
> Markus
> .

Re: [cocci] scripts/coccinelle/free: add NULL test before dev_{put, hold} functions

[Markus Elfring]

>> Do you get further software development ideas from a data processing approach
>> which I published before?
>> https://build.opensuse.org/package/show/home:elfring:semantic_patching:Deletion_of_checks_before_specific_function_calls/Deletion_of_checks_before_specific_function_calls_in_Linux_source_files
> Sorry, I didn't learn about these. I learn about coccinelle recently by coccinelle scripts in Linux
> kernel and coccinelle official website. Now I get more resources.

My data processing approach can point special run time characteristics out.
The available computation resources influence if all Linux source files
can be analysed (or transformed) within several minutes (or some hours).



> I will give the v2 patch with your suggestions.


Would it become more appropriate to rename the file “scripts/coccinelle/free/ifnullfree.cocci”
so that more redundant checks before selected function calls will be reconsidered
(in a consistent way)?
(Further change possibilities are in waiting queues also according to a function
like “IS_ERR_OR_NULL”, aren't they?)

Regards,
Markus

Re: [cocci] [PATCH] scripts/coccinelle/free: add NULL test before dev_{put, hold} functions

[Julia Lawall]

On Thu, 16 Jun 2022, Ziyang Xuan wrote:

> Since commit b37a46683739 ("netdevice: add the case if dev is NULL"),
> NULL check before dev_{put, hold} functions is not needed.

Applied.

I also added a Version min comment to indicate that this only applies
starting in v5.15.

julia


>
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
>  scripts/coccinelle/free/ifnulldev_put.cocci | 54 +++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 scripts/coccinelle/free/ifnulldev_put.cocci
>
> diff --git a/scripts/coccinelle/free/ifnulldev_put.cocci b/scripts/coccinelle/free/ifnulldev_put.cocci
> new file mode 100644
> index 000000000000..7ff36e6212ba
> --- /dev/null
> +++ b/scripts/coccinelle/free/ifnulldev_put.cocci
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/// Since commit b37a46683739 ("netdevice: add the case if dev is NULL"),
> +/// NULL check before dev_{put, hold} functions is not needed.
> +///
> +/// Based on ifnullfree.cocci by Fabian Frederick.
> +///
> +// Copyright: (C) 2022 Ziyang Xuan.
> +// Comments: -
> +// Options: --no-includes --include-headers
> +
> +virtual patch
> +virtual org
> +virtual report
> +virtual context
> +
> +@r2 depends on patch@
> +expression E;
> +@@
> +- if (E != NULL)
> +(
> +  __dev_put(E);
> +|
> +  dev_put(E);
> +|
> +  dev_put_track(E, ...);
> +|
> +  __dev_hold(E);
> +|
> +  dev_hold(E);
> +|
> +  dev_hold_track(E, ...);
> +)
> +
> +@r depends on context || report || org @
> +expression E;
> +position p;
> +@@
> +
> +* if (E != NULL)
> +*	\(__dev_put@p\|dev_put@p\|dev_put_track@p\|__dev_hold@p\|dev_hold@p\|
> +*         dev_hold_track@p\)(E, ...);
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +cocci.print_main("NULL check before dev_{put, hold} functions is not needed", p)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +msg = "WARNING: NULL check before dev_{put, hold} functions is not needed."
> +coccilib.report.print_report(p[0], msg)
> --
> 2.25.1
>
>