[Qemu-devel] [PATCH] checkpatch: detect doubly-encoded UTF-8

[Paolo Bonzini]

Copy and pasting from Thunderbird's "view source" window results in double
encoding of multibyte UTF-8 sequences.  The appearance of those sequences is
very peculiar, so detect it and give an error despite the (low) possibility
of false positives.

As the major offender, I am also adding the same check to my applypatch-msg
and commit-msg hooks, but this will also cause patchew to croak loudly when
this mistake happens.

Signed-off-by: Paolo Bonzini 
---
 scripts/checkpatch.pl | 16 
 1 file changed, 16 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 88682cb..b27e1de 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -262,6 +262,19 @@ our $UTF8  = qr{
| $NON_ASCII_UTF8
 }x;
 
+# some readers default to ISO-8859-1 when showing email source. detect
+# when UTF-8 is incorrectly interpreted as ISO-8859-1 and reencoded back.
+# False positives are possible but very unlikely.
+our $UTF8_MOJIBAKE = qr{
+   \xC3[\x82-\x9F] \xC2[\x80-\xBF]# c2-df 80-bf
+   | \xC3\xA0 \xC2[\xA0-\xBF] \xC2[\x80-\xBF] # e0 a0-bf 80-bf
+   | \xC3[\xA1-\xAC\xAE\xAF] (?: \xC2[\x80-\xBF]){2}  # e1-ec/ee/ef 80-bf 
80-bf
+   | \xC3\xAD \xC2[\x80-\x9F] \xC2[\x80-\xBF] # ed 80-9f 80-bf
+   | \xC3\xB0 \xC2[\x90-\xBF] (?: \xC2[\x80-\xBF]){2} # f0 90-bf 80-bf 
80-bf
+   | \xC3[\xB1-\xB3] (?: \xC2[\x80-\xBF]){3}  # f1-f3 80-bf 80-bf 
80-bf
+   | \xC3\xB4 \xC2[\x80-\x8F] (?: \xC2[\x80-\xBF]){2} # f4 80-b8 80-bf 
80-bf
+}x;
+
 # There are still some false positives, but this catches most
 # common cases.
 our $typeTypedefs = qr{(?x:
@@ -1506,6 +1519,9 @@ sub process {
ERROR("Invalid UTF-8, patch and commit message should 
be encoded in UTF-8\n" . $hereptr);
}
 
+   if ($rawline =~ m/$UTF8_MOJIBAKE/) {
+   ERROR("Doubly-encoded UTF-8\n" . $herecurr);
+   }
 # Check if it's the start of a commit log
 # (not a header line and we haven't seen the patch filename)
if ($in_header_lines && $realfile =~ /^$/ &&
-- 
1.8.3.1

[Qemu-devel] [PATCH] checkpatch: allow SPDX-License-Identifier

[Peter Xu]

According to: https://spdx.org/ids-how, let's still allow QEMU to use
the SPDX license identifier:

// SPDX-License-Identifier: ***

CC: Paolo Bonzini 
Signed-off-by: Peter Xu 
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 88682cb0a9..c2aaf421da 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1949,7 +1949,8 @@ sub process {
}
 
 # no C99 // comments
-   if ($line =~ m{//}) {
+   if ($line =~ m{//} &&
+   $rawline !~ m{// SPDX-License-Identifier: }) {
ERROR("do not use C99 // comments\n" . $herecurr);
}
# Remove C99 comments.
-- 
2.17.1

Re: [Qemu-devel] [PATCH] checkpatch: Flag suspicious attribution lines

[Eric Blake]

On 2/4/19 3:56 PM, Eric Blake wrote:
> Flag commit attribution tags that are unusual (often because they
> were a typo), but only as a warning (because sometimes a humorous
> or otherwise useful tag is intentionally supplied).
> 
> This picks the 6-most popular tags, each with 700 or more uses (well,
> S-o-b was already checked for case-sensitivity and typos, leaving
> only 5 new tags being checked), as determined by:
> $ git log | sed -n 's/^ *\([A-Za-z-]*-by:\).*/\1/p' | \
>   sort | uniq -c | sort -k1,1n | tail
> 
> Most of the rejected lines were obvious typos (among others, we've
> had 4 cases of someone being burnt, based on Singed-off-by; and 2
> cases of list-reading via an e-reader, based on eviewed-by; there
> are also lines forgetting a space after the ':') or otherwise
> tongue-in-check (3 Approximately-suggested-by). A few lines not
> whitelisted here may be legitimate, but as they are orders of
> magnitude rarer, it is therefore not worth worrying about
> (7 Requested-by, 3 Co-authored-by, 1 Inspired-by, etc.).
> 
> Signed-off-by: Eric Blake 
> ---

>   }
> + } elsif($line =~ /^\s*([A-Za-z-])*-by:/ &&
> + ($1 !~ /(Suggest|Report|Test|Ack|Review)ed/ ||
> +  $line !~ /^\s[a-z-]*-by:\S/i)) {

Oops, that last one is supposed to be =~, to flag lines that forget the
space between *-by: and the email address.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] checkpatch: Flag suspicious attribution lines

[Eric Blake]

Flag commit attribution tags that are unusual (often because they
were a typo), but only as a warning (because sometimes a humorous
or otherwise useful tag is intentionally supplied).

This picks the 6-most popular tags, each with 700 or more uses (well,
S-o-b was already checked for case-sensitivity and typos, leaving
only 5 new tags being checked), as determined by:
$ git log | sed -n 's/^ *\([A-Za-z-]*-by:\).*/\1/p' | \
  sort | uniq -c | sort -k1,1n | tail

Most of the rejected lines were obvious typos (among others, we've
had 4 cases of someone being burnt, based on Singed-off-by; and 2
cases of list-reading via an e-reader, based on eviewed-by; there
are also lines forgetting a space after the ':') or otherwise
tongue-in-check (3 Approximately-suggested-by). A few lines not
whitelisted here may be legitimate, but as they are orders of
magnitude rarer, it is therefore not worth worrying about
(7 Requested-by, 3 Co-authored-by, 1 Inspired-by, etc.).

Signed-off-by: Eric Blake 
---
 scripts/checkpatch.pl | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 88682cb0a9f..51d55f80621 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1456,7 +1456,7 @@ sub process {
ERROR("Author email address is mangled by the mailing 
list\n" . $herecurr);
}

-#check the patch for a signoff:
+#check the patch for a signoff, and that other attribution lines are typical:
if ($line =~ /^\s*signed-off-by:/i) {
# This is a signoff, if ugly, so do not double report.
$signoff++;
@@ -1470,6 +1470,10 @@ sub process {
ERROR("space required after Signed-off-by:\n" .
$herecurr);
}
+   } elsif($line =~ /^\s*([A-Za-z-])*-by:/ &&
+   ($1 !~ /(Suggest|Report|Test|Ack|Review)ed/ ||
+$line !~ /^\s[a-z-]*-by:\S/i)) {
+   WARN("suspicious attribution tag:\n" . $herecurr);
}

 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
-- 
2.20.1

Re: [Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Peter Maydell]

On Fri, 18 Jan 2019 at 16:26, Eric Blake  wrote:
>
> On 1/18/19 10:08 AM, Peter Maydell wrote:
>
> >>> The sequence "/\*\*?" was intended to match either "/*" or "/**",
> >>> but Perl's semantics for '?' allow it to backtrack and try the
> >>> "matches 0 chars" option if the "matches 1 char" choice leads to
> >>> a failure of the rest of the regex to match.  Switch to "/\*\*?+"
> >>> which uses what perlre(1) calls the "possessive" quantifier form:
> >>> this means that if it matches the "/**" string it will not later
> >>> backtrack to matching just the "/*" prefix.
> >>
> >> Just wondering if "/\*{1,2}" would also work (it may have to be spelled
> >> "/\*\{1,2}" - I never remember which flavors of regex have which
> >> extensions without rereading docs)
> >
> > Oh yes, that would probably be the less perl-specific way
> > to write it.
>
> Except it would probably fail in the same way as the non-greedy \*? -
> when \*{2} can't satisfy the regex, the engine will retry with \*{1} and
> then see the second * as noise.

OK, so the patch is good as it is, though it doesn't address
the possible incorrect warnings with trailing whitespace.

> > I'm not sure exactly what I was aiming for with that
> > part of the regex when I wrote it. The comment suggests
> > that I was looking for "non-blank", ie I didn't want to
> > fire on /* or /** followed by just trailing whitespace.
> > The regex as written is clearly not actually doing that,
> > though...
>
> Trailing whitespace is generally a problem anyways, though :)

Yes, but checkpatch already checks for that, so we don't want
to report an incorrect "wrong blockquote" warning as well as
the "trailing whitespace" error, which is what the current
code does:

ERROR: trailing whitespace
#159: FILE: hw/arm/mps2-tz.c:391:
+/* $

WARNING: Block comments use a leading /* on a separate line
#159: FILE: hw/arm/mps2-tz.c:391:
+/*

total: 1 errors, 1 warnings, 165 lines checked

thanks
-- PMM

Re: [Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Eric Blake]

On 1/18/19 10:08 AM, Peter Maydell wrote:

>>> The sequence "/\*\*?" was intended to match either "/*" or "/**",
>>> but Perl's semantics for '?' allow it to backtrack and try the
>>> "matches 0 chars" option if the "matches 1 char" choice leads to
>>> a failure of the rest of the regex to match.  Switch to "/\*\*?+"
>>> which uses what perlre(1) calls the "possessive" quantifier form:
>>> this means that if it matches the "/**" string it will not later
>>> backtrack to matching just the "/*" prefix.
>>
>> Just wondering if "/\*{1,2}" would also work (it may have to be spelled
>> "/\*\{1,2}" - I never remember which flavors of regex have which
>> extensions without rereading docs)
> 
> Oh yes, that would probably be the less perl-specific way
> to write it.

Except it would probably fail in the same way as the non-greedy \*? -
when \*{2} can't satisfy the regex, the engine will retry with \*{1} and
then see the second * as noise.


>>>   # Block comments use /* on a line of its own
>>>   if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&  #inline 
>>> /*...*/
>>> - $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** 
>>> non-blank
>>> + $rawline =~ m@^\+.*/\*\*?+[ \t]*.+[ \t]*$@) { # /* or /** 
>>> non-blank
>>
>> Hmm - Isn't "[ \t]*.+[ \t]*$" the same as ".+$?"
> 
> (do you mean '$?"' at the end of your sentence, or '$" ?' ?)

Serves me right for adding "" after the fact.  You are correct, intended
punctuation should be ".+$"? (the 3-character regex, concluded by asking
a question if the 3-character form is the same as the 15-character
form), and where I should have gone further and asked if it was the same
as "." (since who cares whether there are more than one extra characters
between there and the end of the line - once we've found any character
at all, we've already found enough to flag the line).

> 
> I'm not sure exactly what I was aiming for with that
> part of the regex when I wrote it. The comment suggests
> that I was looking for "non-blank", ie I didn't want to
> fire on /* or /** followed by just trailing whitespace.
> The regex as written is clearly not actually doing that,
> though...

Trailing whitespace is generally a problem anyways, though :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Peter Maydell]

On Fri, 18 Jan 2019 at 15:53, Eric Blake  wrote:
>
> On 1/18/19 7:27 AM, Peter Maydell wrote:
> > In checkpatch we attempt to check for and warn about
> > block comments which start with /* or /** followed by a
> > non-blank. Unfortunately a bug in the regex meant that
> > we would incorrectly warn about comments starting with
> > "/**" with no following text:
> >
> >   git show 9813dc6ac3954d58ba16b3920556f106f97e1c67|./scripts/checkpatch.pl 
> > -
> >   WARNING: Block comments use a leading /* on a separate line
> >   #34: FILE: tests/libqtest.h:233:
> >   +/**
> >
> > The sequence "/\*\*?" was intended to match either "/*" or "/**",
> > but Perl's semantics for '?' allow it to backtrack and try the
> > "matches 0 chars" option if the "matches 1 char" choice leads to
> > a failure of the rest of the regex to match.  Switch to "/\*\*?+"
> > which uses what perlre(1) calls the "possessive" quantifier form:
> > this means that if it matches the "/**" string it will not later
> > backtrack to matching just the "/*" prefix.
>
> Just wondering if "/\*{1,2}" would also work (it may have to be spelled
> "/\*\{1,2}" - I never remember which flavors of regex have which
> extensions without rereading docs)

Oh yes, that would probably be the less perl-specific way
to write it.

Perl regexes have the convenient property that backslash
followed by a punctuation character always means "that
character literally", whether or not that punctuation
character has a metacharacter meaning when not escaped.
So it's definitely the non-backslash version.
(As an aside, it's a bit sad that Rust regexes don't have
this property.)

> > This comment check is unique to QEMU checkpatch so the bug
> > doesn't exist in the Linux version.
> > ---
> >  scripts/checkpatch.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d10dddf1be4..5f1ec537d21 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1624,7 +1624,7 @@ sub process {
> >
> >   # Block comments use /* on a line of its own
> >   if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&  #inline 
> > /*...*/
> > - $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** 
> > non-blank
> > + $rawline =~ m@^\+.*/\*\*?+[ \t]*.+[ \t]*$@) { # /* or /** 
> > non-blank
>
> Hmm - Isn't "[ \t]*.+[ \t]*$" the same as ".+$?"

(do you mean '$?"' at the end of your sentence, or '$" ?' ?)

I'm not sure exactly what I was aiming for with that
part of the regex when I wrote it. The comment suggests
that I was looking for "non-blank", ie I didn't want to
fire on /* or /** followed by just trailing whitespace.
The regex as written is clearly not actually doing that,
though...

-- PMM

Re: [Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Eric Blake]

On 1/18/19 7:27 AM, Peter Maydell wrote:
> In checkpatch we attempt to check for and warn about
> block comments which start with /* or /** followed by a
> non-blank. Unfortunately a bug in the regex meant that
> we would incorrectly warn about comments starting with
> "/**" with no following text:
> 
>   git show 9813dc6ac3954d58ba16b3920556f106f97e1c67|./scripts/checkpatch.pl -
>   WARNING: Block comments use a leading /* on a separate line
>   #34: FILE: tests/libqtest.h:233:
>   +/**
> 
> The sequence "/\*\*?" was intended to match either "/*" or "/**",
> but Perl's semantics for '?' allow it to backtrack and try the
> "matches 0 chars" option if the "matches 1 char" choice leads to
> a failure of the rest of the regex to match.  Switch to "/\*\*?+"
> which uses what perlre(1) calls the "possessive" quantifier form:
> this means that if it matches the "/**" string it will not later
> backtrack to matching just the "/*" prefix.

Just wondering if "/\*{1,2}" would also work (it may have to be spelled
"/\*\{1,2}" - I never remember which flavors of regex have which
extensions without rereading docs).  But since your way is tested, I'm
not going to force a respin.

> 
> Reported-by: Thomas Huth 
> Signed-off-by: Peter Maydell 
> ---
> This comment check is unique to QEMU checkpatch so the bug
> doesn't exist in the Linux version.
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d10dddf1be4..5f1ec537d21 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1624,7 +1624,7 @@ sub process {
>  
>   # Block comments use /* on a line of its own
>   if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&  #inline /*...*/
> - $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** 
> non-blank
> + $rawline =~ m@^\+.*/\*\*?+[ \t]*.+[ \t]*$@) { # /* or /** 
> non-blank

Hmm - Isn't "[ \t]*.+[ \t]*$" the same as ".+$?"  Perhaps:

m@^\+.*/\*([^*]|\*.)@

also does the trick in a more legible way (that is, any line that starts
with /* and then contains any additional characters other than a second *)?

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Peter Maydell]

In checkpatch we attempt to check for and warn about
block comments which start with /* or /** followed by a
non-blank. Unfortunately a bug in the regex meant that
we would incorrectly warn about comments starting with
"/**" with no following text:

  git show 9813dc6ac3954d58ba16b3920556f106f97e1c67|./scripts/checkpatch.pl -
  WARNING: Block comments use a leading /* on a separate line
  #34: FILE: tests/libqtest.h:233:
  +/**

The sequence "/\*\*?" was intended to match either "/*" or "/**",
but Perl's semantics for '?' allow it to backtrack and try the
"matches 0 chars" option if the "matches 1 char" choice leads to
a failure of the rest of the regex to match.  Switch to "/\*\*?+"
which uses what perlre(1) calls the "possessive" quantifier form:
this means that if it matches the "/**" string it will not later
backtrack to matching just the "/*" prefix.

Reported-by: Thomas Huth 
Signed-off-by: Peter Maydell 
---
This comment check is unique to QEMU checkpatch so the bug
doesn't exist in the Linux version.
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d10dddf1be4..5f1ec537d21 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1624,7 +1624,7 @@ sub process {
 
# Block comments use /* on a line of its own
if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&  #inline /*...*/
-   $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** 
non-blank
+   $rawline =~ m@^\+.*/\*\*?+[ \t]*.+[ \t]*$@) { # /* or /** 
non-blank
WARN("Block comments use a leading /* on a separate 
line\n" . $herecurr);
}
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH] checkpatch: g_test_message does not need a a trailing newline

[Paolo Bonzini]

On 21/11/18 19:38, Philippe Mathieu-Daudé wrote:
> 
> 
> On 21/11/18 19:27, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini 
>> ---
>>   scripts/checkpatch.pl | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index eccd656c41..d27bc51f8c 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2802,7 +2802,8 @@ sub process {
>>   info_vreport|
>>   error_report|
>>   warn_report|
>> -    info_report}x;
>> +    info_report|
>> +    g_test_message}x;
> 
> This perl file use  here, shouldn't we keep the same style?
> 
>>     if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
>>   ERROR("Error messages should not contain newlines\n" .
>> $herecurr);
>>
> 
> $ ./scripts/checkpatch.pl -f tests/ipmi-bt-test.c
> ERROR: Error messages should not contain newlines
> #408: FILE: tests/ipmi-bt-test.c:408:
> +    g_test_message("Skipping test for non-x86\n");
> total: 1 errors, 0 warnings, 430 lines checked
> 
> Nice :)
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 

Queued then, thanks. :)

Paolo

Re: [Qemu-devel] [PATCH] checkpatch: g_test_message does not need a a trailing newline

[Thomas Huth]

On 2018-11-21 19:38, Philippe Mathieu-Daudé wrote:
> 
> 
> On 21/11/18 19:27, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini 
>> ---
>>   scripts/checkpatch.pl | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index eccd656c41..d27bc51f8c 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2802,7 +2802,8 @@ sub process {
>>   info_vreport|
>>   error_report|
>>   warn_report|
>> -    info_report}x;
>> +    info_report|
>> +    g_test_message}x;
> 
> This perl file use  here, shouldn't we keep the same style?

+1

Apart from that:

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH] checkpatch: g_test_message does not need a a trailing newline

[Philippe Mathieu-Daudé]

On 21/11/18 19:27, Paolo Bonzini wrote:

Signed-off-by: Paolo Bonzini 
---
  scripts/checkpatch.pl | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eccd656c41..d27bc51f8c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2802,7 +2802,8 @@ sub process {
info_vreport|
error_report|
warn_report|
-   info_report}x;
+   info_report|
+g_test_message}x;


This perl file use  here, shouldn't we keep the same style?

  
  	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {

ERROR("Error messages should not contain newlines\n" . 
$herecurr);



$ ./scripts/checkpatch.pl -f tests/ipmi-bt-test.c
ERROR: Error messages should not contain newlines
#408: FILE: tests/ipmi-bt-test.c:408:
+g_test_message("Skipping test for non-x86\n");
total: 1 errors, 0 warnings, 430 lines checked

Nice :)

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

[Qemu-devel] [PATCH] checkpatch: g_test_message does not need a a trailing newline

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eccd656c41..d27bc51f8c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2802,7 +2802,8 @@ sub process {
info_vreport|
error_report|
warn_report|
-   info_report}x;
+   info_report|
+g_test_message}x;
 
if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
ERROR("Error messages should not contain newlines\n" . 
$herecurr);
-- 
2.19.1

[Qemu-devel] [PATCH] checkpatch: handle token pasting better

[Paolo Bonzini]

The mechanism to find possible type tokens can sometimes be confused and go 
into an
infinite loop.  This happens for example in QEMU for a line that looks like

 uint## BITS ##_t S = _S, T = _T;\
 uint## BITS ##_t as, at, xs, xt, xd;\

Because the token pasting operator does not have a space before _t, it does not
match $notPermitted.  However, (?x) is turned on in the regular expression for
modifiers, and thus ##_t matches the empty string.  As a result, annotate_values
goes in an infinite loop.

The solution is simply to remove token pasting operators from the string before
looking for modifiers.  In the example above, the string uintBITS_t will be
evaluated as a candidate modifier.  This is not optimal, but it works as long
as people do not write things like a##s##m, and it fits nicely into sub
possible.

For a similar reason, \# should be rejected always, even if it is not
at end of line or followed by whitespace.

The same patch was sent to the Linux kernel mailing list.

Reported-by: Aleksandar Markovic 
Signed-off-by: Paolo Bonzini 
---
 scripts/checkpatch.pl | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 223681bfd0..42e1c50dd8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1132,11 +1132,10 @@ sub possible {
case|
else|
asm|__asm__|
-   do|
-   \#|
-   \#\#
+   do
)(?:\s|$)|
-   ^(?:typedef|struct|enum)\b
+   ^(?:typedef|struct|enum)\b|
+   ^\#
)}x;
warn "CHECK<$possible> ($line)\n" if ($dbg_possible > 2);
if ($possible !~ $notPermitted) {
@@ -1146,7 +1145,7 @@ sub possible {
if ($possible =~ /^\s*$/) {
 
} elsif ($possible =~ /\s/) {
-   $possible =~ s/\s*$Type\s*//g;
+   $possible =~ s/\s*(?:$Type|\#\#)\s*//g;
for my $modifier (split(' ', $possible)) {
if ($modifier !~ $notPermitted) {
warn "MODIFIER: $modifier ($possible) 
($line)\n" if ($dbg_possible);
-- 
2.17.1

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Thomas Huth]

On 19.04.2018 04:06, Fam Zheng wrote:
> On Mon, 04/16 16:09, Markus Armbruster wrote:
>> Thomas Huth  writes:
>>
>>> On 12.03.2018 14:18, Stefan Hajnoczi wrote:
 Warn if files are added/renamed/deleted without MAINTAINERS file
 changes.  This has helped me in Linux and we could benefit from this
 check in QEMU.

 This patch is a manual cherry-pick of Linux commit
 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
 file add/move/delete") by Joe Perches .

 Signed-off-by: Stefan Hajnoczi 
>>
>> We should really keep upstream's S-o-b intact.  I'd keep the whole
>> commit message intact:
>>
>> From 7fb819c27bf47041a13feed93f86a15bdb2c681f Mon Sep 17 00:00:00 2001
>> From: Joe Perches 
>> Date: Wed, 6 Aug 2014 16:10:59 -0700
>> Subject: [PATCH] checkpatch: emit a warning on file add/move/delete
>>
>> Whenever files are added, moved, or deleted, the MAINTAINERS file
>> patterns can be out of sync or outdated.
>>
>> To try to keep MAINTAINERS more up-to-date, add a one-time warning
>> whenever a patch does any of those.
>>
>> Signed-off-by: Joe Perches 
>> Acked-by: Andy Whitcroft 
>> Signed-off-by: Andrew Morton 
>> Signed-off-by: Linus Torvalds 
>> [Cherry picked from Linux commit 
>> 13f1937ef33950b1112049972249e6191b82e6c9]
>> Signed-off-by: Stefan Hajnoczi 
>>
>> Created by running "git-format-patch -1 13f1937ef33" in the kernel,
>> feeding that to git-am (patch doesn't apply), patch -p1 your patch,
>> git-am --continue, git-commit --amend to add a backporting note and your
>> S-o-b.
>>
 ---
 Note the 80-char lines are from upstream code.  Keep them as-is.

  scripts/checkpatch.pl | 19 +++
  1 file changed, 19 insertions(+)

 diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
 index d1fe79bcc4..d0d8f63d48 100755
 --- a/scripts/checkpatch.pl
 +++ b/scripts/checkpatch.pl
 @@ -1177,6 +1177,7 @@ sub process {
our $clean = 1;
my $signoff = 0;
my $is_patch = 0;
 +  my $reported_maintainer_file = 0;
  
our @report = ();
our $cnt_lines = 0;
 @@ -1379,6 +1380,24 @@ sub process {
}
}
  
 +# Check if MAINTAINERS is being updated.  If so, there's probably no need 
 to
 +# emit the "does MAINTAINERS need updating?" message on file 
 add/move/delete
 +  if ($line =~ /^\s*MAINTAINERS\s*\|/) {
 +  $reported_maintainer_file = 1;
 +  }
 +
 +# Check for added, moved or deleted files
 +  if (!$reported_maintainer_file &&
 +  ($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) {
 +  $is_patch = 1;
 +  $reported_maintainer_file = 1;
 +  WARN("added, moved or deleted file(s), does MAINTAINERS 
 need updating?\n" .
 +  $herecurr);
>>>
>>> Could you please turn this into a notification instead of a warning? For
>>> rationale, please see the discussion of this patch last year:
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html
[...]
> Patchew doesn't speak up unless there is an "error". Warnings and notes are 
> not
> complained about to keep good signal-to-noise ratio.

Ah, ok, didn't know that, that makes sense. Then I'm fine with the code
above. But while you're at it, could you please also include the other
patches that I posted last year, e.g. to warn on wrong utf-8 in the
message? We've had mojibaked commit messages a couple of times already,
and I hope that patch will help to reduce this a little bit...

 Thomas

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Fam Zheng]

On Mon, 04/16 16:09, Markus Armbruster wrote:
> Thomas Huth  writes:
> 
> > On 12.03.2018 14:18, Stefan Hajnoczi wrote:
> >> Warn if files are added/renamed/deleted without MAINTAINERS file
> >> changes.  This has helped me in Linux and we could benefit from this
> >> check in QEMU.
> >> 
> >> This patch is a manual cherry-pick of Linux commit
> >> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
> >> file add/move/delete") by Joe Perches .
> >>
> >> Signed-off-by: Stefan Hajnoczi 
> 
> We should really keep upstream's S-o-b intact.  I'd keep the whole
> commit message intact:
> 
> From 7fb819c27bf47041a13feed93f86a15bdb2c681f Mon Sep 17 00:00:00 2001
> From: Joe Perches 
> Date: Wed, 6 Aug 2014 16:10:59 -0700
> Subject: [PATCH] checkpatch: emit a warning on file add/move/delete
> 
> Whenever files are added, moved, or deleted, the MAINTAINERS file
> patterns can be out of sync or outdated.
> 
> To try to keep MAINTAINERS more up-to-date, add a one-time warning
> whenever a patch does any of those.
> 
> Signed-off-by: Joe Perches 
> Acked-by: Andy Whitcroft 
> Signed-off-by: Andrew Morton 
> Signed-off-by: Linus Torvalds 
> [Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9]
> Signed-off-by: Stefan Hajnoczi 
> 
> Created by running "git-format-patch -1 13f1937ef33" in the kernel,
> feeding that to git-am (patch doesn't apply), patch -p1 your patch,
> git-am --continue, git-commit --amend to add a backporting note and your
> S-o-b.
> 
> >> ---
> >> Note the 80-char lines are from upstream code.  Keep them as-is.
> >> 
> >>  scripts/checkpatch.pl | 19 +++
> >>  1 file changed, 19 insertions(+)
> >> 
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> index d1fe79bcc4..d0d8f63d48 100755
> >> --- a/scripts/checkpatch.pl
> >> +++ b/scripts/checkpatch.pl
> >> @@ -1177,6 +1177,7 @@ sub process {
> >>our $clean = 1;
> >>my $signoff = 0;
> >>my $is_patch = 0;
> >> +  my $reported_maintainer_file = 0;
> >>  
> >>our @report = ();
> >>our $cnt_lines = 0;
> >> @@ -1379,6 +1380,24 @@ sub process {
> >>}
> >>}
> >>  
> >> +# Check if MAINTAINERS is being updated.  If so, there's probably no need 
> >> to
> >> +# emit the "does MAINTAINERS need updating?" message on file 
> >> add/move/delete
> >> +  if ($line =~ /^\s*MAINTAINERS\s*\|/) {
> >> +  $reported_maintainer_file = 1;
> >> +  }
> >> +
> >> +# Check for added, moved or deleted files
> >> +  if (!$reported_maintainer_file &&
> >> +  ($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) {
> >> +  $is_patch = 1;
> >> +  $reported_maintainer_file = 1;
> >> +  WARN("added, moved or deleted file(s), does MAINTAINERS 
> >> need updating?\n" .
> >> +  $herecurr);
> >
> > Could you please turn this into a notification instead of a warning? For
> > rationale, please see the discussion of this patch last year:
> >
> > http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html
> 
> Quoting that one:
> 
> I think chances are high that it still pops up quite frequently with
> false positives:
> 
> 1) The above regex only triggers for patches that contain a diffstat. If
> you run the script on patches without diffstat, you always get the
> warning as soon as you add, delete or move a file, even if you update
> the MAINTAINERS file in the same patch.
> 
> "Doctor, it hurts when I create patches without a diffstat."
> 
> 2) I think it is quite common in patch series to first introduce new
> files in the first patches, and then update MAINTAINERS only once at the
> end.
> 
> That's an okay thing to do now.  But is it a valid reason to block
> tooling improvements that will help us stop the constant trickle of new
> files without a maintainer?  Having to update MAINTAINERS along the way
> isn't *that* much of a burden; we'll get used to it.
> 
> 3) The MAINTAINERS file often covers whole folders with wildcard
> expressions. So if you add/delete/rename a file within such a folder,
> you don't need to update MAINTAINERS thanks to the wildcard.
> 
> True.  Perhaps the kernel would appreciate a suitable checkpatch.pl
> improvement.
> 
> I guess people might be annoyed if checkpatch.pl throws a warning in
> these cases. So a "NOTE: ..." sounds more sane to me. But if you like,
> we can also start with a WARNING first 

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Markus Armbruster]

Peter Maydell  writes:

> On 16 April 2018 at 15:11, Markus Armbruster  wrote:
>> Peter Maydell  writes:
>>
>>> On 12 March 2018 at 13:18, Stefan Hajnoczi  wrote:
 Warn if files are added/renamed/deleted without MAINTAINERS file
 changes.  This has helped me in Linux and we could benefit from this
 check in QEMU.

 This patch is a manual cherry-pick of Linux commit
 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
 file add/move/delete") by Joe Perches .

 Signed-off-by: Stefan Hajnoczi 
 ---
>>>
>>> Unfortunately the patch doesn't magically cause maintainers
>>> to appear for the new files :-)
>>
>> Fortunately, the patch can get new files non-magically rejected unless a
>> maintainers appears :)
>
> I think that "author of code lists themselves in MAINTAINERS
> but then doesn't in practice do anything" is not really much
> better (and arguably worse) than "code has no listed maintainer".

Having our tooling flag new and moved files for a possible MAINTAINERS
update need not mean adding J. Random Codeslinger to MAINTAINERS.  It
should make us stop and think.

If the kernel guys can do that, why can't we?

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Peter Maydell]

On 16 April 2018 at 15:11, Markus Armbruster  wrote:
> Peter Maydell  writes:
>
>> On 12 March 2018 at 13:18, Stefan Hajnoczi  wrote:
>>> Warn if files are added/renamed/deleted without MAINTAINERS file
>>> changes.  This has helped me in Linux and we could benefit from this
>>> check in QEMU.
>>>
>>> This patch is a manual cherry-pick of Linux commit
>>> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
>>> file add/move/delete") by Joe Perches .
>>>
>>> Signed-off-by: Stefan Hajnoczi 
>>> ---
>>
>> Unfortunately the patch doesn't magically cause maintainers
>> to appear for the new files :-)
>
> Fortunately, the patch can get new files non-magically rejected unless a
> maintainers appears :)

I think that "author of code lists themselves in MAINTAINERS
but then doesn't in practice do anything" is not really much
better (and arguably worse) than "code has no listed maintainer".

thanks
-- PMM

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Markus Armbruster]

