| Message ID | 20200807111447.15599-1-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2] checkpatch: detect missing changes to trace-events | expand |
Patchew URL: https://patchew.org/QEMU/20200807111447.15599-1-cfontana@suse.de/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200807111447.15599-1-cfontana@suse.de/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Fri, Aug 07, 2020 at 01:14:47PM +0200, Claudio Fontana wrote: > # Check for added, moved or deleted files > - if (!$reported_maintainer_file && !$in_commit_log && > + if (!$in_commit_log && > ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ || > $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ || > ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ && > (defined($1) || defined($2))))) { > - $reported_maintainer_file = 1; > - WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); > + if (!$reported_maintainer_file) { > + $reported_maintainer_file = 1; > + WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); > + } > + if (!$reported_trace_events_file) { > + if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') { Are there false positives on non-C files (e.g. Makefiles)? The search expressions can be tightened to avoid false positives (at the cost of possible false negatives): -e '#include "trace.h"' -e '#include "trace-root.h"'. This way a C file containing "strace.handler" will not cause a false positive. I wonder if there is a native Perl way to do this search instead of forking grep :). Nevermind though.
On 8/12/20 5:51 PM, Stefan Hajnoczi wrote: > On Fri, Aug 07, 2020 at 01:14:47PM +0200, Claudio Fontana wrote: >> # Check for added, moved or deleted files >> - if (!$reported_maintainer_file && !$in_commit_log && >> + if (!$in_commit_log && >> ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ || >> $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ || >> ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ && >> (defined($1) || defined($2))))) { >> - $reported_maintainer_file = 1; >> - WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); >> + if (!$reported_maintainer_file) { >> + $reported_maintainer_file = 1; >> + WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); >> + } >> + if (!$reported_trace_events_file) { >> + if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') { > > Are there false positives on non-C files (e.g. Makefiles)? > > The search expressions can be tightened to avoid false positives (at the > cost of possible false negatives): -e '#include "trace.h"' -e '#include > "trace-root.h"'. This way a C file containing "strace.handler" will not > cause a false positive. > Yep good point. > I wonder if there is a native Perl way to do this search instead of > forking grep :). Nevermind though. > If only all the speedups from GNU grep would be available as a libgrep.. I had to post an RFC v3 of this one, because there is an issue in the order in_commit_log is set and checked (in my understanding). https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01953.html This is actually potentially an issue in upstream (kernel) checkpatch.pl as well, but it does not bite until you try to use realfile variable (or in this case fromfile also). Ciao, Claudio
Claudio Fontana <cfontana@suse.de> writes: > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > scripts/checkpatch.pl | 37 ++++++++++++++++++++++++++++++------- > 1 file changed, 30 insertions(+), 7 deletions(-) > > v1 -> v2 : > > * track the "from" file in addition to the "to" file, > and grep into both (if they exist), looking for trace.h, trace-root.h > > If files are reachable and readable, emit a warning if there is no > update to trace-events. > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index bd3faa154c..37db212fc6 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -1300,6 +1300,7 @@ sub process { > my $in_header_lines = $file ? 0 : 1; > my $in_commit_log = 0; #Scanning lines before patch > my $reported_maintainer_file = 0; > + my $reported_trace_events_file = 0; > my $non_utf8_charset = 0; > > our @report = (); > @@ -1309,6 +1310,7 @@ sub process { > our $cnt_chk = 0; > > # Trace the real file/line as we go. > + my $fromfile = ''; > my $realfile = ''; > my $realline = 0; > my $realcnt = 0; > @@ -1454,10 +1456,15 @@ sub process { > $here = "#$realline: " if ($file); > > # extract the filename as it passes > - if ($line =~ /^diff --git.*?(\S+)$/) { > - $realfile = $1; > - $realfile =~ s@^([^/]*)/@@ if (!$file); > + if ($line =~ /^diff --git.*?(\S+).*?(\S+)$/) { > + $fromfile = $1; > + $realfile = $2; > + if (!$file) { > + $fromfile =~ s@^([^/]*)/@@ ; > + $realfile =~ s@^([^/]*)/@@ ; > + } > checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected); > + Drop this blank line. > } elsif ($line =~ /^\+\+\+\s+(\S+)/) { > $realfile = $1; > $realfile =~ s@^([^/]*)/@@ if (!$file); > @@ -1470,6 +1477,11 @@ sub process { > } > > next; > + And this one. > + } elsif ($line =~ /^---\s+(\S+)/) { > + $fromfile = $1; > + $fromfile =~ s@^([^/]*)/@@ if (!$file); > + next; > } > > $here .= "FILE: $realfile:$realline:" if ($realcnt != 0); Aside: I don't understand why we need to match both the diff line and the --- line (and now the +++ line, too). > @@ -1524,15 +1536,26 @@ sub process { > if ($line =~ /^\s*MAINTAINERS\s*\|/) { > $reported_maintainer_file = 1; > } > - > +# similar check for trace-events > + if ($line =~ /^\s*trace-events\s*\|/) { > + $reported_trace_events_file = 1; > + } These are meant to match in the diffstat (took me a stare to figure that out). The existing one matches MAINTAINERS in the source root. Good; that's where it is. The new one matches trace-events in the source root. Not so good; We use one trace-events per directory. If I update trace-events in the source root, I won't be warned about other trace-events in need of updating (false negative), will I? If I don't update trace-events in the source root, I will be warned regardless of what other trace-events I update (false positive), won't I? > # Check for added, moved or deleted files > - if (!$reported_maintainer_file && !$in_commit_log && > + if (!$in_commit_log && > ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ || > $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ || > ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ && > (defined($1) || defined($2))))) { > - $reported_maintainer_file = 1; > - WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); > + if (!$reported_maintainer_file) { > + $reported_maintainer_file = 1; > + WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); > + } > + if (!$reported_trace_events_file) { > + if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') { > + $reported_trace_events_file = 1; > + WARN("added, moved or deleted file(s), does trace-events need updating?\n" . $herecurr); > + } > + } > } > > # Check for wrappage within a valid hunk of the file Regarding Stefan's observations: * Yes, the grep patterns need tightening. * Yes, forking grep could be replaced by a simple function that slurps in the file and searches it. Could doesn't imply should, let alome must. As discussed in review of v1, I'm not sure checkpatch.pl is the best home for this kind of check. I'm not going to reject a working patch because of that.
On 9/2/20 5:14 PM, Markus Armbruster wrote: > Claudio Fontana <cfontana@suse.de> writes: > >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- >> scripts/checkpatch.pl | 37 ++++++++++++++++++++++++++++++------- >> 1 file changed, 30 insertions(+), 7 deletions(-) >> >> v1 -> v2 : >> >> * track the "from" file in addition to the "to" file, >> and grep into both (if they exist), looking for trace.h, trace-root.h >> >> If files are reachable and readable, emit a warning if there is no >> update to trace-events. >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index bd3faa154c..37db212fc6 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -1300,6 +1300,7 @@ sub process { >> my $in_header_lines = $file ? 0 : 1; >> my $in_commit_log = 0; #Scanning lines before patch >> my $reported_maintainer_file = 0; >> + my $reported_trace_events_file = 0; >> my $non_utf8_charset = 0; >> >> our @report = (); >> @@ -1309,6 +1310,7 @@ sub process { >> our $cnt_chk = 0; >> >> # Trace the real file/line as we go. >> + my $fromfile = ''; >> my $realfile = ''; >> my $realline = 0; >> my $realcnt = 0; >> @@ -1454,10 +1456,15 @@ sub process { >> $here = "#$realline: " if ($file); >> >> # extract the filename as it passes >> - if ($line =~ /^diff --git.*?(\S+)$/) { >> - $realfile = $1; >> - $realfile =~ s@^([^/]*)/@@ if (!$file); >> + if ($line =~ /^diff --git.*?(\S+).*?(\S+)$/) { >> + $fromfile = $1; >> + $realfile = $2; >> + if (!$file) { >> + $fromfile =~ s@^([^/]*)/@@ ; >> + $realfile =~ s@^([^/]*)/@@ ; >> + } >> checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected); >> + > > Drop this blank line. > >> } elsif ($line =~ /^\+\+\+\s+(\S+)/) { >> $realfile = $1; >> $realfile =~ s@^([^/]*)/@@ if (!$file); >> @@ -1470,6 +1477,11 @@ sub process { >> } >> >> next; >> + > > And this one. > >> + } elsif ($line =~ /^---\s+(\S+)/) { >> + $fromfile = $1; >> + $fromfile =~ s@^([^/]*)/@@ if (!$file); >> + next; >> } >> >> $here .= "FILE: $realfile:$realline:" if ($realcnt != 0); > > Aside: I don't understand why we need to match both the diff line and > the --- line (and now the +++ line, too). > >> @@ -1524,15 +1536,26 @@ sub process { >> if ($line =~ /^\s*MAINTAINERS\s*\|/) { >> $reported_maintainer_file = 1; >> } >> - >> +# similar check for trace-events >> + if ($line =~ /^\s*trace-events\s*\|/) { >> + $reported_trace_events_file = 1; >> + } > > These are meant to match in the diffstat (took me a stare to figure that > out). > > The existing one matches MAINTAINERS in the source root. Good; that's > where it is. > > The new one matches trace-events in the source root. Not so good; We > use one trace-events per directory. > > If I update trace-events in the source root, I won't be warned about > other trace-events in need of updating (false negative), will I? > > If I don't update trace-events in the source root, I will be warned > regardless of what other trace-events I update (false positive), won't > I? > >> # Check for added, moved or deleted files >> - if (!$reported_maintainer_file && !$in_commit_log && >> + if (!$in_commit_log && >> ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ || >> $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ || >> ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ && >> (defined($1) || defined($2))))) { >> - $reported_maintainer_file = 1; >> - WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); >> + if (!$reported_maintainer_file) { >> + $reported_maintainer_file = 1; >> + WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); >> + } >> + if (!$reported_trace_events_file) { >> + if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') { >> + $reported_trace_events_file = 1; >> + WARN("added, moved or deleted file(s), does trace-events need updating?\n" . $herecurr); >> + } >> + } >> } >> >> # Check for wrappage within a valid hunk of the file > > Regarding Stefan's observations: > > * Yes, the grep patterns need tightening. > > * Yes, forking grep could be replaced by a simple function that slurps > in the file and searches it. Could doesn't imply should, let alome > must. > > As discussed in review of v1, I'm not sure checkpatch.pl is the best > home for this kind of check. I'm not going to reject a working patch > because of that. > Hi Marcus, I will rethink this in the future, thanks for the useful feedback, but I think this needs to be rethought, reworked and tested more. Thanks! Claudio
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index bd3faa154c..37db212fc6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1300,6 +1300,7 @@ sub process { my $in_header_lines = $file ? 0 : 1; my $in_commit_log = 0; #Scanning lines before patch my $reported_maintainer_file = 0; + my $reported_trace_events_file = 0; my $non_utf8_charset = 0; our @report = (); @@ -1309,6 +1310,7 @@ sub process { our $cnt_chk = 0; # Trace the real file/line as we go. + my $fromfile = ''; my $realfile = ''; my $realline = 0; my $realcnt = 0; @@ -1454,10 +1456,15 @@ sub process { $here = "#$realline: " if ($file); # extract the filename as it passes - if ($line =~ /^diff --git.*?(\S+)$/) { - $realfile = $1; - $realfile =~ s@^([^/]*)/@@ if (!$file); + if ($line =~ /^diff --git.*?(\S+).*?(\S+)$/) { + $fromfile = $1; + $realfile = $2; + if (!$file) { + $fromfile =~ s@^([^/]*)/@@ ; + $realfile =~ s@^([^/]*)/@@ ; + } checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected); + } elsif ($line =~ /^\+\+\+\s+(\S+)/) { $realfile = $1; $realfile =~ s@^([^/]*)/@@ if (!$file); @@ -1470,6 +1477,11 @@ sub process { } next; + + } elsif ($line =~ /^---\s+(\S+)/) { + $fromfile = $1; + $fromfile =~ s@^([^/]*)/@@ if (!$file); + next; } $here .= "FILE: $realfile:$realline:" if ($realcnt != 0); @@ -1524,15 +1536,26 @@ sub process { if ($line =~ /^\s*MAINTAINERS\s*\|/) { $reported_maintainer_file = 1; } - +# similar check for trace-events + if ($line =~ /^\s*trace-events\s*\|/) { + $reported_trace_events_file = 1; + } # Check for added, moved or deleted files - if (!$reported_maintainer_file && !$in_commit_log && + if (!$in_commit_log && ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ || $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ || ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ && (defined($1) || defined($2))))) { - $reported_maintainer_file = 1; - WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); + if (!$reported_maintainer_file) { + $reported_maintainer_file = 1; + WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr); + } + if (!$reported_trace_events_file) { + if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') { + $reported_trace_events_file = 1; + WARN("added, moved or deleted file(s), does trace-events need updating?\n" . $herecurr); + } + } } # Check for wrappage within a valid hunk of the file
Signed-off-by: Claudio Fontana <cfontana@suse.de> --- scripts/checkpatch.pl | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) v1 -> v2 : * track the "from" file in addition to the "to" file, and grep into both (if they exist), looking for trace.h, trace-root.h If files are reachable and readable, emit a warning if there is no update to trace-events.