2012-05-02 15:47:23

by Joe Korty

[permalink] [raw]
Subject: checkpatch: kstrtol fix

kstrtol is a substitute for simple_strtol() only when when second
arg of simple_strtol() is NULL. For any other value the functionality
of simple_strtol() cannot be implemented in terms of kstrtol.

So modify checkpatch.pl so that it prints out the following warning only
if the second argument is null:

WARNING: simple_strtol is obsolete, use kstrtol instead

Signed-off-by: Joe Korty <[email protected]>

Index: linux/scripts/checkpatch.pl
===================================================================
--- linux.orig/scripts/checkpatch.pl 2012-05-02 10:38:45.000000000 -0400
+++ linux/scripts/checkpatch.pl 2012-05-02 10:47:58.000000000 -0400
@@ -3359,8 +3359,8 @@
"consider using a completion\n" . $herecurr);
}

-# recommend kstrto* over simple_strto* and strict_strto*
- if ($line =~ /\b((simple|strict)_(strto(l|ll|ul|ull)))\s*\(/) {
+# recommend kstrto* over simple_strto* and strict_strto* where appropriate
+ if ($line =~ /\b((simple|strict)_(strto(l|ll|ul|ull)))\s*\([^,]+,\s*NULL\s*,/) {
WARN("CONSIDER_KSTRTO",
"$1 is obsolete, use k$3 instead\n" . $herecurr);
}


2012-05-02 16:15:08

by Guenter Roeck

[permalink] [raw]
Subject: Re: checkpatch: kstrtol fix

On Wed, 2012-05-02 at 11:10 -0400, Joe Korty wrote:
> kstrtol is a substitute for simple_strtol() only when when second
> arg of simple_strtol() is NULL. For any other value the functionality
> of simple_strtol() cannot be implemented in terms of kstrtol.
>
> So modify checkpatch.pl so that it prints out the following warning only
> if the second argument is null:
>
> WARNING: simple_strtol is obsolete, use kstrtol instead
>
> Signed-off-by: Joe Korty <[email protected]>
>

I think this would be a bad idea. Most if not all instances in the
kernel (at least all the ones I looked at) use the second argument to
determine if the parameter was actually a number or not, ie it is used
to detect parameter errors. This is exactly the point of using kstrtol()
in the first place. So it _is_ possible, at least in most cases, to
implement the same functionality with kstrtol(). If there _are_ uses of
simple_strtol() where the second argument is not used for error checking
but for some other purpose, I am sure that an alternative solution can
be found which does not require simple_strtol(). Or just live with the
warning for those cases.

Thanks,
Guenter

2012-05-02 17:13:25

by Joe Korty

[permalink] [raw]
Subject: Re: checkpatch: kstrtol fix

On Wed, May 02, 2012 at 12:13:22PM -0400, Guenter Roeck wrote:
> On Wed, 2012-05-02 at 11:10 -0400, Joe Korty wrote:
>> kstrtol is a substitute for simple_strtol() only when when second
>> arg of simple_strtol() is NULL. For any other value the functionality
>> of simple_strtol() cannot be implemented in terms of kstrtol.
>
> If there _are_ uses of
> simple_strtol() where the second argument is not used for error checking
> but for some other purpose, I am sure that an alternative solution can
> be found which does not require simple_strtol(). Or just live with the
> warning for those cases.
>
> Thanks,
> Guenter

And of course the burning question is, what was the compelling reason
kstrtol wasn't made upwards-compatible with strtol? It just seems
gratituous to have that little bit of incompatibility introduced
into the kernel.

Joe