[PATCH v4] checkpatch: debugfs_remove() can take NULL

[PATCH v4] checkpatch: debugfs_remove() can take NULL

[Constantine Shulyupin]

From: Constantine Shulyupin <const@MakeLinux.com>

debugfs_remove() and  debugfs_remove_recursive() can take a NULL, so let's check and warn about that.

Changes since v3, as Joe Perches suggested:
- removed redundant check

Changes since v2, as Joe Perches suggested:
- match whitespace around argument

Changes since v1, as Joe Perches suggested:
- added debugfs_remove_recursive
- all tests for patterns are "if (a) xxx(a)" are consolidated

Signed-off-by: Constantine Shulyupin <const@MakeLinux.com>
---
 scripts/checkpatch.pl |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f18750e..76ad9f2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3213,21 +3213,25 @@ sub process {
 				$herecurr);
 		}
 
+# check for needless "if (<foo>) fn(<foo>)" uses
+		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+
 # check for needless kfree() checks
-		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
-			my $expr = $1;
-			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
+			if ($line =~ /\bkfree$expr/) {
 				WARN("NEEDLESS_KFREE",
 				     "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
-		}
 # check for needless usb_free_urb() checks
-		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
-			my $expr = $1;
-			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
+			if ($line =~ /\busb_free_urb$expr/) {
 				WARN("NEEDLESS_USB_FREE_URB",
 				     "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
+# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
+			if ($line =~ /\b(debugfs_remove(?:_recursive)?)$expr/) {
+				WARN("NEEDLESS_DEBUGFS_REMOVE",
+				     "$1(NULL) is safe this check is probably not required\n" . $hereprev);
+			}
 		}
 
 # prefer usleep_range over udelay
-- 
1.7.9.5

Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL

[Andy Whitcroft]

On Sun, Nov 18, 2012 at 12:20:18AM +0200, Constantine Shulyupin wrote:
> From: Constantine Shulyupin <const@MakeLinux.com>
> 
> debugfs_remove() and  debugfs_remove_recursive() can take a NULL, so let's check and warn about that.
> 
> Changes since v3, as Joe Perches suggested:
> - removed redundant check
> 
> Changes since v2, as Joe Perches suggested:
> - match whitespace around argument
> 
> Changes since v1, as Joe Perches suggested:
> - added debugfs_remove_recursive
> - all tests for patterns are "if (a) xxx(a)" are consolidated
> 
> Signed-off-by: Constantine Shulyupin <const@MakeLinux.com>
> ---
>  scripts/checkpatch.pl |   18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f18750e..76ad9f2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3213,21 +3213,25 @@ sub process {
>  				$herecurr);
>  		}
>  
> +# check for needless "if (<foo>) fn(<foo>)" uses
> +		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> +			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> +
>  # check for needless kfree() checks
> -		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> -			my $expr = $1;
> -			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
> +			if ($line =~ /\bkfree$expr/) {
>  				WARN("NEEDLESS_KFREE",
>  				     "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
>  			}
> -		}
>  # check for needless usb_free_urb() checks
> -		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> -			my $expr = $1;
> -			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
> +			if ($line =~ /\busb_free_urb$expr/) {
>  				WARN("NEEDLESS_USB_FREE_URB",
>  				     "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
>  			}
> +# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
> +			if ($line =~ /\b(debugfs_remove(?:_recursive)?)$expr/) {
> +				WARN("NEEDLESS_DEBUGFS_REMOVE",
> +				     "$1(NULL) is safe this check is probably not required\n" . $hereprev);
> +			}
>  		}
>  
>  # prefer usleep_range over udelay

This all looks sensible, though we still have three blocks doing the
same thing.  How about we standardise this check into a single check,
generating the capacity from the matched name.

Something like the below on top of V4.

-apw

>From 676ce396a349f2242f80e9b2c5fb68584ec09f14 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <apw@canonical.com>
Date: Tue, 20 Nov 2012 14:27:59 +0000
Subject: [PATCH] checkpatch: consolidate if (foo) bar(NULL) checks

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 313617b..6660246 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3229,19 +3229,16 @@ sub process {
 			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
 
 # check for needless kfree() checks
-			if ($line =~ /\bkfree$expr/) {
-				WARN("NEEDLESS_KFREE",
-				     "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
-			}
 # check for needless usb_free_urb() checks
-			if ($line =~ /\busb_free_urb$expr/) {
-				WARN("NEEDLESS_USB_FREE_URB",
-				     "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
-			}
 # check for needless debugfs_remove() and debugfs_remove_recursive*() checks
-			if ($line =~ /\b(debugfs_remove(?:_recursive)?)$expr/) {
-				WARN("NEEDLESS_DEBUGFS_REMOVE",
-				     "$1(NULL) is safe this check is probably not required\n" . $hereprev);
+			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
+				my $func = $1;
+				my $func_capacity = "NEEDLESS_$1";
+
+				$func_capacity =~ s/(.$)/\U$1\E/;
+
+				WARN($func_capacity,
+				     "$func(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
 		}
 
-- 
1.7.10.4

Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL

[Joe Perches]

On Tue, 2012-11-20 at 14:29 +0000, Andy Whitcroft wrote:

> This all looks sensible, though we still have three blocks doing the
> same thing.  How about we standardise this check into a single check,
> generating the capacity from the matched name.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> +			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> +				my $func = $1;
> +				my $func_capacity = "NEEDLESS_$1";
> +
> +				$func_capacity =~ s/(.$)/\U$1\E/;
> +
> +				WARN($func_capacity,
> +				     "$func(NULL) is safe this check is probably not required\n" . $hereprev);

Perhaps just
				WARN("NEEDLESS_IF",
...

Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL

[Andy Whitcroft]

On Tue, Nov 20, 2012 at 06:43:49AM -0800, Joe Perches wrote:
> On Tue, 2012-11-20 at 14:29 +0000, Andy Whitcroft wrote:
> 
> > This all looks sensible, though we still have three blocks doing the
> > same thing.  How about we standardise this check into a single check,
> > generating the capacity from the matched name.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > +			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> > +				my $func = $1;
> > +				my $func_capacity = "NEEDLESS_$1";
> > +
> > +				$func_capacity =~ s/(.$)/\U$1\E/;
> > +
> > +				WARN($func_capacity,
> > +				     "$func(NULL) is safe this check is probably not required\n" . $hereprev);
> 
> Perhaps just
> 				WARN("NEEDLESS_IF",

I would cirtainly be happy with that, I was trying to avoid changing the
capacity for the existing NEEDLESS_KFREE.  If compatibility there isn't
an issue then that makes life even simpler.

-apw

Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL

[Constantine Shulyupin]

>> On Tue, 2012-11-20 at 14:29 +0000, Andy Whitcroft wrote:
>> Perhaps just
>>                               WARN("NEEDLESS_IF",
>
> I would cirtainly be happy with that, I was trying to avoid changing the
> capacity for the existing NEEDLESS_KFREE.  If compatibility there isn't
> an issue then that makes life even simpler.
>
> -apw

Right.

Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL

[Joe Perches]

On Tue, 2012-11-20 at 16:50 +0200, Constantine Shulyupin wrote:
> >> On Tue, 2012-11-20 at 14:29 +0000, Andy Whitcroft wrote:
> >> Perhaps just
> >>                               WARN("NEEDLESS_IF",
> >
> > I would cirtainly be happy with that, I was trying to avoid changing the
> > capacity for the existing NEEDLESS_KFREE.  If compatibility there isn't
> > an issue then that makes life even simpler.
> >
> > -apw
> 
> Right.

I don't think it's an issue.  I'm not sure anyone
really uses --ignore for much other than LONG_LINE.

Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL

[Constantine Shulyupin]

On Tue, Nov 20, 2012 at 4:58 PM, Joe Perches <joe@perches.com> wrote:
> I don't think it's an issue.  I'm not sure anyone
> really uses --ignore for much other than LONG_LINE.

Indeed. Must all code be 80 characters width?  My be my be better to
make line length configurable than ignore it? Or at least print actual
line length to allow developer be aware how long lines is.
I think 90 char line is preferable than wrapped two or even three times line.

Thanks

Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL

[Andy Whitcroft]

On Tue, Nov 20, 2012 at 05:10:43PM +0200, Constantine Shulyupin wrote:
> On Tue, Nov 20, 2012 at 4:58 PM, Joe Perches <joe@perches.com> wrote:
> > I don't think it's an issue.  I'm not sure anyone
> > really uses --ignore for much other than LONG_LINE.
> 
> Indeed. Must all code be 80 characters width?  My be my be better to
> make line length configurable than ignore it? Or at least print actual
> line length to allow developer be aware how long lines is.
> I think 90 char line is preferable than wrapped two or even three times line.

If wrapping there is that much better then wrapping there is appropriate
in some cases.  The currently limit has been revisited a bunch of times,
and in none have we agreed on what to change it to.  It therefore
remains the recommendation.

-apw

[PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove

[Andy Whitcroft]

Consolidate the if (foo) bar(foo) detectors into a single check.  Add
debugfs_remove and family.

Based on a patch by Constantine Shulyupin <const@MakeLinux.com>.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

This is a fully merged version as the fix patch was bigger than the
whole merged patch and this is much clearer as to purpose.  Hope that
makes sense.
	
-apw

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ae01b90..e83a137 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3239,20 +3239,16 @@ sub process {
 				$herecurr);
 		}
 
+# check for needless "if (<foo>) fn(<foo>)" uses
+		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+
 # check for needless kfree() checks
-		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
-			my $expr = $1;
-			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
-				WARN("NEEDLESS_KFREE",
-				     "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
-			}
-		}
 # check for needless usb_free_urb() checks
-		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
-			my $expr = $1;
-			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
-				WARN("NEEDLESS_USB_FREE_URB",
-				     "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
+# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
+			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
+				WARN('NEEDLESS_IF',
+				     "$1(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
 		}
 
-- 
1.7.10.4

Re: [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove

[Joe Perches]

On Tue, 2012-11-20 at 15:37 +0000, Andy Whitcroft wrote:
> Consolidate the if (foo) bar(foo) detectors into a single check.  Add
> debugfs_remove and family.
> 
> Based on a patch by Constantine Shulyupin <const@MakeLinux.com>.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
>  
> +# check for needless "if (<foo>) fn(<foo>)" uses
> +		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> +			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> +
[]
> +# check for needless debugfs_remove() and debugfs_remove_recursive*() checks

Hey Andy, that's an incomplete comment.
Just remove it.

> +			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> +				WARN('NEEDLESS_IF',
> +				     "$1(NULL) is safe this check is probably not required\n" . $hereprev);
>  			}
>  		}
>  

Re: [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove

[Andy Whitcroft]

On Tue, Nov 20, 2012 at 07:51:17AM -0800, Joe Perches wrote:
> On Tue, 2012-11-20 at 15:37 +0000, Andy Whitcroft wrote:
> > Consolidate the if (foo) bar(foo) detectors into a single check.  Add
> > debugfs_remove and family.
> > 
> > Based on a patch by Constantine Shulyupin <const@MakeLinux.com>.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> >  
> > +# check for needless "if (<foo>) fn(<foo>)" uses
> > +		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> > +			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> > +
> []
> > +# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
> 
> Hey Andy, that's an incomplete comment.
> Just remove it.

Oh it is meant to drop next to the other comments from the preceeding
hunks which are being removed, it should end up looking like this:

# check for needless kfree() checks
# check for needless usb_free_urb() checks
# check for needless debugfs_remove() and debugfs_remove_recursive*() checks

Admitedly the trailing checks on each are a little redundant, but it is
intended to retain the list of functions affected.

> 
> > +			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> > +				WARN('NEEDLESS_IF',
> > +				     "$1(NULL) is safe this check is probably not required\n" . $hereprev);
> >  			}
> >  		}
> >  

-apw

Re: [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove

[Joe Perches]

On Tue, 2012-11-20 at 16:39 +0000, Andy Whitcroft wrote:
> On Tue, Nov 20, 2012 at 07:51:17AM -0800, Joe Perches wrote:
> > On Tue, 2012-11-20 at 15:37 +0000, Andy Whitcroft wrote:
> > > Consolidate the if (foo) bar(foo) detectors into a single check.  Add
> > > debugfs_remove and family.
> > > 
> > > Based on a patch by Constantine Shulyupin <const@MakeLinux.com>.
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > >  
> > > +# check for needless "if (<foo>) fn(<foo>)" uses
> > > +		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> > > +			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> > > +
> > []
> > > +# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
> > 
> > Hey Andy, that's an incomplete comment.
> > Just remove it.
> 
> Oh it is meant to drop next to the other comments from the preceeding
> hunks which are being removed, it should end up looking like this:
> 
> # check for needless kfree() checks
> # check for needless usb_free_urb() checks
> # check for needless debugfs_remove() and debugfs_remove_recursive*() checks
> 
> Admitedly the trailing checks on each are a little redundant, but it is
> intended to retain the list of functions affected.

I think all of those are unnecessary.
Self documenting code is better right?

If not, I'd remove the "check for needless", leave the function,
and remove the trailing "checks"

btw: the * after debugfs_remove_recursive should be removed too.

cheers, Joe

[PATCH V2] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove

[Andy Whitcroft]

Consolidate the if (foo) bar(foo) detectors into a single check.  Add
debugfs_remove and family.

Based on a patch by Constantine Shulyupin <const@MakeLinux.com>.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ae01b90..a1e2471 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3239,20 +3239,12 @@ sub process {
 				$herecurr);
 		}
 
-# check for needless kfree() checks
-		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
-			my $expr = $1;
-			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
-				WARN("NEEDLESS_KFREE",
-				     "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
-			}
-		}
-# check for needless usb_free_urb() checks
-		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
-			my $expr = $1;
-			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
-				WARN("NEEDLESS_USB_FREE_URB",
-				     "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
+# check for needless "if (<foo>) fn(<foo>)" uses
+		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
+				WARN('NEEDLESS_IF',
+				     "$1(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
 		}
 
-- 
1.7.10.4

[PATCH] checkpatch: Allow control over line length warning, default remains 80

[Joe Perches]

Some projects might want a longer line length so allow
a command line --max-line-length=n control over the
long line warnings.  The default line length is 80.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d2d5ba1..38e8890 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -33,6 +33,7 @@ my %ignore_type = ();
 my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
+my $max_line_length = 80;
 
 sub help {
 	my ($exitcode) = @_;
@@ -51,6 +52,7 @@ Options:
   -f, --file                 treat FILE as regular source file
   --subjective, --strict     enable more subjective tests
   --ignore TYPE(,TYPE2...)   ignore various comma separated message types
+  --max-line-length=n        set the maximum line length, if exceeded, warn
   --show-types               show the message "types" in the output
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
@@ -107,6 +109,7 @@ GetOptions(
 	'strict!'	=> \$check,
 	'ignore=s'	=> \@ignore,
 	'show-types!'	=> \$show_types,
+	'max-line-length=i' => \$max_line_length,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
 	'mailback!'	=> \$mailback,
@@ -1760,15 +1763,15 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
+#line length limit
 		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
 		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
 		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
-		    $length > 80)
+		    $length > $max_line_length)
 		{
 			WARN("LONG_LINE",
-			     "line over 80 characters\n" . $herecurr);
+			     "line over $max_line_length characters\n" . $herecurr);
 		}
 
 # Check for user-visible strings broken across lines, which breaks the ability