Peter Maydell  writes:

> On 12 March 2018 at 13:18, Stefan Hajnoczi  wrote:
>> Warn if files are added/renamed/deleted without MAINTAINERS file
>> changes.  This has helped me in Linux and we could benefit from this
>> check in QEMU.
>>
>> This patch is a manual cherry-pick of Linux commit
>> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
>> file add/move/delete") by Joe Perches .
>>
>> Signed-off-by: Stefan Hajnoczi 
>> ---
>
> Unfortunately the patch doesn't magically cause maintainers
> to appear for the new files :-)

Fortunately, the patch can get new files non-magically rejected unless a
maintainers appears :)

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Markus Armbruster]

Thomas Huth  writes:

> On 12.03.2018 14:18, Stefan Hajnoczi wrote:
>> Warn if files are added/renamed/deleted without MAINTAINERS file
>> changes.  This has helped me in Linux and we could benefit from this
>> check in QEMU.
>> 
>> This patch is a manual cherry-pick of Linux commit
>> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
>> file add/move/delete") by Joe Perches .
>>
>> Signed-off-by: Stefan Hajnoczi 

We should really keep upstream's S-o-b intact.  I'd keep the whole
commit message intact:

From 7fb819c27bf47041a13feed93f86a15bdb2c681f Mon Sep 17 00:00:00 2001
From: Joe Perches 
Date: Wed, 6 Aug 2014 16:10:59 -0700
Subject: [PATCH] checkpatch: emit a warning on file add/move/delete

Whenever files are added, moved, or deleted, the MAINTAINERS file
patterns can be out of sync or outdated.

To try to keep MAINTAINERS more up-to-date, add a one-time warning
whenever a patch does any of those.

Signed-off-by: Joe Perches 
Acked-by: Andy Whitcroft 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds 
[Cherry picked from Linux commit 13f1937ef33950b1112049972249e6191b82e6c9]
Signed-off-by: Stefan Hajnoczi 

Created by running "git-format-patch -1 13f1937ef33" in the kernel,
feeding that to git-am (patch doesn't apply), patch -p1 your patch,
git-am --continue, git-commit --amend to add a backporting note and your
S-o-b.

>> ---
>> Note the 80-char lines are from upstream code.  Keep them as-is.
>> 
>>  scripts/checkpatch.pl | 19 +++
>>  1 file changed, 19 insertions(+)
>> 
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index d1fe79bcc4..d0d8f63d48 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1177,6 +1177,7 @@ sub process {
>>  our $clean = 1;
>>  my $signoff = 0;
>>  my $is_patch = 0;
>> +my $reported_maintainer_file = 0;
>>  
>>  our @report = ();
>>  our $cnt_lines = 0;
>> @@ -1379,6 +1380,24 @@ sub process {
>>  }
>>  }
>>  
>> +# Check if MAINTAINERS is being updated.  If so, there's probably no need to
>> +# emit the "does MAINTAINERS need updating?" message on file add/move/delete
>> +if ($line =~ /^\s*MAINTAINERS\s*\|/) {
>> +$reported_maintainer_file = 1;
>> +}
>> +
>> +# Check for added, moved or deleted files
>> +if (!$reported_maintainer_file &&
>> +($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) {
>> +$is_patch = 1;
>> +$reported_maintainer_file = 1;
>> +WARN("added, moved or deleted file(s), does MAINTAINERS 
>> need updating?\n" .
>> +$herecurr);
>
> Could you please turn this into a notification instead of a warning? For
> rationale, please see the discussion of this patch last year:
>
> http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html

Quoting that one:

I think chances are high that it still pops up quite frequently with
false positives:

1) The above regex only triggers for patches that contain a diffstat. If
you run the script on patches without diffstat, you always get the
warning as soon as you add, delete or move a file, even if you update
the MAINTAINERS file in the same patch.

"Doctor, it hurts when I create patches without a diffstat."

2) I think it is quite common in patch series to first introduce new
files in the first patches, and then update MAINTAINERS only once at the
end.

That's an okay thing to do now.  But is it a valid reason to block
tooling improvements that will help us stop the constant trickle of new
files without a maintainer?  Having to update MAINTAINERS along the way
isn't *that* much of a burden; we'll get used to it.

3) The MAINTAINERS file often covers whole folders with wildcard
expressions. So if you add/delete/rename a file within such a folder,
you don't need to update MAINTAINERS thanks to the wildcard.

