Skip to contentSkip to conversation

checkpatch: debugfs_remove() can take NULL

PATCH In review Constantine Shulyupin 3 participants 14 messages Nov 17, 2012 – Nov 20, 2012 Checkpatch

lore
Share & tools
lore
#0 lore raw

PATCH v4 1/1 checkpatch: debugfs_remove() can take NULL

From: Constantine Shulyupin <[email protected]>

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

---
 scripts/checkpatch.pl |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
scripts/checkpatch.pl +11 -7
index f18750e..76ad9f2 100755
@@ -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
Signature · 1 line
1.7.9.5
maintainer
#1 lore raw
Quoted 46 lines · Constantine Shulyupin

From: Constantine Shulyupin <[email protected]>

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

---
 scripts/checkpatch.pl |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
scripts/checkpatch.pl +6 -7
index f18750e..76ad9f2 100755
@@ -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",

scripts/checkpatch.pl sub process { · line 3237

Quoted 10 lines
scripts/checkpatch.pl +5 -0
 				     "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

Quoted 1 line

From 676ce396a349f2242f80e9b2c5fb68584ec09f14 Mon Sep 17 00:00:00 2001

From: Andy Whitcroft <[email protected]>
Date: Tue, 20 Nov 2012 14:27:59 +0000
Subject: [PATCH] checkpatch: consolidate if (foo) bar(NULL) checks

---
 scripts/checkpatch.pl |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
scripts/checkpatch.pl +8 -11
index 313617b..6660246 100755
@@ -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);
 			}
 		}
 
Signature · 1 line
1.7.10.4
maintainer
#2 lore raw
Quoted 3 lines · Andy Whitcroft

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.

[]

scripts/checkpatch.pl

Quoted 1 line
scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 3241

Quoted 8 lines
scripts/checkpatch.pl +8 -0
+			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",

...

maintainer
#3 lore raw
Quoted 18 lines · Joe Perches
Quoted 3 lines · Andy Whitcroft

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.

[]

scripts/checkpatch.pl

Quoted 1 line
scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 3241

Quoted 8 lines
scripts/checkpatch.pl +8 -0
+			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

Quoted 1 line
				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

#4 lore raw
Quoted 9 lines
Quoted 3 lines · Andy Whitcroft

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.

maintainer
#5 lore raw
Quoted 11 lines · Constantine Shulyupin
Quoted 9 lines
Quoted 3 lines · Andy Whitcroft

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.

#6 lore raw
Quoted 2 lines · Joe Perches

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

maintainer
#7 lore raw
Quoted 8 lines · Constantine Shulyupin
Quoted 2 lines · Joe Perches

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

maintainer
#8 lore raw

PATCH 1/1 checkpatch: Allow control over line length warning, default remains 80

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.

---
 scripts/checkpatch.pl |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
scripts/checkpatch.pl +6 -3
index d2d5ba1..38e8890 100755
@@ -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
maintainer
#9 lore raw

PATCH 1/1 checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove

In reply to Joe Perches

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

Based on a patch by Constantine Shulyupin <[email protected]>.

---
 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

scripts/checkpatch.pl +8 -12
index ae01b90..e83a137 100755
@@ -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);
 			}
 		}
 
Signature · 1 line
1.7.10.4
maintainer
#10 lore raw

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

Quoted 4 lines · 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 <[email protected]>.

[]

scripts/checkpatch.pl

Quoted 1 line
scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 3245

Quoted 5 lines
scripts/checkpatch.pl +4 -0
+# check for needless "if (<foo>) fn(<foo>)" uses
+		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+

[]

Quoted 1 line

+# check for needless debugfs_remove() and debugfs_remove_recursive*() checks

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

Quoted 6 lines
scripts/checkpatch.pl +3 -0
+			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);
 			}
 		}
maintainer
#11 lore raw

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

Quoted 14 lines · Joe Perches
Quoted 4 lines · 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 <[email protected]>.

[]

scripts/checkpatch.pl

Quoted 1 line
scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 3245

Quoted 5 lines
scripts/checkpatch.pl +4 -0
+# check for needless "if (<foo>) fn(<foo>)" uses
+		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+

[]

Quoted 4 lines
Quoted 1 line

+# 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.

Quoted 7 lines

scripts/checkpatch.pl sub process { · line 3253

Quoted 7 lines
scripts/checkpatch.pl +3 -0
+			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

maintainer
#12 lore raw

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

Quoted 23 lines · Andy Whitcroft
Quoted 14 lines · Joe Perches
Quoted 4 lines · 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 <[email protected]>.

[]

scripts/checkpatch.pl

Quoted 1 line
scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 3245

Quoted 5 lines
scripts/checkpatch.pl +4 -0
+# check for needless "if (<foo>) fn(<foo>)" uses
+		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+

[]

Quoted 4 lines
Quoted 1 line

+# 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:

Quoted 6 lines
# 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

maintainer
#13 lore raw

PATCH v2 1/1 checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove

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

Based on a patch by Constantine Shulyupin <[email protected]>.

---
 scripts/checkpatch.pl |   20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)
scripts/checkpatch.pl +6 -14
index ae01b90..a1e2471 100755
@@ -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);
 			}
 		}
 
Signature · 1 line
1.7.10.4