checkpatch: debugfs_remove() can take NULL
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
@@ -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
Andy Whitcroft
maintainerQuoted 46 lines · Constantine Shulyupin
From: Constantine Shulyupin <[email protected]>
debugfs_remove()anddebugfs_remove_recursive()can take a NULL, so let's check and warn about that.Changes since v3, as Joe Perches suggested:
- removed redundant checkChanges since v2, as Joe Perches suggested:
- match whitespace around argumentChanges since v1, as Joe Perches suggested:
- addeddebugfs_remove_recursive
- all tests for patterns are "if (a) xxx(a)" are consolidatedSigned-off-by Constantine Shulyupin <[email protected]>--- scripts/checkpatch.pl | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)scripts/checkpatch.pl +6
-7@@ -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
676ce396a349f2242f80e9b2c5fb68584ec09f14Mon 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
@@ -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
Joe Perches
maintainerQuoted 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",...
Andy Whitcroft
maintainerQuoted 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
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.
Joe Perches
maintainerQuoted 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.
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
Andy Whitcroft
maintainerQuoted 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
Andy Whitcroft
maintainerPATCH 1/1 checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove
In reply to Joe PerchesConsolidate 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
@@ -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
Joe Perches
maintainerRe: [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_removeand 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); } }
Andy Whitcroft
maintainerRe: [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_removeand 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*() checksHey 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
Joe Perches
maintainerRe: [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_removeand 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*() checksHey 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*() checksAdmitedly 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
Andy Whitcroft
maintainerPATCH 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
@@ -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
Joe Perches
maintainerPATCH 1/1 checkpatch: Allow control over line length warning, default remains 80
In reply to Andy WhitcroftSome 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
@@ -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