linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] coccinelle: api: add sprintf() support to device_attr_show
       [not found] <ed9a8046-4c21-e849-f68b-9e08991b701d@web.de>
@ 2020-08-14  8:50 ` Denis Efremov
       [not found]   ` <48ffa436-6e73-88b2-07bc-89942f3c6d8e@web.de>
  0 siblings, 1 reply; 3+ messages in thread
From: Denis Efremov @ 2020-08-14  8:50 UTC (permalink / raw)
  To: Markus Elfring, Julia Lawall; +Cc: Coccinelle, linux-kernel, kernel-janitors

Hi,

Markus, I think that CCing new people and spam them with mails they
are obviously not interested in doesn't bring an additional value to
the discussion. linux-kernel and cocci mailing lists are enough
in my opinion. This also will allow us to keep "threaded" mail
order.

On 8/14/20 11:30 AM, Markus Elfring wrote:
>> Interesting enough that with this patch coccinelle starts to skip
>> patch generation in some cases. For example, it skips patch for
>> drivers/base/core.c This is an unexpected result for me.
> 
> Would you like to point questionable differences for such patch hunks out?

Without this patch the script generates:
$ spatch -D patch --no-includes --include-headers --cocci-file scripts/coccinelle/api/device_attr_show.cocci drivers/base/core.c
--- drivers/base/core.c
+++ /tmp/cocci-output-63510-2f17ff-core.c
@@ -1713,7 +1713,7 @@ ssize_t device_show_ulong(struct device
char *buf)
{
struct dev_ext_attribute *ea = to_ext_attr(attr);
-       return snprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
+       return scnprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_ulong);

@@ -1743,7 +1743,7 @@ ssize_t device_show_int(struct device *d
{
struct dev_ext_attribute *ea = to_ext_attr(attr);

-       return snprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
+       return scnprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_int);

@@ -1764,7 +1764,7 @@ ssize_t device_show_bool(struct device *
{
struct dev_ext_attribute *ea = to_ext_attr(attr);

-       return snprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
+       return scnprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_bool);

With this patch it generates nothing. I would expect spatch to generate
a different patch with sprintf instead of scnprintf, because I think 
... is enough to match "*(int *)(ea->var)". Even if it can't match sprintf
pattern it should fallback to scnprintf pattern.

> You propose to use a nested SmPL disjunction for desired adjustments.
> I suggest to start a corresponding case distinction behind
> the key word “return” instead of repeating it three times.

It doesn't work.

Thanks,
Denis

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] coccinelle: api: add sprintf() support to device_attr_show
       [not found]   ` <48ffa436-6e73-88b2-07bc-89942f3c6d8e@web.de>
@ 2020-08-14 14:36     ` Denis Efremov
  0 siblings, 0 replies; 3+ messages in thread
From: Denis Efremov @ 2020-08-14 14:36 UTC (permalink / raw)
  To: Markus Elfring, Coccinelle; +Cc: Julia Lawall, linux-kernel, kernel-janitors



On 8/14/20 3:30 PM, Markus Elfring wrote:
>>> You propose to use a nested SmPL disjunction for desired adjustments.
>>> I suggest to start a corresponding case distinction behind
>>> the key word “return” instead of repeating it three times.
>>
>> It doesn't work.
> 
> How do you think about to apply a SmPL rule variant like the following?
> 
> @rp depends on patch@
> identifier show, dev, attr, buf;
> constant str;
> @@
> 
> ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> 	<...
> 	return
> (
> -		snprintf
> +		sprintf
> 			(
> 			buf,
> -			\(PAGE_SIZE\|PAGE_SIZE - 1\),
> (			str
> |
> (			"%i"\|"%i\n"\|"%li"\|"%li\n"\|"%lli"\|"%lli\n"\|
> 			"%d"\|"%d\n"\|"%ld"\|"%ld\n"\|"%lld"\|"%lld\n"\|
> 			"%u"\|"%u\n"\|"%lu"\|"%lu\n"\|"%llu"\|"%llu\n"\|
> 			"%x"\|"%x\n"\|"%lx"\|"%lx\n"\|"%llx"\|"%llx\n"\|
> 			"%X"\|"%X\n"\|"%lX"\|"%lX\n"\|"%llX"\|"%llX\n"\|
> 			"0x%x"\|"0x%x\n"\|"0x%lx"\|"0x%lx\n"\|"0x%llx"\|"0x%llx\n"\|
> 			"0x%X"\|"0x%X\n"\|"0x%lX"\|"0x%lX\n"\|"0x%llX"\|"0x%llX\n"\|
> 			"%02x\n"\|"%03x\n"\|"%04x\n"\|"%08x\n"\|
> 			"%02X\n"\|"%03X\n"\|"%04X\n"\|"%08X\n"\|
> 			"0x%02x\n"\|"0x%03x\n"\|"0x%04x\n"\|"0x%08x\n"\|
> 			"0x%02X\n"\|"0x%03X\n"\|"0x%04X\n"\|"0x%08X\n"\|
> 			"%zd"\|"%zd\n"\|"%zu"\|"%zu\n"\|"%zx"\|"%zx\n"\|
> 			"%c"\|"%c\n"\|"%p"\|"%p\n"\|"%pU\n"\|"%pUl\n"\|"%hu\n"
> )			,
> 			...
> )
> 			)
> |
> -		snprintf
> +		scnprintf
> 			(...)
> );
> 	...>
> }
> 

