Adapt description, warning message and MODE=patch according to the latest
Documentation/filesystems/sysfs.rst:
> show() should only use sysfs_emit() or sysfs_emit_at() when formatting
> the value to be returned to user space.
After this patch:
When MODE=report,
$ make coccicheck COCCI=scripts/coccinelle/api/device_attr_show.cocci M=drivers/hid/hid-picolcd_core.c MODE=report
<...snip...>
drivers/hid/hid-picolcd_core.c:304:8-16: WARNING: please use sysfs_emit or sysfs_emit_at
drivers/hid/hid-picolcd_core.c:259:9-17: WARNING: please use sysfs_emit or sysfs_emit_at
When MODE=patch,
$ make coccicheck COCCI=scripts/coccinelle/api/device_attr_show.cocci M=drivers/hid/hid-picolcd_core.c MODE=patch
<...snip...>
diff -u -p a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -255,10 +255,12 @@ static ssize_t picolcd_operation_mode_sh
{
struct picolcd_data *data = dev_get_drvdata(dev);
- if (data->status & PICOLCD_BOOTLOADER)
- return snprintf(buf, PAGE_SIZE, "[bootloader] lcd\n");
- else
- return snprintf(buf, PAGE_SIZE, "bootloader [lcd]\n");
+ if (data->status & PICOLCD_BOOTLOADER) {
+ return sysfs_emit(buf, "[bootloader] lcd\n");
+ }
+ else {
+ return sysfs_emit(buf, "bootloader [lcd]\n");
+ }
}
static ssize_t picolcd_operation_mode_store(struct device *dev,
@@ -301,7 +303,7 @@ static ssize_t picolcd_operation_mode_de
{
struct picolcd_data *data = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE, "hello world\n");
+ return sysfs_emit(buf, "hello world\n");
}
static ssize_t picolcd_operation_mode_delay_store(struct device *dev,
CC: Julia Lawall <[email protected]>
CC: Nicolas Palix <[email protected]>
CC: [email protected]
Signed-off-by: Li Zhijian <[email protected]>
---
V2:
- changed title from coccinelle: device_attr_show.cocci: update description and warning message
- Fix MODE=patch
- Extract patch from the patch set[1] so that maintainer can accept it separately.
[1] https://lore.kernel.org/lkml/[email protected]/
---
scripts/coccinelle/api/device_attr_show.cocci | 22 +++++++++++--------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/scripts/coccinelle/api/device_attr_show.cocci b/scripts/coccinelle/api/device_attr_show.cocci
index a28dc061653a..634514937e63 100644
--- a/scripts/coccinelle/api/device_attr_show.cocci
+++ b/scripts/coccinelle/api/device_attr_show.cocci
@@ -1,10 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-only
///
/// From Documentation/filesystems/sysfs.rst:
-/// show() must not use snprintf() when formatting the value to be
-/// returned to user space. If you can guarantee that an overflow
-/// will never happen you can use sprintf() otherwise you must use
-/// scnprintf().
+/// show() should only use sysfs_emit() or sysfs_emit_at() when formatting
+/// the value to be returned to user space.
///
// Confidence: High
// Copyright: (C) 2020 Denis Efremov ISPRAS
@@ -30,15 +28,21 @@ ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
@rp depends on patch@
identifier show, dev, attr, buf;
+expression BUF, SZ, FORMAT, STR;
@@
ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
+(
return
-- snprintf
-+ scnprintf
- (...);
+- snprintf(BUF, SZ, FORMAT, STR);
++ sysfs_emit(BUF, FORMAT, STR);
+|
+ return
+- snprintf(BUF, SZ, STR);
++ sysfs_emit(BUF, STR);
+)
...>
}
@@ -46,10 +50,10 @@ ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
p << r.p;
@@
-coccilib.report.print_report(p[0], "WARNING: use scnprintf or sprintf")
+coccilib.report.print_report(p[0], "WARNING: please use sysfs_emit or sysfs_emit_at")
@script: python depends on org@
p << r.p;
@@
-coccilib.org.print_todo(p[0], "WARNING: use scnprintf or sprintf")
+coccilib.org.print_todo(p[0], "WARNING: please use sysfs_emit or sysfs_emit_at")
--
2.29.2
On Fri, 19 Jan 2024, Li Zhijian wrote:
> Adapt description, warning message and MODE=patch according to the latest
> Documentation/filesystems/sysfs.rst:
> > show() should only use sysfs_emit() or sysfs_emit_at() when formatting
> > the value to be returned to user space.
>
> After this patch:
> When MODE=report,
> $ make coccicheck COCCI=scripts/coccinelle/api/device_attr_show.cocci M=drivers/hid/hid-picolcd_core.c MODE=report
> <...snip...>
> drivers/hid/hid-picolcd_core.c:304:8-16: WARNING: please use sysfs_emit or sysfs_emit_at
> drivers/hid/hid-picolcd_core.c:259:9-17: WARNING: please use sysfs_emit or sysfs_emit_at
>
> When MODE=patch,
> $ make coccicheck COCCI=scripts/coccinelle/api/device_attr_show.cocci M=drivers/hid/hid-picolcd_core.c MODE=patch
> <...snip...>
> diff -u -p a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -255,10 +255,12 @@ static ssize_t picolcd_operation_mode_sh
> {
> struct picolcd_data *data = dev_get_drvdata(dev);
>
> - if (data->status & PICOLCD_BOOTLOADER)
> - return snprintf(buf, PAGE_SIZE, "[bootloader] lcd\n");
> - else
> - return snprintf(buf, PAGE_SIZE, "bootloader [lcd]\n");
> + if (data->status & PICOLCD_BOOTLOADER) {
> + return sysfs_emit(buf, "[bootloader] lcd\n");
> + }
> + else {
> + return sysfs_emit(buf, "bootloader [lcd]\n");
> + }
> }
>
> static ssize_t picolcd_operation_mode_store(struct device *dev,
> @@ -301,7 +303,7 @@ static ssize_t picolcd_operation_mode_de
> {
> struct picolcd_data *data = dev_get_drvdata(dev);
>
> - return snprintf(buf, PAGE_SIZE, "hello world\n");
> + return sysfs_emit(buf, "hello world\n");
> }
>
> static ssize_t picolcd_operation_mode_delay_store(struct device *dev,
>
> CC: Julia Lawall <[email protected]>
> CC: Nicolas Palix <[email protected]>
> CC: [email protected]
> Signed-off-by: Li Zhijian <[email protected]>
> ---
> V2:
> - changed title from coccinelle: device_attr_show.cocci: update description and warning message
> - Fix MODE=patch
> - Extract patch from the patch set[1] so that maintainer can accept it separately.
Applied.
I subsequently simplified the patch case to contain the following:
- snprintf(BUF, SZ, FORMAT
+ sysfs_emit(BUF, FORMAT
,...);
This also works for the case where there are only three arguments.
It has the benefit that the change is recognized as a line replacement, so
no extra {} are added when the call is in an if branch.
julia
> [1] https://lore.kernel.org/lkml/[email protected]/
> ---
> scripts/coccinelle/api/device_attr_show.cocci | 22 +++++++++++--------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/coccinelle/api/device_attr_show.cocci b/scripts/coccinelle/api/device_attr_show.cocci
> index a28dc061653a..634514937e63 100644
> --- a/scripts/coccinelle/api/device_attr_show.cocci
> +++ b/scripts/coccinelle/api/device_attr_show.cocci
> @@ -1,10 +1,8 @@
> // SPDX-License-Identifier: GPL-2.0-only
> ///
> /// From Documentation/filesystems/sysfs.rst:
> -/// show() must not use snprintf() when formatting the value to be
> -/// returned to user space. If you can guarantee that an overflow
> -/// will never happen you can use sprintf() otherwise you must use
> -/// scnprintf().
> +/// show() should only use sysfs_emit() or sysfs_emit_at() when formatting
> +/// the value to be returned to user space.
> ///
> // Confidence: High
> // Copyright: (C) 2020 Denis Efremov ISPRAS
> @@ -30,15 +28,21 @@ ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
>
> @rp depends on patch@
> identifier show, dev, attr, buf;
> +expression BUF, SZ, FORMAT, STR;
> @@
>
> ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> +(
> return
> -- snprintf
> -+ scnprintf
> - (...);
> +- snprintf(BUF, SZ, FORMAT, STR);
> ++ sysfs_emit(BUF, FORMAT, STR);
> +|
> + return
> +- snprintf(BUF, SZ, STR);
> ++ sysfs_emit(BUF, STR);
> +)
> ...>
> }
>
> @@ -46,10 +50,10 @@ ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
> p << r.p;
> @@
>
> -coccilib.report.print_report(p[0], "WARNING: use scnprintf or sprintf")
> +coccilib.report.print_report(p[0], "WARNING: please use sysfs_emit or sysfs_emit_at")
>
> @script: python depends on org@
> p << r.p;
> @@
>
> -coccilib.org.print_todo(p[0], "WARNING: use scnprintf or sprintf")
> +coccilib.org.print_todo(p[0], "WARNING: please use sysfs_emit or sysfs_emit_at")
> --
> 2.29.2
>
>
> I subsequently simplified the patch case to contain the following:
>
> - snprintf(BUF, SZ, FORMAT
> + sysfs_emit(BUF, FORMAT
> ,...);
>
> This also works for the case where there are only three arguments.
> It has the benefit that the change is recognized as a line replacement, so
> no extra {} are added when the call is in an if branch.
Would you like to consider the application of the following SmPL code variant?
-snprintf
+sysfs_emit
(BUF,
- SZ,
FORMAT,
...
);
Regards,
Markus
On Sun, 21 Jan 2024, Markus Elfring wrote:
> > I subsequently simplified the patch case to contain the following:
> >
> > - snprintf(BUF, SZ, FORMAT
> > + sysfs_emit(BUF, FORMAT
> > ,...);
> >
> > This also works for the case where there are only three arguments.
> > It has the benefit that the change is recognized as a line replacement, so
> > no extra {} are added when the call is in an if branch.
>
> Would you like to consider the application of the following SmPL code variant?
>
> -snprintf
> +sysfs_emit
> (BUF,
> - SZ,
> FORMAT,
> ...
> );
It's ok too.
julia
>
> Regards,
> Markus
>
>> Would you like to consider the application of the following SmPL code variant?
>>
>> -snprintf
>> +sysfs_emit
>> (BUF,
>> - SZ,
>> FORMAT,
>> ...
>> );
>
> It's ok too.
Are you going to integrate such a transformation approach?
Regards,
Markus
On Sun, 21 Jan 2024, Markus Elfring wrote:
> >> Would you like to consider the application of the following SmPL code variant?
> >>
> >> -snprintf
> >> +sysfs_emit
> >> (BUF,
> >> - SZ,
> >> FORMAT,
> >> ...
> >> );
> >
> > It's ok too.
>
> Are you going to integrate such a transformation approach?
No. I already pushed the commit for Linux v6.8, and I don't think the
change has any practical impact.
julia
>>>> -snprintf
>>>> +sysfs_emit
>>>> (BUF,
>>>> - SZ,
>>>> FORMAT,
>>>> ...
>>>> );
>>>
>>> It's ok too.
>>
>> Are you going to integrate such a transformation approach?
>
> No.
This is a pity.
> I already pushed the commit for Linux v6.8,
I hope then that the change acceptance for the “beautification”
and refinement of SmPL code can grow for subsequent versions.
> and I don't think the change has any practical impact.
I got an other development view for further fine-tuning possibilities.
Regards,
Markus
On 21/01/2024 05:20, Julia Lawall wrote:
>> V2:
>> - changed title from coccinelle: device_attr_show.cocci: update description and warning message
>> - Fix MODE=patch
>> - Extract patch from the patch set[1] so that maintainer can accept it separately.
> Applied.
>
> I subsequently simplified the patch case to contain the following:
>
> - snprintf(BUF, SZ, FORMAT
> + sysfs_emit(BUF, FORMAT
> ,...);
Great, that was what i want. But i didn't get the correct syntax. I wrote it wrongly:
- snprintf(BUF, SZ, FORMAT, ...);
+ sysfs_emit(BUF, SZ, FORMAT, ...);
>
> This also works for the case where there are only three arguments.
> It has the benefit that the change is recognized as a line replacement, so
> no extra {} are added when the call is in an if branch.
>>> It's ok too.
>>
>> Are you going to integrate such a transformation approach?
>
> No. I already pushed the commit for Linux v6.8,
I suggest to reconsider such a view together with the metavariable “STR”
which became obsolete in the discussed SmPL script.
> and I don't think the change has any practical impact.
Can you ever get into the mood to increase the granularity of
a source code change specification a bit more?
Regards,
Markus