True.  Perhaps the kernel would appreciate a suitable checkpatch.pl
improvement.

I guess people might be annoyed if checkpatch.pl throws a warning in
these cases. So a "NOTE: ..." sounds more sane to me. But if you like,
we can also start with a WARNING first and only ease it if people start
to complain?

I'd stick to the upstream version.  But if it takes deviations to get
this improvement accepted, I can live with them, as long as patchew
still flags offenders.

Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Stefan Hajnoczi]

On Tue, Mar 13, 2018 at 11:49:57AM +0100, Thomas Huth wrote:
> On 13.03.2018 11:37, Stefan Hajnoczi wrote:
> > On Mon, Mar 12, 2018 at 02:46:20PM +0100, Thomas Huth wrote:
> >> On 12.03.2018 14:18, Stefan Hajnoczi wrote:
> >>> Warn if files are added/renamed/deleted without MAINTAINERS file
> >>> changes.  This has helped me in Linux and we could benefit from this
> >>> check in QEMU.
> >>>
> >>> This patch is a manual cherry-pick of Linux commit
> >>> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
> >>> file add/move/delete") by Joe Perches .
> >>>
> >>> Signed-off-by: Stefan Hajnoczi 
> >>> ---
> >>> Note the 80-char lines are from upstream code.  Keep them as-is.
> >>>
> >>>  scripts/checkpatch.pl | 19 +++
> >>>  1 file changed, 19 insertions(+)
> >>>
> >>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>> index d1fe79bcc4..d0d8f63d48 100755
> >>> --- a/scripts/checkpatch.pl
> >>> +++ b/scripts/checkpatch.pl
> >>> @@ -1177,6 +1177,7 @@ sub process {
> >>>   our $clean = 1;
> >>>   my $signoff = 0;
> >>>   my $is_patch = 0;
> >>> + my $reported_maintainer_file = 0;
> >>>  
> >>>   our @report = ();
> >>>   our $cnt_lines = 0;
> >>> @@ -1379,6 +1380,24 @@ sub process {
> >>>   }
> >>>   }
> >>>  
> >>> +# Check if MAINTAINERS is being updated.  If so, there's probably no 
> >>> need to
> >>> +# emit the "does MAINTAINERS need updating?" message on file 
> >>> add/move/delete
> >>> + if ($line =~ /^\s*MAINTAINERS\s*\|/) {
> >>> + $reported_maintainer_file = 1;
> >>> + }
> >>> +
> >>> +# Check for added, moved or deleted files
> >>> + if (!$reported_maintainer_file &&
> >>> + ($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) {
> >>> + $is_patch = 1;
> >>> + $reported_maintainer_file = 1;
> >>> + WARN("added, moved or deleted file(s), does MAINTAINERS 
> >>> need updating?\n" .
> >>> + $herecurr);
> >>
> >> Could you please turn this into a notification instead of a warning? For
> >> rationale, please see the discussion of this patch last year:
> >>
> >> http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html
> > 
> > It's a warning, not an error, so this already means "don't treat it as
> > fatal".
> > 
> > Why is a warning a bad user experience but a notification would be fine?
> 
> Since this will likely cause a lot of false positives. I think people
> will then rather be annoyed if checkpatch.pl nags them with a warning in
> these cases.

This approach works fine for Linux, I don't think it will be a problem
for QEMU.

The idea of zero false positives is nice though.  Do you have time to
implement a real MAINTAINERS checker that avoids false positives (i.e.
it applies the MAINTAINERS hunk in the patch, if present, onto the
current MAINTAINERS file and then looks up every new file or rename)?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180312131806.23209-1-stefa...@redhat.com
Subject: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file 
changes

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
89d665069d checkpatch: warn about missing MAINTAINERS file changes

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: warn about missing MAINTAINERS file changes...
WARNING: line over 80 characters
#47: FILE: scripts/checkpatch.pl:1393:
+($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ 
&&

ERROR: line over 90 characters
#51: FILE: scripts/checkpatch.pl:1397:
+   WARN("added, moved or deleted file(s), does MAINTAINERS 
need updating?\n" .

total: 1 errors, 1 warnings, 31 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Thomas Huth]

On 13.03.2018 11:37, Stefan Hajnoczi wrote:
> On Mon, Mar 12, 2018 at 02:46:20PM +0100, Thomas Huth wrote:
>> On 12.03.2018 14:18, Stefan Hajnoczi wrote:
>>> Warn if files are added/renamed/deleted without MAINTAINERS file
>>> changes.  This has helped me in Linux and we could benefit from this
>>> check in QEMU.
>>>
>>> This patch is a manual cherry-pick of Linux commit
>>> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
>>> file add/move/delete") by Joe Perches .
>>>
>>> Signed-off-by: Stefan Hajnoczi 
>>> ---
>>> Note the 80-char lines are from upstream code.  Keep them as-is.
>>>
>>>  scripts/checkpatch.pl | 19 +++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index d1fe79bcc4..d0d8f63d48 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -1177,6 +1177,7 @@ sub process {
>>> our $clean = 1;
>>> my $signoff = 0;
>>> my $is_patch = 0;
>>> +   my $reported_maintainer_file = 0;
>>>  
>>> our @report = ();
>>> our $cnt_lines = 0;
>>> @@ -1379,6 +1380,24 @@ sub process {
>>> }
>>> }
>>>  
>>> +# Check if MAINTAINERS is being updated.  If so, there's probably no need 
>>> to
>>> +# emit the "does MAINTAINERS need updating?" message on file 
>>> add/move/delete
>>> +   if ($line =~ /^\s*MAINTAINERS\s*\|/) {
>>> +   $reported_maintainer_file = 1;
>>> +   }
>>> +
>>> +# Check for added, moved or deleted files
>>> +   if (!$reported_maintainer_file &&
>>> +   ($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) {
>>> +   $is_patch = 1;
>>> +   $reported_maintainer_file = 1;
>>> +   WARN("added, moved or deleted file(s), does MAINTAINERS 
>>> need updating?\n" .
>>> +   $herecurr);
>>
>> Could you please turn this into a notification instead of a warning? For
>> rationale, please see the discussion of this patch last year:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html
> 
> It's a warning, not an error, so this already means "don't treat it as
> fatal".
> 
> Why is a warning a bad user experience but a notification would be fine?

Since this will likely cause a lot of false positives. I think people
will then rather be annoyed if checkpatch.pl nags them with a warning in
these cases.

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Stefan Hajnoczi]

On Mon, Mar 12, 2018 at 02:46:20PM +0100, Thomas Huth wrote:
> On 12.03.2018 14:18, Stefan Hajnoczi wrote:
> > Warn if files are added/renamed/deleted without MAINTAINERS file
> > changes.  This has helped me in Linux and we could benefit from this
> > check in QEMU.
> > 
> > This patch is a manual cherry-pick of Linux commit
> > 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
> > file add/move/delete") by Joe Perches .
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> > Note the 80-char lines are from upstream code.  Keep them as-is.
> > 
> >  scripts/checkpatch.pl | 19 +++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d1fe79bcc4..d0d8f63d48 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1177,6 +1177,7 @@ sub process {
> > our $clean = 1;
> > my $signoff = 0;
> > my $is_patch = 0;
> > +   my $reported_maintainer_file = 0;
> >  
> > our @report = ();
> > our $cnt_lines = 0;
> > @@ -1379,6 +1380,24 @@ sub process {
> > }
> > }
> >  
> > +# Check if MAINTAINERS is being updated.  If so, there's probably no need 
> > to
> > +# emit the "does MAINTAINERS need updating?" message on file 
> > add/move/delete
> > +   if ($line =~ /^\s*MAINTAINERS\s*\|/) {
> > +   $reported_maintainer_file = 1;
> > +   }
> > +
> > +# Check for added, moved or deleted files
> > +   if (!$reported_maintainer_file &&
> > +   ($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) {
> > +   $is_patch = 1;
> > +   $reported_maintainer_file = 1;
> > +   WARN("added, moved or deleted file(s), does MAINTAINERS 
> > need updating?\n" .
> > +   $herecurr);
> 
> Could you please turn this into a notification instead of a warning? For
> rationale, please see the discussion of this patch last year:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html

It's a warning, not an error, so this already means "don't treat it as
fatal".

Why is a warning a bad user experience but a notification would be fine?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Thomas Huth]

On 12.03.2018 14:18, Stefan Hajnoczi wrote:
> Warn if files are added/renamed/deleted without MAINTAINERS file
> changes.  This has helped me in Linux and we could benefit from this
> check in QEMU.
> 
> This patch is a manual cherry-pick of Linux commit
> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
> file add/move/delete") by Joe Perches .
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
> Note the 80-char lines are from upstream code.  Keep them as-is.
> 
>  scripts/checkpatch.pl | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d1fe79bcc4..d0d8f63d48 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1177,6 +1177,7 @@ sub process {
>   our $clean = 1;
>   my $signoff = 0;
>   my $is_patch = 0;
> + my $reported_maintainer_file = 0;
>  
>   our @report = ();
>   our $cnt_lines = 0;
> @@ -1379,6 +1380,24 @@ sub process {
>   }
>   }
>  
> +# Check if MAINTAINERS is being updated.  If so, there's probably no need to
> +# emit the "does MAINTAINERS need updating?" message on file add/move/delete
> + if ($line =~ /^\s*MAINTAINERS\s*\|/) {
> + $reported_maintainer_file = 1;
> + }
> +
> +# Check for added, moved or deleted files
> + if (!$reported_maintainer_file &&
> + ($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) {
> + $is_patch = 1;
> + $reported_maintainer_file = 1;
> + WARN("added, moved or deleted file(s), does MAINTAINERS 
> need updating?\n" .
> + $herecurr);

Could you please turn this into a notification instead of a warning? For
rationale, please see the discussion of this patch last year:

http://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Marc-André Lureau]

On Mon, Mar 12, 2018 at 2:18 PM, Stefan Hajnoczi  wrote:
> Warn if files are added/renamed/deleted without MAINTAINERS file
> changes.  This has helped me in Linux and we could benefit from this
> check in QEMU.
>
> This patch is a manual cherry-pick of Linux commit
> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
> file add/move/delete") by Joe Perches .
>
> Signed-off-by: Stefan Hajnoczi 

nice,
Reviewed-by: Marc-André Lureau 


> ---
> Note the 80-char lines are from upstream code.  Keep them as-is.
>
>  scripts/checkpatch.pl | 19 +++
>  1 file changed, 19 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d1fe79bcc4..d0d8f63d48 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1177,6 +1177,7 @@ sub process {
> our $clean = 1;
> my $signoff = 0;
> my $is_patch = 0;
> +   my $reported_maintainer_file = 0;
>
> our @report = ();
> our $cnt_lines = 0;
> @@ -1379,6 +1380,24 @@ sub process {
> }
> }
>
> +# Check if MAINTAINERS is being updated.  If so, there's probably no need to
> +# emit the "does MAINTAINERS need updating?" message on file add/move/delete
> +   if ($line =~ /^\s*MAINTAINERS\s*\|/) {
> +   $reported_maintainer_file = 1;
> +   }
> +
> +# Check for added, moved or deleted files
> +   if (!$reported_maintainer_file &&
> +   ($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) {
> +   $is_patch = 1;
> +   $reported_maintainer_file = 1;
> +   WARN("added, moved or deleted file(s), does 
> MAINTAINERS need updating?\n" .
> +   $herecurr);
> +   }
> +
>  # Check for wrappage within a valid hunk of the file
> if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) 
> {
> ERROR("patch seems to be corrupt (line wrapped?)\n" .
> --
> 2.14.3
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Peter Maydell]

On 12 March 2018 at 13:18, Stefan Hajnoczi  wrote:
> Warn if files are added/renamed/deleted without MAINTAINERS file
> changes.  This has helped me in Linux and we could benefit from this
> check in QEMU.
>
> This patch is a manual cherry-pick of Linux commit
> 13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
> file add/move/delete") by Joe Perches .
>
> Signed-off-by: Stefan Hajnoczi 
> ---

Unfortunately the patch doesn't magically cause maintainers
to appear for the new files :-)

thanks
-- PMM

[Qemu-devel] [PATCH] checkpatch: warn about missing MAINTAINERS file changes

[Stefan Hajnoczi]

Warn if files are added/renamed/deleted without MAINTAINERS file
changes.  This has helped me in Linux and we could benefit from this
check in QEMU.

This patch is a manual cherry-pick of Linux commit
13f1937ef33950b1112049972249e6191b82e6c9 ("checkpatch: emit a warning on
file add/move/delete") by Joe Perches .

Signed-off-by: Stefan Hajnoczi 
---
Note the 80-char lines are from upstream code.  Keep them as-is.

 scripts/checkpatch.pl | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d1fe79bcc4..d0d8f63d48 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1177,6 +1177,7 @@ sub process {
our $clean = 1;
my $signoff = 0;
my $is_patch = 0;
+   my $reported_maintainer_file = 0;
 
our @report = ();
our $cnt_lines = 0;
@@ -1379,6 +1380,24 @@ sub process {
}
}
 
+# Check if MAINTAINERS is being updated.  If so, there's probably no need to
+# emit the "does MAINTAINERS need updating?" message on file add/move/delete
+   if ($line =~ /^\s*MAINTAINERS\s*\|/) {
+   $reported_maintainer_file = 1;
+   }
+
+# Check for added, moved or deleted files
+   if (!$reported_maintainer_file &&
+   ($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) {
+   $is_patch = 1;
+   $reported_maintainer_file = 1;
+   WARN("added, moved or deleted file(s), does MAINTAINERS 
need updating?\n" .
+   $herecurr);
+   }
+
 # Check for wrappage within a valid hunk of the file
if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
ERROR("patch seems to be corrupt (line wrapped?)\n" .
-- 
2.14.3

Re: [Qemu-devel] [PATCH] checkpatch: Exempt long URLs

[Paolo Bonzini]

On 22/02/2018 22:58, Eric Blake wrote:
> Sometimes, we want to refer to really long URLs, but checkpatch
> balks, and we have to manually bypass the check.  URL shorterners
> may be nice at reducing long links, but it's hard to guarantee the
> shortened link will live as long as the real target, and it is
> also nice to see the original target without having to load the
> shortened URL through a browser.  So exempt a line containing
> only a URL from the long-line syntax check.
> 
> Suggested-by: Peter Maydell 
> Signed-off-by: Eric Blake 
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 1b4b812e28f..0d3f753c665 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1447,9 +1447,10 @@ sub process {
>  # check we are in a valid source file if not then ignore this hunk
>   next if ($realfile !~ /$SrcFile/);
> 
> -#90 column limit
> +#90 column limit; exempt URLs, if no other words on line
>   if ($line =~ /^\+/ &&
>   !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
> + !($rawline =~ /^[^[:alnum:]]*https?:\S*$/) &&
>   $length > 80)
>   {
>   if ($length > 90) {
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH] checkpatch: Exempt long URLs

[Stefan Hajnoczi]

On Thu, Feb 22, 2018 at 03:58:38PM -0600, Eric Blake wrote:
> Sometimes, we want to refer to really long URLs, but checkpatch
> balks, and we have to manually bypass the check.  URL shorterners
> may be nice at reducing long links, but it's hard to guarantee the
> shortened link will live as long as the real target, and it is
> also nice to see the original target without having to load the
> shortened URL through a browser.  So exempt a line containing
> only a URL from the long-line syntax check.
> 
> Suggested-by: Peter Maydell 
> Signed-off-by: Eric Blake 
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] checkpatch: add a warning for basename/dirname

[Julia Suvorova via Qemu-devel]

On 02.03.2018 11:56, Paolo Bonzini wrote:
> On 02/03/2018 09:22, Julia Suvorova wrote:
>> Signed-off-by: Julia Suvorova 
>> ---
>>  scripts/checkpatch.pl | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 1b4b812..6c4fb42 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2584,6 +2584,11 @@ sub process {
>>  ERROR("__func__ should be used instead of gcc specific 
>> __FUNCTION__\n"  . $herecurr);
>>  }
>>  
>> +# recommend g_path_get_* over basename(3) and dirname(3)
>> +if ($line =~ /\b(basename|dirname)\s*\(/) {
>> +WARN("consider using g_path_get_$1 in preference to 
>> $1(3)\n" . $herecurr);
>> +}
>> +
>>  # recommend qemu_strto* over strto* for numeric conversions
>>  if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
>>  ERROR("consider using qemu_$1 in preference to $1\n" . 
>> $herecurr);
>>
> 
> Hi Julia, the patch is fine, but given Alex's objections let's warn only
> if you are doing g_strdup(basename(...)) or g_strdup(dirname(...)).
> 

Ok.

> (No other action is needed on your other patch).
> 
> Thanks!
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH] checkpatch: add a warning for basename/dirname

[Paolo Bonzini]

On 02/03/2018 09:22, Julia Suvorova wrote:
> Signed-off-by: Julia Suvorova 
> ---
>  scripts/checkpatch.pl | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 1b4b812..6c4fb42 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2584,6 +2584,11 @@ sub process {
>   ERROR("__func__ should be used instead of gcc specific 
> __FUNCTION__\n"  . $herecurr);
>   }
>  
> +# recommend g_path_get_* over basename(3) and dirname(3)
> + if ($line =~ /\b(basename|dirname)\s*\(/) {
> + WARN("consider using g_path_get_$1 in preference to 
> $1(3)\n" . $herecurr);
> + }
> +
>  # recommend qemu_strto* over strto* for numeric conversions
>   if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
>   ERROR("consider using qemu_$1 in preference to $1\n" . 
> $herecurr);
> 

Hi Julia, the patch is fine, but given Alex's objections let's warn only
if you are doing g_strdup(basename(...)) or g_strdup(dirname(...)).

(No other action is needed on your other patch).

Thanks!

Paolo

Re: [Qemu-devel] [PATCH] checkpatch: add a warning for basename/dirname

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1519978965-16865-1-git-send-email-jus...@mail.ru
Subject: [Qemu-devel] [PATCH] checkpatch: add a warning for basename/dirname

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   
patchew/1519978965-16865-1-git-send-email-jus...@mail.ru -> 
patchew/1519978965-16865-1-git-send-email-jus...@mail.ru
Switched to a new branch 'test'
d5b85a9adb checkpatch: add a warning for basename/dirname

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: add a warning for basename/dirname...
ERROR: line over 90 characters
#19: FILE: scripts/checkpatch.pl:2589:
+   WARN("consider using g_path_get_$1 in preference to 
$1(3)\n" . $herecurr);

total: 1 errors, 0 warnings, 11 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] checkpatch: add a warning for basename/dirname

[Julia Suvorova via Qemu-devel]

Signed-off-by: Julia Suvorova 
---
 scripts/checkpatch.pl | 5 +
 1 file changed, 5 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1b4b812..6c4fb42 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2584,6 +2584,11 @@ sub process {
ERROR("__func__ should be used instead of gcc specific 
__FUNCTION__\n"  . $herecurr);
}
 
+# recommend g_path_get_* over basename(3) and dirname(3)
+   if ($line =~ /\b(basename|dirname)\s*\(/) {
+   WARN("consider using g_path_get_$1 in preference to 
$1(3)\n" . $herecurr);
+   }
+
 # recommend qemu_strto* over strto* for numeric conversions
if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
ERROR("consider using qemu_$1 in preference to $1\n" . 
$herecurr);
-- 
2.1.4

Re: [Qemu-devel] [PATCH] checkpatch: Exempt long URLs

[Eric Blake]

On 02/22/2018 03:58 PM, Eric Blake wrote:

Sometimes, we want to refer to really long URLs, but checkpatch
balks, and we have to manually bypass the check.  URL shorterners


s/shorterners/shorteners/

(Is it bad when I spell-check my own patches?)


may be nice at reducing long links, but it's hard to guarantee the
shortened link will live as long as the real target, and it is
also nice to see the original target without having to load the
shortened URL through a browser.  So exempt a line containing
only a URL from the long-line syntax check.

Suggested-by: Peter Maydell 
Signed-off-by: Eric Blake 
---


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH] checkpatch: Exempt long URLs

[Eric Blake]

Sometimes, we want to refer to really long URLs, but checkpatch
balks, and we have to manually bypass the check.  URL shorterners
may be nice at reducing long links, but it's hard to guarantee the
shortened link will live as long as the real target, and it is
also nice to see the original target without having to load the
shortened URL through a browser.  So exempt a line containing
only a URL from the long-line syntax check.

Suggested-by: Peter Maydell 
Signed-off-by: Eric Blake 
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1b4b812e28f..0d3f753c665 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1447,9 +1447,10 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
next if ($realfile !~ /$SrcFile/);

-#90 column limit
+#90 column limit; exempt URLs, if no other words on line
if ($line =~ /^\+/ &&
!($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
+   !($rawline =~ /^[^[:alnum:]]*https?:\S*$/) &&
$length > 80)
{
if ($length > 90) {
-- 
2.14.3

Re: [Qemu-devel] [PATCH] checkpatch: warn when using volatile with a comment

[Darren Kenny]

Nevermind, saw that updated comment in the later patch...

Thanks,

Darren.

On Mon, Dec 18, 2017 at 01:36:52PM +, Darren Kenny wrote:

Hi Paolo,

Slight nit on the subject line, did you mean to s/with/without/ -
that seems to reflect the change in the patch more correctly.

Thanks,

Darren.

On Mon, Dec 18, 2017 at 01:49:52PM +0100, Paolo Bonzini wrote:

On 15/12/2017 19:18, Marc-André Lureau wrote:

Instead of an error, lower to a warning message, assuming the comment
gives some justification.

Discussed in:
'[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a process 
to debug"'

Suggested-by: Fam Zheng 
Signed-off-by: Marc-André Lureau 


We can drop the error at all if there is a comment.  Also, "volatile 
sig_atomic_t" is probably self-explanatory and usually correct.  So what about this:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f5a523af10..3dc27d9656 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2475,13 +2475,11 @@ sub process {

# no volatiles please
my $asm_volatile = 
qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
-   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
-   my $msg = "Use of volatile is usually wrong: see 
Documentation/volatile-considered-harmful.txt\n" . $herecurr;
-   if (ctx_has_comment($first_line, $linenr)) {
-   WARN($msg);
-   } else {
-   ERROR($msg);
-   }
+   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ &&
+$line !~ /sig_atomic_t/ &&
+!ctx_has_comment($first_line, $linenr)) {
+   my $msg = "Use of volatile is usually wrong, please add a 
comment\n" . $herecurr;
+ERROR($msg);
}

# warn about #if 0

Re: [Qemu-devel] [PATCH] checkpatch: warn when using volatile with a comment

[Darren Kenny]

Hi Paolo,

Slight nit on the subject line, did you mean to s/with/without/ -
that seems to reflect the change in the patch more correctly.

Thanks,

Darren.

On Mon, Dec 18, 2017 at 01:49:52PM +0100, Paolo Bonzini wrote:

On 15/12/2017 19:18, Marc-André Lureau wrote:

Instead of an error, lower to a warning message, assuming the comment
gives some justification.

Discussed in:
'[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a process 
to debug"'

Suggested-by: Fam Zheng 
Signed-off-by: Marc-André Lureau 


We can drop the error at all if there is a comment.  Also, "volatile 
sig_atomic_t" is probably self-explanatory and usually correct.  So what about this:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f5a523af10..3dc27d9656 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2475,13 +2475,11 @@ sub process {

# no volatiles please
my $asm_volatile = 
qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
-   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
-   my $msg = "Use of volatile is usually wrong: see 
Documentation/volatile-considered-harmful.txt\n" . $herecurr;
-   if (ctx_has_comment($first_line, $linenr)) {
-   WARN($msg);
-   } else {
-   ERROR($msg);
-   }
+   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ &&
+$line !~ /sig_atomic_t/ &&
+!ctx_has_comment($first_line, $linenr)) {
+   my $msg = "Use of volatile is usually wrong, please add a 
comment\n" . $herecurr;
+ERROR($msg);
}

# warn about #if 0

Re: [Qemu-devel] [PATCH] checkpatch: warn when using volatile with a comment

[Paolo Bonzini]

On 18/12/2017 13:54, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Dec 18, 2017 at 1:49 PM, Paolo Bonzini  wrote:
>> On 15/12/2017 19:18, Marc-André Lureau wrote:
>>> Instead of an error, lower to a warning message, assuming the comment
>>> gives some justification.
>>>
>>> Discussed in:
>>> '[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without 
>>> a process to debug"'
>>>
>>> Suggested-by: Fam Zheng 
>>> Signed-off-by: Marc-André Lureau 
>>
>> We can drop the error at all if there is a comment.  Also, "volatile 
>> sig_atomic_t" is probably self-explanatory and usually correct.  So what 
>> about this:
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index f5a523af10..3dc27d9656 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2475,13 +2475,11 @@ sub process {
>>
>>  # no volatiles please
>> my $asm_volatile = 
>> qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
>> -   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
>> -   my $msg = "Use of volatile is usually wrong: see 
>> Documentation/volatile-considered-harmful.txt\n" . $herecurr;
>> -   if (ctx_has_comment($first_line, $linenr)) {
>> -   WARN($msg);
>> -   } else {
>> -   ERROR($msg);
>> -   }
>> +   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ &&
>> +$line !~ /sig_atomic_t/ &&
>> +!ctx_has_comment($first_line, $linenr)) {
>> +   my $msg = "Use of volatile is usually wrong, please 
>> add a comment\n" . $herecurr;
>> +ERROR($msg);
>> }
>>
> 
> Fine for me, can you send a proper patch?

Yes, will do.

Paolo

Re: [Qemu-devel] [PATCH] checkpatch: warn when using volatile with a comment

[Marc-André Lureau]

Hi

On Mon, Dec 18, 2017 at 1:49 PM, Paolo Bonzini  wrote:
> On 15/12/2017 19:18, Marc-André Lureau wrote:
>> Instead of an error, lower to a warning message, assuming the comment
>> gives some justification.
>>
>> Discussed in:
>> '[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a 
>> process to debug"'
>>
>> Suggested-by: Fam Zheng 
>> Signed-off-by: Marc-André Lureau 
>
> We can drop the error at all if there is a comment.  Also, "volatile 
> sig_atomic_t" is probably self-explanatory and usually correct.  So what 
> about this:
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f5a523af10..3dc27d9656 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2475,13 +2475,11 @@ sub process {
>
>  # no volatiles please
> my $asm_volatile = 
> qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
> -   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
> -   my $msg = "Use of volatile is usually wrong: see 
> Documentation/volatile-considered-harmful.txt\n" . $herecurr;
> -   if (ctx_has_comment($first_line, $linenr)) {
> -   WARN($msg);
> -   } else {
> -   ERROR($msg);
> -   }
> +   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ &&
> +$line !~ /sig_atomic_t/ &&
> +!ctx_has_comment($first_line, $linenr)) {
> +   my $msg = "Use of volatile is usually wrong, please 
> add a comment\n" . $herecurr;
> +ERROR($msg);
> }
>

Fine for me, can you send a proper patch?

Thanks



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] checkpatch: warn when using volatile with a comment

[Paolo Bonzini]

On 15/12/2017 19:18, Marc-André Lureau wrote:
> Instead of an error, lower to a warning message, assuming the comment
> gives some justification.
> 
> Discussed in:
> '[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a 
> process to debug"'
> 
> Suggested-by: Fam Zheng 
> Signed-off-by: Marc-André Lureau 

We can drop the error at all if there is a comment.  Also, "volatile 
sig_atomic_t" is probably self-explanatory and usually correct.  So what about 
this:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f5a523af10..3dc27d9656 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2475,13 +2475,11 @@ sub process {
 
 # no volatiles please
my $asm_volatile = 
qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
-   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
-   my $msg = "Use of volatile is usually wrong: see 
Documentation/volatile-considered-harmful.txt\n" . $herecurr;
-   if (ctx_has_comment($first_line, $linenr)) {
-   WARN($msg);
-   } else {
-   ERROR($msg);
-   }
+   if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ &&
+$line !~ /sig_atomic_t/ &&
+!ctx_has_comment($first_line, $linenr)) {
+   my $msg = "Use of volatile is usually wrong, please add 
a comment\n" . $herecurr;
+ERROR($msg);
}
 
 # warn about #if 0

Re: [Qemu-devel] [PATCH] checkpatch: warn when using volatile with a comment

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20171215181810.4122-1-marcandre.lur...@redhat.com
Subject: [Qemu-devel] [PATCH] checkpatch: warn when using volatile with a 
comment

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]
patchew/20171215150659.1811-1-marcandre.lur...@redhat.com -> 
patchew/20171215150659.1811-1-marcandre.lur...@redhat.com
 t [tag update]
patchew/20171215181810.4122-1-marcandre.lur...@redhat.com -> 
patchew/20171215181810.4122-1-marcandre.lur...@redhat.com
Switched to a new branch 'test'
80828ed1b5 checkpatch: warn when using volatile with a comment

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: warn when using volatile with a comment...
ERROR: line over 90 characters
#28: FILE: scripts/checkpatch.pl:2479:
+   my $msg = "Use of volatile is usually wrong: see 
Documentation/volatile-considered-harmful.txt\n" . $herecurr;

total: 1 errors, 0 warnings, 13 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] checkpatch: warn when using volatile with a comment

[Marc-André Lureau]

Instead of an error, lower to a warning message, assuming the comment
gives some justification.

Discussed in:
'[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a 
process to debug"'

Suggested-by: Fam Zheng 
Signed-off-by: Marc-André Lureau 
---
 scripts/checkpatch.pl | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34df753571..f5a523af10 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2476,7 +2476,12 @@ sub process {
 # no volatiles please
my $asm_volatile = 
qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
-   ERROR("Use of volatile is usually wrong: see 
Documentation/volatile-considered-harmful.txt\n" . $herecurr);
+   my $msg = "Use of volatile is usually wrong: see 
Documentation/volatile-considered-harmful.txt\n" . $herecurr;
+   if (ctx_has_comment($first_line, $linenr)) {
+   WARN($msg);
+   } else {
+   ERROR($msg);
+   }
}
 
 # warn about #if 0
-- 
2.15.1.355.g36791d7216

Re: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN for extern checking.

[Markus Armbruster]

Peter Maydell  writes:

> On 11 October 2017 at 05:33, Jiang Biao  wrote:
>> There are some rare cases which need external declarations in .c
>> files. patchew.org and checkpatch.pl will complain errors on
>> patches for these declarations.
>>
>> Degrade ERROR to WARN to erase the error complaints taking
>> checkpatch.pl in kernel as reference.
>>
>> Signed-off-by: Jiang Biao 
>> ---
>>  scripts/checkpatch.pl | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> I'd rather not drop this to a warning for the sake of a single
> use case that's already in the tree (and which if you really

Concur.  Rare false positives from checkpatch are tolerable.

> cared about you could work around by putting the link_error()
> declaration in a header file I suppose, though I wouldn't
> bother personally.)

Re: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN forextern checking.

[jiang.biao2]

> On 11 October 2017 at 05:33, Jiang Biao  wrote:
> 
> I'd rather not drop this to a warning for the sake of a single
> use case that's already in the tree (and which if you really
> cared about you could work around by putting the link_error()
> declaration in a header file I suppose, though I wouldn't
> bother personally.)



Neither would I :). Thanks for the reply. 


Regards, Jiang

Re: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN for extern checking.

[Peter Maydell]

On 11 October 2017 at 05:33, Jiang Biao  wrote:
> There are some rare cases which need external declarations in .c
> files. patchew.org and checkpatch.pl will complain errors on
> patches for these declarations.
>
> Degrade ERROR to WARN to erase the error complaints taking
> checkpatch.pl in kernel as reference.
>
> Signed-off-by: Jiang Biao 
> ---
>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

I'd rather not drop this to a warning for the sake of a single
use case that's already in the tree (and which if you really
cared about you could work around by putting the link_error()
declaration in a header file I suppose, though I wouldn't
bother personally.)

thanks
-- PMM

[Qemu-devel] [PATCH] checkpatch: ignore perl files.

[Jiang Biao]

Most coding styles are not suit for perl files. If we check
scripts/checkpatch.pl with itself, there would be tons of complaints.
And if we make patches for checkpatch.pl, patchew.org and
checkpatch.pl would complain errors for the patches.

Ignore perl files taking Linux kernel version as reference.

Signed-off-by: Jiang Biao 
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3c0a28e..c4ec031 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1442,7 +1442,7 @@ sub process {
}
 
 # check we are in a valid source file if not then ignore this hunk
-   next if ($realfile !~ /\.(h|c|cpp|s|S|pl|py|sh)$/);
+   next if ($realfile !~ /\.(h|c|cpp|s|S|py|sh)$/);
 
 #90 column limit
if ($line =~ /^\+/ &&
-- 
2.9.5

Re: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN for extern checking.

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1507696406-11168-1-git-send-email-jiang.bi...@zte.com.cn
Subject: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN for extern 
checking.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5a8e815e78 checkpatch: replace ERROR with WARN for extern checking.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: replace ERROR with WARN for extern checking
ERROR: line over 90 characters
#25: FILE: scripts/checkpatch.pl:2550:
+   WARN("externs should be avoided in .c files\n" 
.  $herecurr);

WARNING: line over 80 characters
#34: FILE: scripts/checkpatch.pl:2560:
+   WARN("externs should be avoided in .c files\n" .  
$herecurr);

total: 1 errors, 1 warnings, 16 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN for extern checking.

[Jiang Biao]

There are some rare cases which need external declarations in .c
files. patchew.org and checkpatch.pl will complain errors on
patches for these declarations.

Degrade ERROR to WARN to erase the error complaints taking
checkpatch.pl in kernel as reference.

Signed-off-by: Jiang Biao 
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3c0a28e..9123788 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2546,7 +2546,7 @@ sub process {
if ($s =~ /^\s*;/ &&
$function_name ne 'uninitialized_var')
{
-   ERROR("externs should be avoided in .c files\n" 
.  $herecurr);
+   WARN("externs should be avoided in .c files\n" 
.  $herecurr);
}
 
if ($paren_space =~ /\n/) {
@@ -2556,7 +2556,7 @@ sub process {
} elsif ($realfile =~ /\.c$/ && defined $stat &&
$stat =~ /^.\s*extern\s+/)
{
-   ERROR("externs should be avoided in .c files\n" .  
$herecurr);
+   WARN("externs should be avoided in .c files\n" .  
$herecurr);
}
 
 # check for pointless casting of g_malloc return
-- 
2.9.5

Re: [Qemu-devel] [PATCH] checkpatch: fix incompatibility with old perl

[Stefan Hajnoczi]

On Wed, Oct 04, 2017 at 06:44:20PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Do not use '/r' modifier which was introduced in perl 5.14.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Thanks, applied to my tracing tree:
https://github.com/stefanha/qemu/commits/tracing

Stefan

Re: [Qemu-devel] [PATCH] checkpatch: fix incompatibility with old perl

[Daniel P. Berrange]

On Wed, Oct 04, 2017 at 06:44:20PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Do not use '/r' modifier which was introduced in perl 5.14.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrange 


> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 3c0a28e644..0c41f1212f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1432,7 +1432,8 @@ sub process {
>   qr/%[-+ 
> *.0-9]*([hljztL]|ll|hh)?(x|X|"\s*PRI[xX][^"]*"?)/;
>  
>   # don't consider groups splitted by [.:/ ], 
> like 2A.20:12ab
> - my $tmpline = $rawline =~ s/($hex[.:\/ 
> ])+$hex//gr;
> + my $tmpline = $rawline;
> + $tmpline =~ s/($hex[.:\/ ])+$hex//g;
>  
>   if ($tmpline =~ /(?   ERROR("Hex numbers must be prefixed 
> with '0x'\n" .

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] checkpatch: fix incompatibility with old perl

[Alex Williamson]

On Wed,  4 Oct 2017 18:44:20 +0300
Vladimir Sementsov-Ogievskiy  wrote:

> Do not use '/r' modifier which was introduced in perl 5.14.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---

Fixes: 3e5875afc0f ("checkpatch: check trace-events code style")
Tested-by: Alex Williamson 

Thanks

>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 3c0a28e644..0c41f1212f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1432,7 +1432,8 @@ sub process {
>   qr/%[-+ 
> *.0-9]*([hljztL]|ll|hh)?(x|X|"\s*PRI[xX][^"]*"?)/;
>  
>   # don't consider groups splitted by [.:/ ], 
> like 2A.20:12ab
> - my $tmpline = $rawline =~ s/($hex[.:\/ 
> ])+$hex//gr;
> + my $tmpline = $rawline;
> + $tmpline =~ s/($hex[.:\/ ])+$hex//g;
>  
>   if ($tmpline =~ /(?   ERROR("Hex numbers must be prefixed 
> with '0x'\n" .

[Qemu-devel] [PATCH] checkpatch: fix incompatibility with old perl

[Vladimir Sementsov-Ogievskiy]

Do not use '/r' modifier which was introduced in perl 5.14.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3c0a28e644..0c41f1212f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1432,7 +1432,8 @@ sub process {
qr/%[-+ 
*.0-9]*([hljztL]|ll|hh)?(x|X|"\s*PRI[xX][^"]*"?)/;
 
# don't consider groups splitted by [.:/ ], 
like 2A.20:12ab
-   my $tmpline = $rawline =~ s/($hex[.:\/ 
])+$hex//gr;
+   my $tmpline = $rawline;
+   $tmpline =~ s/($hex[.:\/ ])+$hex//g;
 
if ($tmpline =~ /(?

Re: [Qemu-devel] [PATCH] checkpatch: add hwaddr to @typeList

[Paolo Bonzini]

On 14/09/2017 11:12, Greg Kurz wrote:
> The script doesn't know about all possible types and learn them as
> it parses the code. If it reaches a line with a type cast but the
> type isn't known yet, it is misinterpreted as an identifier.
> 
> For example the following line:
> 
> foo = (hwaddr) -1;
> 
> results in the following false-positive to be reported:
> 
> ERROR: spaces required around that '-' (ctx:VxV)
> 
> Let's add this standard QEMU type to the list of pre-known types.
> 
> Signed-off-by: Greg Kurz 
> ---
> 
> checkpatch doesn't have an official maintainer, Cc'ing David in case he
> agrees to carry this patch through his ppc tree, along with:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg03686.html
> 
>  scripts/checkpatch.pl |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fa478074b88d..def5bc1cc0e1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -213,6 +213,7 @@ our @typeList = (
>   qr{${Ident}_handler},
>   qr{${Ident}_handler_fn},
>   qr{target_(?:u)?long},
> + qr{hwaddr},
>  );
>  
>  # This can be modified by sub possible.  Since it can be empty, be careful
> 

Queued, thanks.

Paolo

[Qemu-devel] [PATCH] checkpatch: add hwaddr to @typeList

[Greg Kurz]

The script doesn't know about all possible types and learn them as
it parses the code. If it reaches a line with a type cast but the
type isn't known yet, it is misinterpreted as an identifier.

For example the following line:

foo = (hwaddr) -1;

results in the following false-positive to be reported:

ERROR: spaces required around that '-' (ctx:VxV)

Let's add this standard QEMU type to the list of pre-known types.

Signed-off-by: Greg Kurz 
---

checkpatch doesn't have an official maintainer, Cc'ing David in case he
agrees to carry this patch through his ppc tree, along with:

https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg03686.html

 scripts/checkpatch.pl |1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fa478074b88d..def5bc1cc0e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -213,6 +213,7 @@ our @typeList = (
qr{${Ident}_handler},
qr{${Ident}_handler_fn},
qr{target_(?:u)?long},
+   qr{hwaddr},
 );
 
 # This can be modified by sub possible.  Since it can be empty, be careful

Re: [Qemu-devel] [PATCH] checkpatch: Error if signal(2) is used non-portably.

[Richard W.M. Jones]

Ignore this, Paolo also sent a similar patch after we were discussing
this on IRC.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [Qemu-devel] [PATCH] checkpatch: should not use signal except for SIG_DFL or SIG_IGN

[Richard W.M. Jones]

On Tue, Jul 04, 2017 at 11:27:04AM +0200, Paolo Bonzini wrote:
> Using signal to establish a signal handler is not portable; on
> SysV systems, the signal handler would be reset to SIG_DFL after
> delivery, while BSD preserves the signal handler.  Daniel Berrange
> reported that (to complicate matters further) the signal system call
> has SysV behavior, but glibc signal() actually calls the sigaction
> system call to provide BSD behavior.
> 
> However, using signal() to set a signal's disposition to SIG_DFL
> or SIG_IGN is portable and is a relatively common occurrence in
> QEMU source code, so allow that.
> 
> Signed-off-by: Paolo Bonzini 

This looks good.  Ignore the parallel version which I just sent :-(

  Reviewed-by: Richard W.M. Jones 

Rich.

>  scripts/checkpatch.pl | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 45027b9281..73efc927a9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2473,6 +2473,10 @@ sub process {
>   if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
>   ERROR("consider using qemu_$1 in preference to $1\n" . 
> $herecurr);
>   }
> +# recommend sigaction over signal for portability, when establishing a 
> handler
> + if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
> + ERROR("use sigaction to establish signal handlers; 
> signal is not portable\n" . $herecurr);
> + }
>  # check for module_init(), use category-specific init macros explicitly 
> please
>   if ($line =~ /^module_init\s*\(/) {
>   ERROR("please use block_init(), type_init() etc. 
> instead of module_init()\n" . $herecurr);
> -- 
> 2.13.0
> 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

[Qemu-devel] [PATCH] checkpatch: Error if signal(2) is used non-portably.

[Richard W.M. Jones]

The only portable use for signal(2) is setting a signal to SIG_IGN or
SIG_DFL.  Everything else is a portability minefield.  This change
adds such a check to checkpatch.  It gives an error like this:

  ERROR: Use sigaction instead of signal, except when setting the handler to 
SIG_IGN or SIG_DFL
  #39: FILE: qemu-nbd.c:585:
  +signal(SIGPIPE, foobar);

  total: 1 errors, 0 warnings, 10 lines checked

Signed-off-by: Richard W.M. Jones 
---
 scripts/checkpatch.pl | 9 +
 1 file changed, 9 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 45027b9281..76a2a87b25 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2593,6 +2593,15 @@ sub process {
$line =~ /\b(?:$non_exit_glib_asserts)\(/) {
ERROR("Use g_assert or g_assert_not_reached\n". 
$herecurr);
}
+
+# signal(2) can only portably be used for SIG_IGN or SIG_DFL.  For
+# everything else, sigaction should be used instead.
+if ($line =~ /\bsignal\([^,]+, ([^,\)]+)/ &&
+$1 !~ /^SIG_(IGN|DFL)$/) {
+ERROR("Use sigaction instead of signal, ".
+  "except when setting the handler to ".
+  "SIG_IGN or SIG_DFL\n" . $herecurr);
+}
}
 
# If we have no input at all, then there is nothing to report on
-- 
2.13.2

Re: [Qemu-devel] [PATCH] checkpatch: should not use signal except for SIG_DFL or SIG_IGN

[Daniel P. Berrange]

On Tue, Jul 04, 2017 at 11:27:04AM +0200, Paolo Bonzini wrote:
> Using signal to establish a signal handler is not portable; on
> SysV systems, the signal handler would be reset to SIG_DFL after
> delivery, while BSD preserves the signal handler.  Daniel Berrange
> reported that (to complicate matters further) the signal system call
> has SysV behavior, but glibc signal() actually calls the sigaction
> system call to provide BSD behavior.
> 
> However, using signal() to set a signal's disposition to SIG_DFL
> or SIG_IGN is portable and is a relatively common occurrence in
> QEMU source code, so allow that.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  scripts/checkpatch.pl | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Daniel P. Berrange 

We currently only have 1 violation of this rule, and that
is in the TCG test suite, so pretty harmless.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 45027b9281..73efc927a9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2473,6 +2473,10 @@ sub process {
>   if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
>   ERROR("consider using qemu_$1 in preference to $1\n" . 
> $herecurr);
>   }
> +# recommend sigaction over signal for portability, when establishing a 
> handler
> + if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
> + ERROR("use sigaction to establish signal handlers; 
> signal is not portable\n" . $herecurr);
> + }
>  # check for module_init(), use category-specific init macros explicitly 
> please
>   if ($line =~ /^module_init\s*\(/) {
>   ERROR("please use block_init(), type_init() etc. 
> instead of module_init()\n" . $herecurr);
> -- 
> 2.13.0
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH] checkpatch: should not use signal except for SIG_DFL or SIG_IGN

[Paolo Bonzini]

Using signal to establish a signal handler is not portable; on
SysV systems, the signal handler would be reset to SIG_DFL after
delivery, while BSD preserves the signal handler.  Daniel Berrange
reported that (to complicate matters further) the signal system call
has SysV behavior, but glibc signal() actually calls the sigaction
system call to provide BSD behavior.

However, using signal() to set a signal's disposition to SIG_DFL
or SIG_IGN is portable and is a relatively common occurrence in
QEMU source code, so allow that.

Signed-off-by: Paolo Bonzini 
---
 scripts/checkpatch.pl | 4 
 1 file changed, 4 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 45027b9281..73efc927a9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2473,6 +2473,10 @@ sub process {
if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
ERROR("consider using qemu_$1 in preference to $1\n" . 
$herecurr);
}
+# recommend sigaction over signal for portability, when establishing a handler
+   if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
+   ERROR("use sigaction to establish signal handlers; 
signal is not portable\n" . $herecurr);
+   }
 # check for module_init(), use category-specific init macros explicitly please
if ($line =~ /^module_init\s*\(/) {
ERROR("please use block_init(), type_init() etc. 
instead of module_init()\n" . $herecurr);
-- 
2.13.0

Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Eric Blake]

On 04/28/2017 10:10 AM, Daniel P. Berrange wrote:

>>> Or could we perhaps instead undo the damage via a hack like
>>>
>>>  #define g_assert_cmpint g_assert_cmpint_orig
>>>  #define g_assert_cmpint(x, y, z) \
>>>  g_assert_cmpint_orig(x, y,x); \
>>>  abort()

Not quite the right hack (we don't want to unconditionally abort, but
only when the condition fails).

>>
>> I'd be kind of OK adding a q_assert_cmpint if you wanted,
>> but I think we shouldn't change the semantics of a public
>> name.

I tend to agree there; having our own distinct name means that we can
see at a glance that our version will quit, no matter what the glib
version does.

> 
> Personally I think it would be worth having them - the whole point of
> these more specific g_assert_* macros is that they provide clearer
> error messages when they're triggered, so I prefer their use generally

I agree that the improved error messages part is worthwhile.  So maybe
we want:

#define q_assert_cmpint(x, y, z) \
  do { \
g_assert_cmpint(x, y, z); \
assert(x y z); \
  } while (0)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Daniel P. Berrange]

On Fri, Apr 28, 2017 at 02:45:32PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berra...@redhat.com) wrote:
> > On Thu, Apr 27, 2017 at 05:55:26PM +0100, Dr. David Alan Gilbert (git) 
> > wrote:
> > > From: "Dr. David Alan Gilbert" 
> > > 
> > > Glib commit a6a875068779 (from 2013) made many of the glib assert
> > > macros non-fatal if a flag is set.
> > > This causes two problems:
> > >   a) Compilers moan that your code is unsafe even though you've
> > >  put an assert in before the point of use.
> > >   b) Someone evil could, in a library, call
> > >  g_test_set_nonfatal_assertions() and cause our assertions in
> > >  important places not to fail and potentially allow memory overruns.
> > > 
> > > Ban most of the glib assertion functions (basically everything except
> > > g_assert and g_assert_not_reached) except in tests/
> > > 
> > > This makes checkpatch gives an error such as:
> > > 
> > >   ERROR: Use g_assert or g_assert_not_reached
> > >   #77: FILE: vl.c:4725:
> > >   +g_assert_cmpstr("Chocolate", >, "Cheese");
> > 
> > Or could we perhaps instead undo the damage via a hack like
> > 
> >  #define g_assert_cmpint g_assert_cmpint_orig
> >  #define g_assert_cmpint(x, y, z) \
> >  g_assert_cmpint_orig(x, y,x); \
> >  abort()
> 
> I'd be kind of OK adding a q_assert_cmpint if you wanted,
> but I think we shouldn't change the semantics of a public
> name.

Personally I think it would be worth having them - the whole point of
these more specific g_assert_* macros is that they provide clearer
error messages when they're triggered, so I prefer their use generally


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Eric Blake]

On 04/28/2017 08:34 AM, Markus Armbruster wrote:

>>
>> Ban most of the glib assertion functions (basically everything except
>> g_assert and g_assert_not_reached) except in tests/

You'll also want to exclude scripts/ and possible include/glib-compat.h...


> If these are screwy enough to warrant rejecting them in new code,
> they're probably screwy enough to purge them from existing code:
> 
> $ git-grep -E 
> 'g_assert_cmpstr|g_assert_cmpint|g_assert_cmpuint|g_assert_cmphex|g_assert_cmpfloat|g_assert_true|g_assert_false|g_assert_nonnull|g_assert_null|g_assert_no_error|g_assert_error|g_test_assert_expected_messages|g_test_trap_assert_passed|g_test_trap_assert_stdout|g_test_trap_assert_stdout_unmatched|g_test_trap_assert_stderr|g_test_trap_assert_stderr_unmatched'
>  | grep -v ^tests/
> hw/ide/ahci.c:g_assert_cmpint(size, >, 1);
> hw/ppc/spapr_ovec.c:g_assert_cmpint(bitnr, <, OV_MAXBITS);
> hw/ppc/spapr_ovec.c:g_assert_cmpint(bitnr, <, OV_MAXBITS);
> hw/ppc/spapr_ovec.c:g_assert_cmpint(bitnr, <, OV_MAXBITS);
> hw/ppc/spapr_ovec.c:g_assert_cmpint(vector, >=, 1); /* vector 
> numbering starts at 1 */
> hw/ppc/spapr_ovec.c:g_assert_cmpint(vector_len, <=, OV_MAXBYTES);
> hw/ppc/spapr_ovec.c:g_assert_cmpint(vec_len, <=, OV_MAXBYTES);

Those should go.

> include/glib-compat.h:#ifndef g_assert_true
> include/glib-compat.h:#define g_assert_true(expr) 
>\
> include/glib-compat.h:#ifndef g_assert_false
> include/glib-compat.h:#define g_assert_false(expr)
>\
> include/glib-compat.h:#ifndef g_assert_null
> include/glib-compat.h:#define g_assert_null(expr) 
>\
> include/glib-compat.h:#ifndef g_assert_nonnull
> include/glib-compat.h:#define g_assert_nonnull(expr)  
>\

We still need these until we can require glib 2.38 or even 2.40.

> qom/object.c:g_assert_cmpint(parent->class_size, <=, 
> ti->class_size);
> qom/object.c:g_assert_cmpint(type->instance_size, >=, sizeof(Object));
> qom/object.c:g_assert_cmpint(size, >=, type->instance_size);
> qom/object.c:g_assert_cmpint(obj->ref, ==, 0);
> qom/object.c:g_assert_cmpint(obj->ref, >, 0);

These should go.

> scripts/cocci-macro-file.h:#define g_assert_cmpint(a, op, b)   g_assert(a 
> op b)
> scripts/cocci-macro-file.h:#define g_assert_cmpuint(a, op, b)   
> g_assert(a op b)
> scripts/cocci-macro-file.h:#define g_assert_cmphex(a, op, b)   g_assert(a 
> op b)
> scripts/cocci-macro-file.h:#define g_assert_cmpstr(a, op, b)   
> g_assert(strcmp(a, b) op 0)

These must stay permanently.

> util/qht.c:g_assert_cmpuint(new->n_buckets, !=, old->n_buckets);
> 
> Volunteers?
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Peter Maydell]

On 28 April 2017 at 14:34, Markus Armbruster  wrote:
> If these are screwy enough to warrant rejecting them in new code,
> they're probably screwy enough to purge them from existing code:
>

> include/glib-compat.h:#ifndef g_assert_true
> include/glib-compat.h:#define g_assert_true(expr)

These ones prompted me to wonder if we could/should use
#pragma GCC poison g_assert_true

rather than just leaving it up to checkpatch.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Thu, Apr 27, 2017 at 05:55:26PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" 
> > 
> > Glib commit a6a875068779 (from 2013) made many of the glib assert
> > macros non-fatal if a flag is set.
> > This causes two problems:
> >   a) Compilers moan that your code is unsafe even though you've
> >  put an assert in before the point of use.
> >   b) Someone evil could, in a library, call
> >  g_test_set_nonfatal_assertions() and cause our assertions in
> >  important places not to fail and potentially allow memory overruns.
> > 
> > Ban most of the glib assertion functions (basically everything except
> > g_assert and g_assert_not_reached) except in tests/
> > 
> > This makes checkpatch gives an error such as:
> > 
> >   ERROR: Use g_assert or g_assert_not_reached
> >   #77: FILE: vl.c:4725:
> >   +g_assert_cmpstr("Chocolate", >, "Cheese");
> 
> Or could we perhaps instead undo the damage via a hack like
> 
>  #define g_assert_cmpint g_assert_cmpint_orig
>  #define g_assert_cmpint(x, y, z) \
>  g_assert_cmpint_orig(x, y,x); \
>  abort()

I'd be kind of OK adding a q_assert_cmpint if you wanted,
but I think we shouldn't change the semantics of a public
name.

Dave

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Daniel P. Berrange]

On Thu, Apr 27, 2017 at 05:55:26PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Glib commit a6a875068779 (from 2013) made many of the glib assert
> macros non-fatal if a flag is set.
> This causes two problems:
>   a) Compilers moan that your code is unsafe even though you've
>  put an assert in before the point of use.
>   b) Someone evil could, in a library, call
>  g_test_set_nonfatal_assertions() and cause our assertions in
>  important places not to fail and potentially allow memory overruns.
> 
> Ban most of the glib assertion functions (basically everything except
> g_assert and g_assert_not_reached) except in tests/
> 
> This makes checkpatch gives an error such as:
> 
>   ERROR: Use g_assert or g_assert_not_reached
>   #77: FILE: vl.c:4725:
>   +g_assert_cmpstr("Chocolate", >, "Cheese");

Or could we perhaps instead undo the damage via a hack like

 #define g_assert_cmpint g_assert_cmpint_orig
 #define g_assert_cmpint(x, y, z) \
 g_assert_cmpint_orig(x, y,x); \
 abort()


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Markus Armbruster]

"Dr. David Alan Gilbert (git)"  writes:

> From: "Dr. David Alan Gilbert" 
>
> Glib commit a6a875068779 (from 2013) made many of the glib assert
> macros non-fatal if a flag is set.
> This causes two problems:
>   a) Compilers moan that your code is unsafe even though you've
>  put an assert in before the point of use.
>   b) Someone evil could, in a library, call
>  g_test_set_nonfatal_assertions() and cause our assertions in
>  important places not to fail and potentially allow memory overruns.
>
> Ban most of the glib assertion functions (basically everything except
> g_assert and g_assert_not_reached) except in tests/
>
> This makes checkpatch gives an error such as:
>
>   ERROR: Use g_assert or g_assert_not_reached
>   #77: FILE: vl.c:4725:
>   +g_assert_cmpstr("Chocolate", >, "Cheese");
>
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  scripts/checkpatch.pl | 21 +
>  1 file changed, 21 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f084542934..73cee81b79 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2571,6 +2571,27 @@ sub process {
>   if ($line =~ /\bbzero\(/) {
>   ERROR("use memset() instead of bzero()\n" . $herecurr);
>   }
> + my $non_exit_glib_asserts = qr{g_assert_cmpstr|
> + g_assert_cmpint|
> + g_assert_cmpuint|
> + g_assert_cmphex|
> + g_assert_cmpfloat|
> + g_assert_true|
> + g_assert_false|
> + g_assert_nonnull|
> + g_assert_null|
> + g_assert_no_error|
> + g_assert_error|
> + g_test_assert_expected_messages|
> + g_test_trap_assert_passed|
> + g_test_trap_assert_stdout|
> + 
> g_test_trap_assert_stdout_unmatched|
> + g_test_trap_assert_stderr|
> + 
> g_test_trap_assert_stderr_unmatched}x;
> + if ($realfile !~ /^tests\// &&
> + $line =~ /\b(?:$non_exit_glib_asserts)\(/) {
> + ERROR("Use g_assert or g_assert_not_reached\n". 
> $herecurr);
> + }
>   }
>  
>   # If we have no input at all, then there is nothing to report on

If these are screwy enough to warrant rejecting them in new code,
they're probably screwy enough to purge them from existing code:

$ git-grep -E 
'g_assert_cmpstr|g_assert_cmpint|g_assert_cmpuint|g_assert_cmphex|g_assert_cmpfloat|g_assert_true|g_assert_false|g_assert_nonnull|g_assert_null|g_assert_no_error|g_assert_error|g_test_assert_expected_messages|g_test_trap_assert_passed|g_test_trap_assert_stdout|g_test_trap_assert_stdout_unmatched|g_test_trap_assert_stderr|g_test_trap_assert_stderr_unmatched'
 | grep -v ^tests/
hw/ide/ahci.c:g_assert_cmpint(size, >, 1);
hw/ppc/spapr_ovec.c:g_assert_cmpint(bitnr, <, OV_MAXBITS);
hw/ppc/spapr_ovec.c:g_assert_cmpint(bitnr, <, OV_MAXBITS);
hw/ppc/spapr_ovec.c:g_assert_cmpint(bitnr, <, OV_MAXBITS);
hw/ppc/spapr_ovec.c:g_assert_cmpint(vector, >=, 1); /* vector numbering 
starts at 1 */
hw/ppc/spapr_ovec.c:g_assert_cmpint(vector_len, <=, OV_MAXBYTES);
hw/ppc/spapr_ovec.c:g_assert_cmpint(vec_len, <=, OV_MAXBYTES);
include/glib-compat.h:#ifndef g_assert_true
include/glib-compat.h:#define g_assert_true(expr)   
 \
include/glib-compat.h:#ifndef g_assert_false
include/glib-compat.h:#define g_assert_false(expr)  
 \
include/glib-compat.h:#ifndef g_assert_null
include/glib-compat.h:#define g_assert_null(expr)   
 \
include/glib-compat.h:#ifndef g_assert_nonnull
include/glib-compat.h:#define g_assert_nonnull(expr)
 \
qom/object.c:g_assert_cmpint(parent->class_size, <=, 
ti->class_size);
qom/object.c:g_assert_cmpint(type->instance_size, >=, sizeof(Object));
qom/object.c:g_assert_cmpint(size, >=, type->instance_size);
qom/object.c:g_assert_cmpint(obj->ref, ==, 0);
qom/object.c:g_assert_cmpint(obj->ref, >, 0);
scripts/cocci-macro-file.h:#define g_assert_cmpint(a, op, b)   g_assert(a 
op b)
scripts/cocci-macro-file.h:#define 

Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Paolo Bonzini]

- Original Message -
> From: "Dr. David Alan Gilbert (git)" <dgilb...@redhat.com>
> To: qemu-devel@nongnu.org, pbonz...@redhat.com, arm...@redhat.com
> Sent: Thursday, April 27, 2017 6:55:26 PM
> Subject: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code
> 
> From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> 
> Glib commit a6a875068779 (from 2013) made many of the glib assert
> macros non-fatal if a flag is set.
> This causes two problems:
>   a) Compilers moan that your code is unsafe even though you've
>  put an assert in before the point of use.
>   b) Someone evil could, in a library, call
>  g_test_set_nonfatal_assertions() and cause our assertions in
>  important places not to fail and potentially allow memory overruns.
> 
> Ban most of the glib assertion functions (basically everything except
> g_assert and g_assert_not_reached) except in tests/
> 
> This makes checkpatch gives an error such as:
> 
>   ERROR: Use g_assert or g_assert_not_reached
>   #77: FILE: vl.c:4725:
>   +g_assert_cmpstr("Chocolate", >, "Cheese");
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> ---
>  scripts/checkpatch.pl | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f084542934..73cee81b79 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2571,6 +2571,27 @@ sub process {
>   if ($line =~ /\bbzero\(/) {
>   ERROR("use memset() instead of bzero()\n" . $herecurr);
>   }
> + my $non_exit_glib_asserts = qr{g_assert_cmpstr|
> + g_assert_cmpint|
> + g_assert_cmpuint|
> + g_assert_cmphex|
> + g_assert_cmpfloat|
> + g_assert_true|
> + g_assert_false|
> + g_assert_nonnull|
> + g_assert_null|
> + g_assert_no_error|
> + g_assert_error|
> + g_test_assert_expected_messages|
> + g_test_trap_assert_passed|
> + g_test_trap_assert_stdout|
> + 
> g_test_trap_assert_stdout_unmatched|
> + g_test_trap_assert_stderr|
> + 
> g_test_trap_assert_stderr_unmatched}x;
> + if ($realfile !~ /^tests\// &&
> + $line =~ /\b(?:$non_exit_glib_asserts)\(/) {
> + ERROR("Use g_assert or g_assert_not_reached\n". 
> $herecurr);
> + }
>   }
>  
>   # If we have no input at all, then there is nothing to report on
> --
> 2.12.2
> 
> 
> 

Queued, thanks.

Paolo

[Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Glib commit a6a875068779 (from 2013) made many of the glib assert
macros non-fatal if a flag is set.
This causes two problems:
  a) Compilers moan that your code is unsafe even though you've
 put an assert in before the point of use.
  b) Someone evil could, in a library, call
 g_test_set_nonfatal_assertions() and cause our assertions in
 important places not to fail and potentially allow memory overruns.

Ban most of the glib assertion functions (basically everything except
g_assert and g_assert_not_reached) except in tests/

This makes checkpatch gives an error such as:

  ERROR: Use g_assert or g_assert_not_reached
  #77: FILE: vl.c:4725:
  +g_assert_cmpstr("Chocolate", >, "Cheese");

Signed-off-by: Dr. David Alan Gilbert 
---
 scripts/checkpatch.pl | 21 +
 1 file changed, 21 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f084542934..73cee81b79 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2571,6 +2571,27 @@ sub process {
if ($line =~ /\bbzero\(/) {
ERROR("use memset() instead of bzero()\n" . $herecurr);
}
+   my $non_exit_glib_asserts = qr{g_assert_cmpstr|
+   g_assert_cmpint|
+   g_assert_cmpuint|
+   g_assert_cmphex|
+   g_assert_cmpfloat|
+   g_assert_true|
+   g_assert_false|
+   g_assert_nonnull|
+   g_assert_null|
+   g_assert_no_error|
+   g_assert_error|
+   g_test_assert_expected_messages|
+   g_test_trap_assert_passed|
+   g_test_trap_assert_stdout|
+   
g_test_trap_assert_stdout_unmatched|
+   g_test_trap_assert_stderr|
+   
g_test_trap_assert_stderr_unmatched}x;
+   if ($realfile !~ /^tests\// &&
+   $line =~ /\b(?:$non_exit_glib_asserts)\(/) {
+   ERROR("Use g_assert or g_assert_not_reached\n". 
$herecurr);
+   }
}
 
# If we have no input at all, then there is nothing to report on
-- 
2.12.2

Re: [Qemu-devel] [PATCH] checkpatch: Supress warning in function pointer typedefs

[Vinzenz Feenstra]

> On Mar 21, 2017, at 1:37 PM, Peter Maydell  wrote:
> 
> On 16 March 2017 at 11:14, Vinzenz 'evilissimo' Feenstra
>  wrote:
>> From: Vinzenz Feenstra 
>> 
>> When importing dynamically functions via `GetProcAddress` in windows
>> related code, it is quite common to make a typedef for the resulting
>> function pointer. When the function to be imported, has a stdcall
>> calling convention, usually the `WINAPI` macro is used. This patch adds an
>> exception in the checkpatch.pl script to allow the calling convention
>> specification in function pointer typedefs, to be `WINAPI`.
>> 
>> Signed-off-by: Vinzenz Feenstra 
> 
> Could you provide an example of the kind of source line that provokes
> the incorrect warning and the checkpatch output that results,
> please?

Hmm I just realize that it is working now, but I figured out now what is the 
real problem and that
my patch is wrong. Please discard this patch and thanks for making me think 
again :-)

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] checkpatch: Supress warning in function pointer typedefs

[Peter Maydell]

On 16 March 2017 at 11:14, Vinzenz 'evilissimo' Feenstra
 wrote:
> From: Vinzenz Feenstra 
>
> When importing dynamically functions via `GetProcAddress` in windows
> related code, it is quite common to make a typedef for the resulting
> function pointer. When the function to be imported, has a stdcall
> calling convention, usually the `WINAPI` macro is used. This patch adds an
> exception in the checkpatch.pl script to allow the calling convention
> specification in function pointer typedefs, to be `WINAPI`.
>
> Signed-off-by: Vinzenz Feenstra 

Could you provide an example of the kind of source line that provokes
the incorrect warning and the checkpatch output that results,
please?

thanks
-- PMM

[Qemu-devel] [PATCH] checkpatch: Supress warning in function pointer typedefs

[Vinzenz 'evilissimo' Feenstra]

From: Vinzenz Feenstra 

When importing dynamically functions via `GetProcAddress` in windows
related code, it is quite common to make a typedef for the resulting
function pointer. When the function to be imported, has a stdcall
calling convention, usually the `WINAPI` macro is used. This patch adds an
exception in the checkpatch.pl script to allow the calling convention
specification in function pointer typedefs, to be `WINAPI`.

Signed-off-by: Vinzenz Feenstra 
---
 scripts/checkpatch.pl | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f084542..33bf585 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1774,7 +1774,14 @@ sub process {
# likely a typedef for a function.
} elsif ($ctx =~ /$Type$/) {
 
-   } else {
+# If this is a typedef we need to allow WINAPI as a calling
+# convention. Even though there should be only one space around the
+# star, we allow none or any, to suppress the following warning.
+# The check for the number of spaces around the star is checked
+# elsewhere.
+   } elsif($ctx =~ 
/^\s*typedef\s+$Type\(WINAPI\s*\*\s*$Ident\)/) {
+
+} else {
ERROR("space prohibited between function name 
and open parenthesis '('\n" . $herecurr);
}
}
-- 
2.9.3

Re: [Qemu-devel] [PATCH] checkpatch: allow longer lines for logging functions

[Greg Kurz]

On Fri, 17 Mar 2017 03:45:56 -0700 (PDT)
no-re...@patchew.org wrote:

> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 148974701986.29545.5414999102981738774.stgit@bahia
> Subject: [Qemu-devel] [PATCH] checkpatch: allow longer lines for logging 
> functions
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
> echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; 
> then
> failed=1
> echo
> fi
> n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag] 
> patchew/148974701986.29545.5414999102981738774.stgit@bahia -> 
> patchew/148974701986.29545.5414999102981738774.stgit@bahia
>  * [new tag] 
> patchew/148974744281.30636.17973264008285415592.stgit@bahia -> 
> patchew/148974744281.30636.17973264008285415592.stgit@bahia
> Switched to a new branch 'test'
> 390d7c4 checkpatch: allow longer lines for logging functions
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/1: checkpatch: allow longer lines for logging functions...
> ERROR: line over 90 characters

Ha ha ! Should checkpatch allow long regexp lines in itself ? ;)

> #38: FILE: scripts/checkpatch.pl:1349:
> + !($line =~ 
> /^\+\s*$logFunctions\s*\(\s*(?:([^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ ||
> 
> total: 1 errors, 0 warnings, 20 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-de...@freelists.org


pgp2tH0mj8iPN.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] checkpatch: allow longer lines for logging functions

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 148974701986.29545.5414999102981738774.stgit@bahia
Subject: [Qemu-devel] [PATCH] checkpatch: allow longer lines for logging 
functions
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/148974701986.29545.5414999102981738774.stgit@bahia 
-> patchew/148974701986.29545.5414999102981738774.stgit@bahia
 * [new tag] 
patchew/148974744281.30636.17973264008285415592.stgit@bahia -> 
patchew/148974744281.30636.17973264008285415592.stgit@bahia
Switched to a new branch 'test'
390d7c4 checkpatch: allow longer lines for logging functions

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: allow longer lines for logging functions...
ERROR: line over 90 characters
#38: FILE: scripts/checkpatch.pl:1349:
+   !($line =~ 
/^\+\s*$logFunctions\s*\(\s*(?:([^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ ||

total: 1 errors, 0 warnings, 20 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] checkpatch: allow longer lines for logging functions

[Greg Kurz]

Commit f1e155bbf863a removed a bunch of stuff that really don't make sense
outside the linux kernel. An exception though is logging functions: it is
convenient to be able to grep error messages in the code. For this to work,
error strings mustn't be broken down on multiple lines, and therefore are
likely to overcome the 90 characters limit, and make checkpatch unhappy.

This patch reverts the change for logging functions and adapts it to QEMU.

Signed-off-by: Greg Kurz 
---
 scripts/checkpatch.pl |8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f0845429342a..cc2796238170 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -192,6 +192,11 @@ our $typeTypedefs = qr{(?x:
 | QEMUBH# all uppercase
 )};
 
+our $logFunctions = qr{(?x:
+   error_report|
+   error_printf
+)};
+
 our @typeList = (
qr{void},
qr{(?:unsigned\s+)?char},
@@ -1341,7 +1346,8 @@ sub process {
 
 #90 column limit
if ($line =~ /^\+/ &&
-   !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
+   !($line =~ 
/^\+\s*$logFunctions\s*\(\s*(?:([^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ ||
+ $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
$length > 80)
{
if ($length > 90) {

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Markus Armbruster]

Paolo Bonzini  writes:

> On 22/09/2016 09:12, Markus Armbruster wrote:
>> Paolo Bonzini  writes:
>> 
>>> ---
>>>  scripts/checkpatch.pl | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index dde3f5f..3afa19a 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -2407,7 +2407,7 @@ sub process {
>>>  # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
>>>  # where they might be necessary.
>>> if ($line =~ m@^.\s*\#\s*if.*\b__@) {
>>> -   ERROR("architecture specific defines should be 
>>> avoided\n" .  $herecurr);
>>> +   WARN("architecture specific defines should be 
>>> avoided\n" .  $herecurr);
>>> }
>>>  
>>>  # Check that the storage class is at the beginning of a declaration
>> 
>> git-grep finds almost 400 of them.  We certainly want people to think
>> twice (or thrice) before they add more.  The question to discuss here is
>> whether we want to force that thinking onto the list.  If yes, keep
>> ERROR.  If no, downgrade to warn.
>
> I actually count 450, but:
>
> - about a 100 are in imported code (disas/libvixl,
> include/standard-headers and linux-headers, disas)
>
> - another 40-odd hits are __NR_* syscall numbers
>
> - about 80 are in user-exec.c, block/raw-posix.c, util/oslib-posix.c,
> util/qemu-openpty.c, util/qemu-thread-posix.c which is probably unavoidable
>
> - another 30 are in tcg
>
> So this already covers more than half the hits.
>
>
> The patch is a bit of a heavy hammer, but I don't think it's an endemic
> problem that warrants a complaint on the list.  If we want to keep the
> error, I think we should have:
>
> - a symbol blacklist.  For example __linux__ and _WIN32 can be trivially
> replaced by CONFIG_LINUX and CONFIG_WIN32, and __GNUC__ is probably a
> bad idea (but __clang__ not so much; clang defines __GNUC__ for an
> absurdly old version).
>
> - a file blacklist, for example I would not expect target-*/ and hw/
> should not have __ symbols and in fact they hardly have any
>
> and warn for everything else.  Something for bite-sized tasks?

Works for me.

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Paolo Bonzini]

On 22/09/2016 09:12, Markus Armbruster wrote:
> Paolo Bonzini  writes:
> 
>> ---
>>  scripts/checkpatch.pl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index dde3f5f..3afa19a 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2407,7 +2407,7 @@ sub process {
>>  # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
>>  # where they might be necessary.
>>  if ($line =~ m@^.\s*\#\s*if.*\b__@) {
>> -ERROR("architecture specific defines should be 
>> avoided\n" .  $herecurr);
>> +WARN("architecture specific defines should be 
>> avoided\n" .  $herecurr);
>>  }
>>  
>>  # Check that the storage class is at the beginning of a declaration
> 
> git-grep finds almost 400 of them.  We certainly want people to think
> twice (or thrice) before they add more.  The question to discuss here is
> whether we want to force that thinking onto the list.  If yes, keep
> ERROR.  If no, downgrade to warn.

I actually count 450, but:

- about a 100 are in imported code (disas/libvixl,
include/standard-headers and linux-headers, disas)

- another 40-odd hits are __NR_* syscall numbers

- about 80 are in user-exec.c, block/raw-posix.c, util/oslib-posix.c,
util/qemu-openpty.c, util/qemu-thread-posix.c which is probably unavoidable

- another 30 are in tcg

So this already covers more than half the hits.


The patch is a bit of a heavy hammer, but I don't think it's an endemic
problem that warrants a complaint on the list.  If we want to keep the
error, I think we should have:

- a symbol blacklist.  For example __linux__ and _WIN32 can be trivially
replaced by CONFIG_LINUX and CONFIG_WIN32, and __GNUC__ is probably a
bad idea (but __clang__ not so much; clang defines __GNUC__ for an
absurdly old version).

- a file blacklist, for example I would not expect target-*/ and hw/
should not have __ symbols and in fact they hardly have any

and warn for everything else.  Something for bite-sized tasks?

Paolo

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Markus Armbruster]

Paolo Bonzini  writes:

> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index dde3f5f..3afa19a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2407,7 +2407,7 @@ sub process {
>  # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
>  # where they might be necessary.
>   if ($line =~ m@^.\s*\#\s*if.*\b__@) {
> - ERROR("architecture specific defines should be 
> avoided\n" .  $herecurr);
> + WARN("architecture specific defines should be 
> avoided\n" .  $herecurr);
>   }
>  
>  # Check that the storage class is at the beginning of a declaration

git-grep finds almost 400 of them.  We certainly want people to think
twice (or thrice) before they add more.  The question to discuss here is
whether we want to force that thinking onto the list.  If yes, keep
ERROR.  If no, downgrade to warn.

Re: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1474476607-4990-1-git-send-email-pbonz...@redhat.com
Subject: [Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific 
defines should be avoided"

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  
patchew/1474435433-9029-1-git-send-email-da...@gibson.dropbear.id.au -> 
patchew/1474435433-9029-1-git-send-email-da...@gibson.dropbear.id.au
 * [new tag] 
patchew/1474476607-4990-1-git-send-email-pbonz...@redhat.com -> 
patchew/1474476607-4990-1-git-send-email-pbonz...@redhat.com
 * [new tag] patchew/147447700612.30952.9420141963781948805.stgit@bahia 
-> patchew/147447700612.30952.9420141963781948805.stgit@bahia
 * [new tag] 
patchew/1474477573-6386-1-git-send-email-lviv...@redhat.com -> 
patchew/1474477573-6386-1-git-send-email-lviv...@redhat.com
Switched to a new branch 'test'
ada0573 checkpatch: downgrade "architecture specific defines should be avoided"

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: downgrade "architecture specific defines should 
be avoided"...
ERROR: line over 90 characters
#18: FILE: scripts/checkpatch.pl:2410:
+   WARN("architecture specific defines should be 
avoided\n" .  $herecurr);

ERROR: Missing Signed-off-by: line(s)

total: 2 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] checkpatch: downgrade "architecture specific defines should be avoided"

[Paolo Bonzini]

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dde3f5f..3afa19a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2407,7 +2407,7 @@ sub process {
 # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
 # where they might be necessary.
if ($line =~ m@^.\s*\#\s*if.*\b__@) {
-   ERROR("architecture specific defines should be 
avoided\n" .  $herecurr);
+   WARN("architecture specific defines should be 
avoided\n" .  $herecurr);
}
 
 # Check that the storage class is at the beginning of a declaration
-- 
2.7.4

Re: [Qemu-devel] [PATCH] checkpatch: Fix whitespace checks for documentation code blocks

[Lluís Vilanova]

Paolo Bonzini writes:

> On 06/09/2016 12:30, Lluís Vilanova wrote:
>> Prevent blank lines in documentation code blocks to be signalled as
>> incorrect trailing whitespace.
>> 
>> Code blocks in documentation are 4-column aligned, and blank lines in
>> them should have exactly 4 columns of trailing whitespace to prevent
>> QEMU's wiki to render them as separate code blocks.
>> 
>> Signed-off-by: Lluís Vilanova 
>> ---
>> scripts/checkpatch.pl |   10 ++
>> 1 file changed, 10 insertions(+)
>> 
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index b0096a4..4bdfe2e 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1320,6 +1320,16 @@ sub process {
>> my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>> ERROR("DOS line endings\n" . $herevet);
>> 
>> +} elsif ($realfile =~ /^docs\/.+\.txt/ ||
>> + $realfile =~ /^docs\/.+\.md/) {
>> +if ($rawline =~ /^\+\s+$/ && $rawline !~ /^\+\s{4}$/) {

> \s includes tabs, doesn't it?  So the second condition should be just

> $rawline ne '+'

> I think.

> Paolo

So true; I'll resend a v2.

Thanks,
  Lluis

Re: [Qemu-devel] [PATCH] checkpatch: Fix whitespace checks for documentation code blocks

[Paolo Bonzini]

On 06/09/2016 12:30, Lluís Vilanova wrote:
> Prevent blank lines in documentation code blocks to be signalled as
> incorrect trailing whitespace.
> 
> Code blocks in documentation are 4-column aligned, and blank lines in
> them should have exactly 4 columns of trailing whitespace to prevent
> QEMU's wiki to render them as separate code blocks.
> 
> Signed-off-by: Lluís Vilanova 
> ---
>  scripts/checkpatch.pl |   10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b0096a4..4bdfe2e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1320,6 +1320,16 @@ sub process {
>   my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>   ERROR("DOS line endings\n" . $herevet);
>  
> + } elsif ($realfile =~ /^docs\/.+\.txt/ ||
> +  $realfile =~ /^docs\/.+\.md/) {
> + if ($rawline =~ /^\+\s+$/ && $rawline !~ /^\+\s{4}$/) {

\s includes tabs, doesn't it?  So the second condition should be just

$rawline ne '+'

I think.

Paolo


> + # TODO: properly check we're in a code block
> + #   (surrounding text is 4-column aligned)
> + my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> + ERROR("code blocks in documentation should have " .
> +   "exactly 4 columns of trailing whitespace\n" .
> +   $herevet);
> + }
>   } elsif ($rawline =~ /^\+.*\S\s+$/ || $rawline =~ /^\+\s+$/) {
>   my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>   ERROR("trailing whitespace\n" . $herevet);
> 

[Qemu-devel] [PATCH] checkpatch: Fix whitespace checks for documentation code blocks

[Lluís Vilanova]

Prevent blank lines in documentation code blocks to be signalled as
incorrect trailing whitespace.

Code blocks in documentation are 4-column aligned, and blank lines in
them should have exactly 4 columns of trailing whitespace to prevent
QEMU's wiki to render them as separate code blocks.

Signed-off-by: Lluís Vilanova 
---
 scripts/checkpatch.pl |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b0096a4..4bdfe2e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1320,6 +1320,16 @@ sub process {
my $herevet = "$here\n" . cat_vet($rawline) . "\n";
ERROR("DOS line endings\n" . $herevet);
 
+   } elsif ($realfile =~ /^docs\/.+\.txt/ ||
+$realfile =~ /^docs\/.+\.md/) {
+   if ($rawline =~ /^\+\s+$/ && $rawline !~ /^\+\s{4}$/) {
+   # TODO: properly check we're in a code block
+   #   (surrounding text is 4-column aligned)
+   my $herevet = "$here\n" . cat_vet($rawline) . "\n";
+   ERROR("code blocks in documentation should have " .
+ "exactly 4 columns of trailing whitespace\n" .
+ $herevet);
+   }
} elsif ($rawline =~ /^\+.*\S\s+$/ || $rawline =~ /^\+\s+$/) {
my $herevet = "$here\n" . cat_vet($rawline) . "\n";
ERROR("trailing whitespace\n" . $herevet);

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[Cornelia Huck]

On Wed, 10 Aug 2016 15:55:28 +0200
Radim Krčmář  wrote:

> 2016-08-10 09:09+0200, Cornelia Huck:
> > On Tue, 9 Aug 2016 12:14:14 -0400 (EDT)
> > Paolo Bonzini  wrote:
> >> > Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of
> >> > changing scripts/update-linux-headers.sh to expand tabs when importing.
> >> > 
> >> > Signed-off-by: Radim Krčmář 
> >> > ---
> >> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> > index 929708721299..38232d4b25c3 100755
> >> > --- a/scripts/checkpatch.pl
> >> > +++ b/scripts/checkpatch.pl
> >> > @@ -1355,7 +1355,7 @@ sub process {
> >> >  next if ($realfile !~ /\.(h|c|cpp|pl)$/);
> >> >  
> >> >  # in QEMU, no tabs are allowed
> >> > -if ($rawline =~ /^\+.*\t/) {
> >> > +if ($rawline =~ /^\+.*\t/ && $realfile !~ 
> >> > /^linux-headers\//) {
> >> >  my $herevet = "$here\n" . cat_vet($rawline) . 
> >> > "\n";
> >> >  ERROR("code indent should never use tabs\n" . 
> >> > $herevet);
> >> >  $rpt_cleaners = 1;
> >> > 
> >> 
> >> Could you do the same for standard-headers/ too?
> > 
> > I think it would be better to not apply any qemu coding style checks to
> > a headers update. Something like 'check if this contains header updates
> > _only_' would make more sense, but that is beyond my nonexisting perl
> > skills...
> 
> I have posted another vesion that does not check for any code style in
> hunks that modify linux-headers and include/standard-headers,
> http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg01824.html
> 
> We still want to check header-only updates in other headers ...
> Your condition would draw attention to linux header updates that also
> touch other files, but I think that a diffstat is enough.
> 
> The script would need some preprocessing to know that only headers are
> modified or buffering of errors until the script knows that only headers
> were modified; neither is hard, but the added complexity is not
> compensated by usefulness, IMO.
> 

If there's no quick way to check, it's not worth spending too much time
on it, I agree.

Re: [Qemu-devel] [PATCH] checkpatch: ignore automatically imported Linux headers

[Cornelia Huck]

On Tue,  9 Aug 2016 19:38:41 +0200
Radim Krčmář  wrote:

> Linux uses tabs for indentation and checkpatch always complained about
> automatically imported headers.  update-linux-headers.sh could be modified to
> expand tabs, but there is no real reason to complain about any ugly code in
> Linux headers, so skip all hunk-related checks.
> 
> Signed-off-by: Radim Krčmář 
> ---
>  Line offset of the diff assumes that Paolo's "[PATCH 0/3] checkpatch tweaks"
>  is applied first.  Context remains the same, though.
> 
>  scripts/checkpatch.pl | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 0f9ae512ae55..873a50292442 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1312,6 +1312,9 @@ sub process {
>  # ignore non-hunk lines and lines being removed
>   next if (!$hunk_line || $line =~ /^-/);
>  
> +# ignore files that are being periodically imported from Linux
> + next if ($realfile =~ 
> /^(linux-headers|include\/standard-headers)\//);
> +
>  #trailing whitespace
>   if ($line =~ /^\+.*\015/) {
>   my $herevet = "$here\n" . cat_vet($rawline) . "\n";

Acked-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[Radim Krčmář]

2016-08-10 09:09+0200, Cornelia Huck:
> On Tue, 9 Aug 2016 12:14:14 -0400 (EDT)
> Paolo Bonzini  wrote:
>> > Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of
>> > changing scripts/update-linux-headers.sh to expand tabs when importing.
>> > 
>> > Signed-off-by: Radim Krčmář 
>> > ---
>> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> > index 929708721299..38232d4b25c3 100755
>> > --- a/scripts/checkpatch.pl
>> > +++ b/scripts/checkpatch.pl
>> > @@ -1355,7 +1355,7 @@ sub process {
>> >next if ($realfile !~ /\.(h|c|cpp|pl)$/);
>> >  
>> >  # in QEMU, no tabs are allowed
>> > -  if ($rawline =~ /^\+.*\t/) {
>> > +  if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
>> >my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>> >ERROR("code indent should never use tabs\n" . $herevet);
>> >$rpt_cleaners = 1;
>> > 
>> 
>> Could you do the same for standard-headers/ too?
> 
> I think it would be better to not apply any qemu coding style checks to
> a headers update. Something like 'check if this contains header updates
> _only_' would make more sense, but that is beyond my nonexisting perl
> skills...

I have posted another vesion that does not check for any code style in
hunks that modify linux-headers and include/standard-headers,
http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg01824.html

We still want to check header-only updates in other headers ...
Your condition would draw attention to linux header updates that also
touch other files, but I think that a diffstat is enough.

The script would need some preprocessing to know that only headers are
modified or buffering of errors until the script knows that only headers
were modified; neither is hard, but the added complexity is not
compensated by usefulness, IMO.

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[Cornelia Huck]

On Tue, 9 Aug 2016 12:14:14 -0400 (EDT)
Paolo Bonzini  wrote:

> 
> 
> - Original Message -
> > From: "Radim Krčmář" 
> > To: no-re...@ec2-52-6-146-230.compute-1.amazonaws.com
> > Cc: f...@redhat.com, ehabk...@redhat.com, m...@redhat.com, 
> > qemu-devel@nongnu.org, pet...@redhat.com, "jan kiszka"
> > , pbonz...@redhat.com, r...@twiddle.net
> > Sent: Tuesday, August 9, 2016 6:07:04 PM
> > Subject: [PATCH] checkpatch: allow tabs in linux-headers
> > 
> > 2016-08-09 08:31-0700, no-re...@ec2-52-6-146-230.compute-1.amazonaws.com:
> > > Hi,
> > > 
> > > Your series seems to have some coding style problems. See output below for
> > > more information:
> > > 
> > > Message-id: 20160809150333.9991-1-rkrc...@redhat.com
> > > Type: series
> > > Subject: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to
> > > quirkless KVM
> > > 
> > > === TEST SCRIPT BEGIN ===
> > > #!/bin/bash
> > > 
> > > BASE=base
> > > n=1
> > > total=$(git log --oneline $BASE.. | wc -l)
> > > failed=0
> > > 
> > > commits="$(git log --format=%H --reverse $BASE..)"
> > > for c in $commits; do
> > > echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s
> > > $c)..."
> > > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback 
> > > -;
> > > then
> > > failed=1
> > > echo
> > > fi
> > > n=$((n+1))
> > > done
> > > 
> > > exit $failed
> > > === TEST SCRIPT END ===
> > > 
> > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> > > Switched to a new branch 'test'
> > > e018fb0 intel-iommu: restrict EIM to quirkless KVM
> > > 5ef6f2f linux-headers: update to v4.8-rc1
> > > 
> > > === OUTPUT BEGIN ===
> > > Checking PATCH 1/2: linux-headers: update to v4.8-rc1...
> > > ERROR: code indent should never use tabs
> > > #32: FILE: linux-headers/linux/kvm.h:885:
> > > +^Iunion {$
> > > 
> > > ERROR: code indent should never use tabs
> > > #33: FILE: linux-headers/linux/kvm.h:886:
> > > +^I^I__u32 pad;$
> > > 
> > > ERROR: code indent should never use tabs
> > > #34: FILE: linux-headers/linux/kvm.h:887:
> > > +^I^I__u32 devid;$
> > > 
> > > ERROR: code indent should never use tabs
> > > #35: FILE: linux-headers/linux/kvm.h:888:
> > > +^I};$
> > > 
> > > ERROR: code indent should never use tabs
> > > #43: FILE: linux-headers/linux/kvm.h:1034:
> > > +#define KVM_MSI_VALID_DEVID^I(1U << 0)$
> > > 
> > > ERROR: code indent should never use tabs
> > > #50: FILE: linux-headers/linux/kvm.h:1040:
> > > +^I__u32 devid;$
> > > 
> > > ERROR: code indent should never use tabs
> > > #51: FILE: linux-headers/linux/kvm.h:1041:
> > > +^I__u8  pad[12];$
> > > 
> > > ERROR: code indent should never use tabs
> > > #59: FILE: linux-headers/linux/kvm.h:1086:
> > > +^IKVM_DEV_TYPE_ARM_VGIC_ITS,$
> > > 
> > > ERROR: code indent should never use tabs
> > > #60: FILE: linux-headers/linux/kvm.h:1087:
> > > +#define KVM_DEV_TYPE_ARM_VGIC_ITS^IKVM_DEV_TYPE_ARM_VGIC_ITS$
> > > 
> > > total: 9 errors, 0 warnings, 51 lines checked
> > > 
> > > Your patch has style problems, please review.  If any of these errors
> > > are false positives report them to the maintainer, see
> > > CHECKPATCH in MAINTAINERS.
> > 
> > These indentation errors are false positives.
> > ---8<---
> > Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of
> > changing scripts/update-linux-headers.sh to expand tabs when importing.
> > 
> > Signed-off-by: Radim Krčmář 
> > ---
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 929708721299..38232d4b25c3 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1355,7 +1355,7 @@ sub process {
> > next if ($realfile !~ /\.(h|c|cpp|pl)$/);
> >  
> >  # in QEMU, no tabs are allowed
> > -   if ($rawline =~ /^\+.*\t/) {
> > +   if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
> > my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> > ERROR("code indent should never use tabs\n" . $herevet);
> > $rpt_cleaners = 1;
> > 
> 
> Could you do the same for standard-headers/ too?

I think it would be better to not apply any qemu coding style checks to
a headers update. Something like 'check if this contains header updates
_only_' would make more sense, but that is beyond my nonexisting perl
skills...

Re: [Qemu-devel] [PATCH] checkpatch: ignore automatically imported Linux headers

[Paolo Bonzini]

On 09/08/2016 19:38, Radim Krčmář wrote:
> Linux uses tabs for indentation and checkpatch always complained about
> automatically imported headers.  update-linux-headers.sh could be modified to
> expand tabs, but there is no real reason to complain about any ugly code in
> Linux headers, so skip all hunk-related checks.
> 
> Signed-off-by: Radim Krčmář 
> ---
>  Line offset of the diff assumes that Paolo's "[PATCH 0/3] checkpatch tweaks"
>  is applied first.  Context remains the same, though.
> 
>  scripts/checkpatch.pl | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 0f9ae512ae55..873a50292442 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1312,6 +1312,9 @@ sub process {
>  # ignore non-hunk lines and lines being removed
>   next if (!$hunk_line || $line =~ /^-/);
>  
> +# ignore files that are being periodically imported from Linux
> + next if ($realfile =~ 
> /^(linux-headers|include\/standard-headers)\//);
> +
>  #trailing whitespace
>   if ($line =~ /^\+.*\015/) {
>   my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH] checkpatch: ignore automatically imported Linux headers

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 20160809173841.716-1-rkrc...@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH] checkpatch: ignore automatically imported Linux 
headers

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
b43ef9c checkpatch: ignore automatically imported Linux headers

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: ignore automatically imported Linux headers...
WARNING: line over 80 characters
#26: FILE: scripts/checkpatch.pl:1323:
+   next if ($realfile =~ 
/^(linux-headers|include\/standard-headers)\//);

ERROR: code indent should never use tabs
#26: FILE: scripts/checkpatch.pl:1323:
+^I^Inext if ($realfile =~ /^(linux-headers|include\/standard-headers)\//);$

total: 1 errors, 1 warnings, 9 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] checkpatch: ignore automatically imported Linux headers

[Radim Krčmář]

Linux uses tabs for indentation and checkpatch always complained about
automatically imported headers.  update-linux-headers.sh could be modified to
expand tabs, but there is no real reason to complain about any ugly code in
Linux headers, so skip all hunk-related checks.

Signed-off-by: Radim Krčmář 
---
 Line offset of the diff assumes that Paolo's "[PATCH 0/3] checkpatch tweaks"
 is applied first.  Context remains the same, though.

 scripts/checkpatch.pl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0f9ae512ae55..873a50292442 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1312,6 +1312,9 @@ sub process {
 # ignore non-hunk lines and lines being removed
next if (!$hunk_line || $line =~ /^-/);
 
+# ignore files that are being periodically imported from Linux
+   next if ($realfile =~ 
/^(linux-headers|include\/standard-headers)\//);
+
 #trailing whitespace
if ($line =~ /^\+.*\015/) {
my $herevet = "$here\n" . cat_vet($rawline) . "\n";
-- 
2.9.2

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[Radim Krčmář]

2016-08-09 18:39+0200, Paolo Bonzini:
> On 09/08/2016 18:37, Radim Krčmář wrote:
 Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of
 changing scripts/update-linux-headers.sh to expand tabs when importing.

 Signed-off-by: Radim Krčmář 
 ---
 diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
 index 929708721299..38232d4b25c3 100755
 --- a/scripts/checkpatch.pl
 +++ b/scripts/checkpatch.pl
 @@ -1355,7 +1355,7 @@ sub process {
next if ($realfile !~ /\.(h|c|cpp|pl)$/);
  
  # in QEMU, no tabs are allowed
 -  if ($rawline =~ /^\+.*\t/) {
 +  if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
my $herevet = "$here\n" . cat_vet($rawline) . "\n";
ERROR("code indent should never use tabs\n" . $herevet);
$rpt_cleaners = 1;

>>>
>>> Could you do the same for standard-headers/ too?
>> 
>> Sure, and when we are at it ... are *.pl files going to be reindented?
>> 
>> e.g. checkpatch.pl uses tabs consistently, get_maintainer.pl is a
>> horrible mix of tabs and spaces [1], and clean-header-guards.pl uses 4
>> spaces for an indent.
> 
> I've sent a patch series to fix the most obvious issues in automated
> checkpatch email, it skips the tab check on imported Perl scripts.

I will base on that, thanks.

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[Radim Krčmář]

>> Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of
>> changing scripts/update-linux-headers.sh to expand tabs when importing.
>> 
>> Signed-off-by: Radim Krčmář 
>> ---
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 929708721299..38232d4b25c3 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1355,7 +1355,7 @@ sub process {
>>  next if ($realfile !~ /\.(h|c|cpp|pl)$/);
>>  
>>  # in QEMU, no tabs are allowed
>> -if ($rawline =~ /^\+.*\t/) {
>> +if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
>>  my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>>  ERROR("code indent should never use tabs\n" . $herevet);
>>  $rpt_cleaners = 1;
>> 
> 
> Could you do the same for standard-headers/ too?

Sure, and when we are at it ... are *.pl files going to be reindented?

e.g. checkpatch.pl uses tabs consistently, get_maintainer.pl is a
horrible mix of tabs and spaces [1], and clean-header-guards.pl uses 4
spaces for an indent.

---
1: get_maintainer.pl assumes that tab is 8 spaces and uses 1 tab for
   every two consecutive indents and 4 spaces for indents that cannot be
   tabified, then tops the ugliness by also using 8 and 12 spaces for 2
   and 3 indents, respectively.

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 20160809160703.GA10637@potion
Type: series
Subject: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
e923294 checkpatch: allow tabs in linux-headers

=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: allow tabs in linux-headers...
ERROR: code indent should never use tabs
#106: FILE: scripts/checkpatch.pl:1358:
+^I^Iif ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {$

total: 1 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[Radim Krčmář]

2016-08-09 08:31-0700, no-re...@ec2-52-6-146-230.compute-1.amazonaws.com:
> Hi,
> 
> Your series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 20160809150333.9991-1-rkrc...@redhat.com
> Type: series
> Subject: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to 
> quirkless KVM
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
> echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
> if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; 
> then
> failed=1
> echo
> fi
> n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> e018fb0 intel-iommu: restrict EIM to quirkless KVM
> 5ef6f2f linux-headers: update to v4.8-rc1
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/2: linux-headers: update to v4.8-rc1...
> ERROR: code indent should never use tabs
> #32: FILE: linux-headers/linux/kvm.h:885:
> +^Iunion {$
> 
> ERROR: code indent should never use tabs
> #33: FILE: linux-headers/linux/kvm.h:886:
> +^I^I__u32 pad;$
> 
> ERROR: code indent should never use tabs
> #34: FILE: linux-headers/linux/kvm.h:887:
> +^I^I__u32 devid;$
> 
> ERROR: code indent should never use tabs
> #35: FILE: linux-headers/linux/kvm.h:888:
> +^I};$
> 
> ERROR: code indent should never use tabs
> #43: FILE: linux-headers/linux/kvm.h:1034:
> +#define KVM_MSI_VALID_DEVID^I(1U << 0)$
> 
> ERROR: code indent should never use tabs
> #50: FILE: linux-headers/linux/kvm.h:1040:
> +^I__u32 devid;$
> 
> ERROR: code indent should never use tabs
> #51: FILE: linux-headers/linux/kvm.h:1041:
> +^I__u8  pad[12];$
> 
> ERROR: code indent should never use tabs
> #59: FILE: linux-headers/linux/kvm.h:1086:
> +^IKVM_DEV_TYPE_ARM_VGIC_ITS,$
> 
> ERROR: code indent should never use tabs
> #60: FILE: linux-headers/linux/kvm.h:1087:
> +#define KVM_DEV_TYPE_ARM_VGIC_ITS^IKVM_DEV_TYPE_ARM_VGIC_ITS$
> 
> total: 9 errors, 0 warnings, 51 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.

These indentation errors are false positives.
---8<---
Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of
changing scripts/update-linux-headers.sh to expand tabs when importing.

Signed-off-by: Radim Krčmář 
---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 929708721299..38232d4b25c3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1355,7 +1355,7 @@ sub process {
next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 
 # in QEMU, no tabs are allowed
-   if ($rawline =~ /^\+.*\t/) {
+   if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
my $herevet = "$here\n" . cat_vet($rawline) . "\n";
ERROR("code indent should never use tabs\n" . $herevet);
$rpt_cleaners = 1;

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[Paolo Bonzini]

On 09/08/2016 18:37, Radim Krčmář wrote:
>>> Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of
>>> changing scripts/update-linux-headers.sh to expand tabs when importing.
>>>
>>> Signed-off-by: Radim Krčmář 
>>> ---
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 929708721299..38232d4b25c3 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -1355,7 +1355,7 @@ sub process {
>>> next if ($realfile !~ /\.(h|c|cpp|pl)$/);
>>>  
>>>  # in QEMU, no tabs are allowed
>>> -   if ($rawline =~ /^\+.*\t/) {
>>> +   if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
>>> my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>>> ERROR("code indent should never use tabs\n" . $herevet);
>>> $rpt_cleaners = 1;
>>>
>>
>> Could you do the same for standard-headers/ too?
> 
> Sure, and when we are at it ... are *.pl files going to be reindented?
> 
> e.g. checkpatch.pl uses tabs consistently, get_maintainer.pl is a
> horrible mix of tabs and spaces [1], and clean-header-guards.pl uses 4
> spaces for an indent.

I've sent a patch series to fix the most obvious issues in automated
checkpatch email, it skips the tab check on imported Perl scripts.

Paolo

Re: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers

[Paolo Bonzini]

- Original Message -
> From: "Radim Krčmář" 
> To: no-re...@ec2-52-6-146-230.compute-1.amazonaws.com
> Cc: f...@redhat.com, ehabk...@redhat.com, m...@redhat.com, 
> qemu-devel@nongnu.org, pet...@redhat.com, "jan kiszka"
> , pbonz...@redhat.com, r...@twiddle.net
> Sent: Tuesday, August 9, 2016 6:07:04 PM
> Subject: [PATCH] checkpatch: allow tabs in linux-headers
> 
> 2016-08-09 08:31-0700, no-re...@ec2-52-6-146-230.compute-1.amazonaws.com:
> > Hi,
> > 
> > Your series seems to have some coding style problems. See output below for
> > more information:
> > 
> > Message-id: 20160809150333.9991-1-rkrc...@redhat.com
> > Type: series
> > Subject: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to
> > quirkless KVM
> > 
> > === TEST SCRIPT BEGIN ===
> > #!/bin/bash
> > 
> > BASE=base
> > n=1
> > total=$(git log --oneline $BASE.. | wc -l)
> > failed=0
> > 
> > commits="$(git log --format=%H --reverse $BASE..)"
> > for c in $commits; do
> > echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s
> > $c)..."
> > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -;
> > then
> > failed=1
> > echo
> > fi
> > n=$((n+1))
> > done
> > 
> > exit $failed
> > === TEST SCRIPT END ===
> > 
> > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> > Switched to a new branch 'test'
> > e018fb0 intel-iommu: restrict EIM to quirkless KVM
> > 5ef6f2f linux-headers: update to v4.8-rc1
> > 
> > === OUTPUT BEGIN ===
> > Checking PATCH 1/2: linux-headers: update to v4.8-rc1...
> > ERROR: code indent should never use tabs
> > #32: FILE: linux-headers/linux/kvm.h:885:
> > +^Iunion {$
> > 
> > ERROR: code indent should never use tabs
> > #33: FILE: linux-headers/linux/kvm.h:886:
> > +^I^I__u32 pad;$
> > 
> > ERROR: code indent should never use tabs
> > #34: FILE: linux-headers/linux/kvm.h:887:
> > +^I^I__u32 devid;$
> > 
> > ERROR: code indent should never use tabs
> > #35: FILE: linux-headers/linux/kvm.h:888:
> > +^I};$
> > 
> > ERROR: code indent should never use tabs
> > #43: FILE: linux-headers/linux/kvm.h:1034:
> > +#define KVM_MSI_VALID_DEVID^I(1U << 0)$
> > 
> > ERROR: code indent should never use tabs
> > #50: FILE: linux-headers/linux/kvm.h:1040:
> > +^I__u32 devid;$
> > 
> > ERROR: code indent should never use tabs
> > #51: FILE: linux-headers/linux/kvm.h:1041:
> > +^I__u8  pad[12];$
> > 
> > ERROR: code indent should never use tabs
> > #59: FILE: linux-headers/linux/kvm.h:1086:
> > +^IKVM_DEV_TYPE_ARM_VGIC_ITS,$
> > 
> > ERROR: code indent should never use tabs
> > #60: FILE: linux-headers/linux/kvm.h:1087:
> > +#define KVM_DEV_TYPE_ARM_VGIC_ITS^IKVM_DEV_TYPE_ARM_VGIC_ITS$
> > 
> > total: 9 errors, 0 warnings, 51 lines checked
> > 
> > Your patch has style problems, please review.  If any of these errors
> > are false positives report them to the maintainer, see
> > CHECKPATCH in MAINTAINERS.
> 
> These indentation errors are false positives.
> ---8<---
> Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of
> changing scripts/update-linux-headers.sh to expand tabs when importing.
> 
> Signed-off-by: Radim Krčmář 
> ---
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 929708721299..38232d4b25c3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1355,7 +1355,7 @@ sub process {
>   next if ($realfile !~ /\.(h|c|cpp|pl)$/);
>  
>  # in QEMU, no tabs are allowed
> - if ($rawline =~ /^\+.*\t/) {
> + if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {
>   my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>   ERROR("code indent should never use tabs\n" . $herevet);
>   $rpt_cleaners = 1;
> 

Could you do the same for standard-headers/ too?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] checkpatch: fix break by renaming README

[Peter Xu]

On Thu, Jul 21, 2016 at 08:37:01AM -0600, Eric Blake wrote:
> On 07/21/2016 04:15 AM, Peter Xu wrote:
> > Without this, we cannot run checkpatch.pl under QEMU root directory.
> > 
> > Signed-off-by: Peter Xu 
> > ---
> >  scripts/checkpatch.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> NACK; see
> https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg04850.html for
> a more complete solution

Yeah it's already reverted and it's working in master. Thanks for the
pointer. :)

-- peterx

Re: [Qemu-devel] [PATCH] checkpatch: fix break by renaming README

[Eric Blake]

On 07/21/2016 04:15 AM, Peter Xu wrote:
> Without this, we cannot run checkpatch.pl under QEMU root directory.
> 
> Signed-off-by: Peter Xu 
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

NACK; see
https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg04850.html for
a more complete solution

> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index afa7f79..8247305 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -286,7 +286,7 @@ sub top_of_kernel_tree {
>  
>   my @tree_check = (
>   "COPYING", "MAINTAINERS", "Makefile",
> - "README", "docs", "VERSION",
> + "README.md", "docs", "VERSION",
>   "vl.c"
>   );
>  
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] checkpatch: rename README to README.md

[Eric Blake]

On 07/20/2016 07:33 PM, Changlong Xie wrote:
> Since commit e5dfc5e broke the logic of @top_of_kernel_tree
> 
> Cc: Pranith Kumar 
> Cc: Paolo Bonzini 
> Cc: Stefan Hajnoczi 
> Signed-off-by: Changlong Xie 
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

NACK; see
https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg04850.html for
a more complete solution.

> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index afa7f79..8247305 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -286,7 +286,7 @@ sub top_of_kernel_tree {
>  
>   my @tree_check = (
>   "COPYING", "MAINTAINERS", "Makefile",
> - "README", "docs", "VERSION",
> + "README.md", "docs", "VERSION",
>   "vl.c"
>   );
>  
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] checkpatch: fix break by renaming README

[Peter Xu]

Without this, we cannot run checkpatch.pl under QEMU root directory.

Signed-off-by: Peter Xu 
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index afa7f79..8247305 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -286,7 +286,7 @@ sub top_of_kernel_tree {
 
my @tree_check = (
"COPYING", "MAINTAINERS", "Makefile",
-   "README", "docs", "VERSION",
+   "README.md", "docs", "VERSION",
"vl.c"
);
 
-- 
2.4.11