3 levels of nested disjunctions makes this pattern completely unreadable
and gives no comparable benefits. I don't think we should care much about
number of characters in the kernel sources, gzip will do a better job
anyway.


Thanks,
Denis

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH] coccinelle: api: add sprintf() support to device_attr_show
@ 2020-08-13 22:49 Denis Efremov
  0 siblings, 0 replies; 3+ messages in thread
From: Denis Efremov @ 2020-08-13 22:49 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Denis Efremov, cocci, linux-kernel

It's safe to use sprintf() for simple cases in device_attr_show
type of functions. Add support for sprintf() in patch mode to
the device_attr_show.cocci script to print numbers and pointers.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
Interesting enough that with this patch coccinelle starts to skip
patch generation in some cases. For example, it skips patch for
drivers/base/core.c This is an unexpected result for me.

 scripts/coccinelle/api/device_attr_show.cocci | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/scripts/coccinelle/api/device_attr_show.cocci b/scripts/coccinelle/api/device_attr_show.cocci
index d8ec4bb8ac41..1248b8c76cfe 100644
--- a/scripts/coccinelle/api/device_attr_show.cocci
+++ b/scripts/coccinelle/api/device_attr_show.cocci
@@ -30,15 +30,45 @@ ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
 
 @rp depends on patch@
 identifier show, dev, attr, buf;
+constant str;
 @@
 
 ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	<...
+(
+	return
+-		snprintf
++		sprintf
+			(buf,
+-			\(PAGE_SIZE\|PAGE_SIZE - 1\),
+			str);
+|
+	return
+-		snprintf
++		sprintf
+			(buf,
+-			\(PAGE_SIZE\|PAGE_SIZE - 1\),
+			\("%i"\|"%i\n"\|"%li"\|"%li\n"\|"%lli"\|"%lli\n"\|
+			  "%d"\|"%d\n"\|"%ld"\|"%ld\n"\|"%lld"\|"%lld\n"\|
+			  "%u"\|"%u\n"\|"%lu"\|"%lu\n"\|"%llu"\|"%llu\n"\|
+			  "%x"\|"%x\n"\|"%lx"\|"%lx\n"\|"%llx"\|"%llx\n"\|
+			  "%X"\|"%X\n"\|"%lX"\|"%lX\n"\|"%llX"\|"%llX\n"\|
+			  "0x%x"\|"0x%x\n"\|"0x%lx"\|"0x%lx\n"\|"0x%llx"\|"0x%llx\n"\|
+			  "0x%X"\|"0x%X\n"\|"0x%lX"\|"0x%lX\n"\|"0x%llX"\|"0x%llX\n"\|
+			  "%02x\n"\|"%03x\n"\|"%04x\n"\|"%08x\n"\|
+			  "%02X\n"\|"%03X\n"\|"%04X\n"\|"%08X\n"\|
+			  "0x%02x\n"\|"0x%03x\n"\|"0x%04x\n"\|"0x%08x\n"\|
+			  "0x%02X\n"\|"0x%03X\n"\|"0x%04X\n"\|"0x%08X\n"\|
+			  "%zd"\|"%zd\n"\|"%zu"\|"%zu\n"\|"%zx"\|"%zx\n"\|
+			  "%c"\|"%c\n"\|"%p"\|"%p\n"\|"%pU\n"\|"%pUl\n"\|"%hu\n"\),
+			...);
+|
 	return
 -		snprintf
 +		scnprintf
 			(...);
+)
 	...>
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-08-14 14:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <ed9a8046-4c21-e849-f68b-9e08991b701d@web.de>
2020-08-14  8:50 ` [PATCH] coccinelle: api: add sprintf() support to device_attr_show Denis Efremov
     [not found]   ` <48ffa436-6e73-88b2-07bc-89942f3c6d8e@web.de>
2020-08-14 14:36     ` Denis Efremov
2020-08-13 22:49 Denis Efremov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).