* [RFC v2] checkpatch: detect missing changes to trace-events
@ 2020-08-07 11:14 Claudio Fontana
2020-08-07 11:22 ` no-reply
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Claudio Fontana @ 2020-08-07 11:14 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael S . Tsirkin, Philippe Mathieu-Daudé, qemu-devel,
Claudio Fontana, Stefan Hajnoczi, Paolo Bonzini, Alex Bennée
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);
+
} 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
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC v2] checkpatch: detect missing changes to trace-events
2020-08-07 11:14 [RFC v2] checkpatch: detect missing changes to trace-events Claudio Fontana
@ 2020-08-07 11:22 ` no-reply
2020-08-12 15:51 ` Stefan Hajnoczi
2020-09-02 15:14 ` Markus Armbruster
2 siblings, 0 replies; 6+ messages in thread
From: no-reply @ 2020-08-07 11:22 UTC (permalink / raw)
To: cfontana
Cc: mst, alex.bennee, qemu-devel, armbru, cfontana, stefanha,
pbonzini, philmd
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC v2] checkpatch: detect missing changes to trace-events
2020-08-07 11:14 [RFC v2] checkpatch: detect missing changes to trace-events Claudio Fontana
2020-08-07 11:22 ` no-reply
@ 2020-08-12 15:51 ` Stefan Hajnoczi
2020-08-12 16:08 ` Claudio Fontana
2020-09-02 15:14 ` Markus Armbruster
2 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-08-12 15:51 UTC (permalink / raw)
To: Claudio Fontana
Cc: Michael S . Tsirkin, Philippe Mathieu-Daudé, qemu-devel,
Markus Armbruster, Paolo Bonzini, Alex Bennée
[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC v2] checkpatch: detect missing changes to trace-events
2020-08-12 15:51 ` Stefan Hajnoczi
@ 2020-08-12 16:08 ` Claudio Fontana
0 siblings, 0 replies; 6+ messages in thread
From: Claudio Fontana @ 2020-08-12 16:08 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Michael S . Tsirkin, Philippe Mathieu-Daudé, qemu-devel,
Markus Armbruster, Paolo Bonzini, Alex Bennée
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC v2] checkpatch: detect missing changes to trace-events
2020-08-07 11:14 [RFC v2] checkpatch: detect missing changes to trace-events Claudio Fontana
2020-08-07 11:22 ` no-reply
2020-08-12 15:51 ` Stefan Hajnoczi
@ 2020-09-02 15:14 ` Markus Armbruster
2020-09-02 16:40 ` Claudio Fontana
2 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2020-09-02 15:14 UTC (permalink / raw)
To: Claudio Fontana
Cc: Michael S . Tsirkin, Philippe Mathieu-Daudé, qemu-devel,
Stefan Hajnoczi, Paolo Bonzini, Alex Bennée
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC v2] checkpatch: detect missing changes to trace-events
2020-09-02 15:14 ` Markus Armbruster
@ 2020-09-02 16:40 ` Claudio Fontana
0 siblings, 0 replies; 6+ messages in thread
From: Claudio Fontana @ 2020-09-02 16:40 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael S . Tsirkin, Philippe Mathieu-Daudé, qemu-devel,
Stefan Hajnoczi, Paolo Bonzini, Alex Bennée
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-02 16:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-07 11:14 [RFC v2] checkpatch: detect missing changes to trace-events Claudio Fontana
2020-08-07 11:22 ` no-reply
2020-08-12 15:51 ` Stefan Hajnoczi
2020-08-12 16:08 ` Claudio Fontana
2020-09-02 15:14 ` Markus Armbruster
2020-09-02 16:40 ` Claudio Fontana
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).