[PATCH] checkpatch: check for missing \n at the end of logging message

[PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Strings logged with pr_xxx and dev_xxx often lack a trailing '\n'.
Introduce new tests to try to catch them early.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This is more a PoC for now.

Regex could be improved, merged, ...
We could also check for surrounding pr_cont...

This patch is based on idea from [1]. coccinelle spots too many places
where \n are missing (~ 2800 with the heuristic I've used).
Fixing them would be painful.
I instead propose to teach checkpatch.pl about it to try to spot cases
early and avoid introducing new cases.

[1]: https://marc.info/?l=kernel-janitors&m=158619533629657&w=4
---
 scripts/checkpatch.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c392ab8ea12e..792804bd6ad9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5676,6 +5676,16 @@ sub process {
 			}
 		}
 
+# check for missing \n at the end of logging function
+		if ($line =~ /\bpr_(emerg|alert|crit|err|warning|warn|notice|info|debug|dbg)\s*\("([^"]*(?<!\\n))"/) {
+			WARN("MISSING NL",
+			     "Possible missing '\\n' at the end of a log message\n" . $hereprev);
+		}
+		if ($line =~ /\bdev_(emerg|alert|crit|err|warning|warn|notice|info|debug|dbg)\s*\([^,]*,\s*"([^"]*(?<!\\n))"/) {
+			WARN("MISSING NL",
+			     "Possible missing '\\n' at the end of a log message\n" . $hereprev);
+		}
+
 # check for logging functions with KERN_<LEVEL>
 		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
 		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {
-- 
2.20.1

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Tue, 2020-04-07 at 22:49 +0200, Christophe JAILLET wrote:
> Strings logged with pr_xxx and dev_xxx often lack a trailing '\n'.
> Introduce new tests to try to catch them early.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This is more a PoC for now.
> 
> Regex could be improved, merged, ...
> We could also check for surrounding pr_cont...
> 
> This patch is based on idea from [1]. coccinelle spots too many places
> where \n are missing (~ 2800 with the heuristic I've used).
> Fixing them would be painful.
> I instead propose to teach checkpatch.pl about it to try to spot cases
> early and avoid introducing new cases.
> 
> [1]: https://marc.info/?l=kernel-janitors&m=158619533629657&w=4
> ---
>  scripts/checkpatch.pl | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c392ab8ea12e..792804bd6ad9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5676,6 +5676,16 @@ sub process {
>  			}
>  		}
>  
> +# check for missing \n at the end of logging function
> +		if ($line =~ /\bpr_(emerg|alert|crit|err|warning|warn|notice|info|debug|dbg)\s*\("([^"]*(?<!\\n))"/) {
> +			WARN("MISSING NL",
> +			     "Possible missing '\\n' at the end of a log message\n" . $hereprev);
> +		}
> +		if ($line =~ /\bdev_(emerg|alert|crit|err|warning|warn|notice|info|debug|dbg)\s*\([^,]*,\s*"([^"]*(?<!\\n))"/) {
> +			WARN("MISSING NL",
> +			     "Possible missing '\\n' at the end of a log message\n" . $hereprev);
> +		}

This can't work as string is masked to "XXX"

This is probably better using $stat and checking if a "XX" format
string exists as 1st or 2nd arg and adding an extraction
from the $rawline equivalent and checking that.

Also this test should probably using $logFunctions and check
if the initial block is one of the known functions that
use a newline termination (pr_|dev_|netdev_|wiphy_)

Something like:
---
 scripts/checkpatch.pl | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d64c67..79eee2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5673,6 +5673,27 @@ sub process {
 			}
 		}
 
+# check for possible missing newlines at the end of common logging functions
+		if (defined($stat) &&
+		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
+		    $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
+			my $cnt = statement_rawlines($stat);
+			my $extracted_string = "";
+			for (my $i = 0; $i < $cnt; $i++) {
+				$extracted_string = get_quoted_string($lines[$linenr + $i - 1],
+								      $rawlines[$linenr + $i - 1]);
+				last if ($extracted_string ne "");
+			}
+			if ($extracted_string ne "" && $extracted_string !~ /\\n"$/) {
+				my $herectx = $here . "\n";
+				for (my $n = 0; $n < $cnt; $n++) {
+					$herectx .=  raw_line($linenr, $n) . "\n";
+				}
+				WARN("MISSING_FORMAT_NEWLINE",
+				     "Possible missing '\\n' at the end of a logging message format string\n" . $herectx);
+			}
+		}
+
 # check for logging functions with KERN_<LEVEL>
 		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
 		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Marion & Christophe JAILLET]

Le 08/04/2020 à 02:33, Joe Perches a écrit :
> On Tue, 2020-04-07 at 22:49 +0200, Christophe JAILLET wrote:
>> Strings logged with pr_xxx and dev_xxx often lack a trailing '\n'.
>> Introduce new tests to try to catch them early.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> This is more a PoC for now.
>>
>> Regex could be improved, merged, ...
>> We could also check for surrounding pr_cont...
>>
>> This patch is based on idea from [1]. coccinelle spots too many places
>> where \n are missing (~ 2800 with the heuristic I've used).
>> Fixing them would be painful.
>> I instead propose to teach checkpatch.pl about it to try to spot cases
>> early and avoid introducing new cases.
>>
>> [1]: https://marc.info/?l=kernel-janitors&m=158619533629657&w=4
>> ---
>>   scripts/checkpatch.pl | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index c392ab8ea12e..792804bd6ad9 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -5676,6 +5676,16 @@ sub process {
>>   			}
>>   		}
>>   
>> +# check for missing \n at the end of logging function
>> +		if ($line =~ /\bpr_(emerg|alert|crit|err|warning|warn|notice|info|debug|dbg)\s*\("([^"]*(?<!\\n))"/) {
>> +			WARN("MISSING NL",
>> +			     "Possible missing '\\n' at the end of a log message\n" . $hereprev);
>> +		}
>> +		if ($line =~ /\bdev_(emerg|alert|crit|err|warning|warn|notice|info|debug|dbg)\s*\([^,]*,\s*"([^"]*(?<!\\n))"/) {
>> +			WARN("MISSING NL",
>> +			     "Possible missing '\\n' at the end of a log message\n" . $hereprev);
>> +		}
> This can't work as string is masked to "XXX"

Ok. I wasn't aware of that.

I tested the regex with regex101.org and only tested with patches that 
trigger the checkpatch.pl test, and it worked fine for me.
I didn't test with string with trailing \n, that should NOT trigger the 
test. I should have! :(

> This is probably better using $stat and checking if a "XX" format
> string exists as 1st or 2nd arg and adding an extraction
> from the $rawline equivalent and checking that.
>
> Also this test should probably using $logFunctions and check
> if the initial block is one of the known functions that
> use a newline termination (pr_|dev_|netdev_|wiphy_)

Agreed but your perl and regex is much more fluent than mine. ;-)

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

This works rather better:

Perhaps you could test?
---

v2:

o Avoid pr_cont
o Use only last format line if split across multiple lines

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d64c67..f00a6c8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5673,6 +5673,28 @@ sub process {
 			}
 		}
 
+# check for possible missing newlines at the end of common logging functions
+		if (defined($stat) &&
+		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
+		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
+			my $cnt = statement_rawlines($stat);
+			my $extracted_string = "";
+			for (my $i = 0; $i < $cnt; $i++) {
+				next if ($lines[$linenr + $i - 1] !~ /$String\s*[,\)]/);
+				$extracted_string = get_quoted_string($lines[$linenr + $i - 1],
+								      $rawlines[$linenr + $i - 1]);
+				last if ($extracted_string ne "");
+			}
+			if ($extracted_string ne "" && $extracted_string !~ /\\n"$/) {
+				my $herectx = $here . "\n";
+				for (my $n = 0; $n < $cnt; $n++) {
+					$herectx .=  raw_line($linenr, $n) . "\n";
+				}
+				WARN("MISSING_FORMAT_NEWLINE",
+				     "Possible missing '\\n' at the end of a logging message format string\n" . $herectx);
+			}
+		}
+
 # check for logging functions with KERN_<LEVEL>
 		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
 		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Marion & Christophe JAILLET]

Le 08/04/2020 à 04:14, Joe Perches a écrit :
> This works rather better:
>
> Perhaps you could test?
> ---
>
> v2:
>
> o Avoid pr_cont
> o Use only last format line if split across multiple lines
>
>   scripts/checkpatch.pl | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d64c67..f00a6c8 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5673,6 +5673,28 @@ sub process {
>   			}
>   		}
>   
> +# check for possible missing newlines at the end of common logging functions
> +		if (defined($stat) &&
> +		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> +		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> +			my $cnt = statement_rawlines($stat);
> +			my $extracted_string = "";
> +			for (my $i = 0; $i < $cnt; $i++) {
> +				next if ($lines[$linenr + $i - 1] !~ /$String\s*[,\)]/);
> +				$extracted_string = get_quoted_string($lines[$linenr + $i - 1],
> +								      $rawlines[$linenr + $i - 1]);
> +				last if ($extracted_string ne "");
> +			}
> +			if ($extracted_string ne "" && $extracted_string !~ /\\n"$/) {
> +				my $herectx = $here . "\n";
> +				for (my $n = 0; $n < $cnt; $n++) {
> +					$herectx .=  raw_line($linenr, $n) . "\n";
> +				}
> +				WARN("MISSING_FORMAT_NEWLINE",
> +				     "Possible missing '\\n' at the end of a logging message format string\n" . $herectx);
> +			}
> +		}
> +
>   # check for logging functions with KERN_<LEVEL>
>   		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
>   		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {
>
For what I wanted to check and according to the few tests I've made, it 
looks fine.

Thank you very much for sharing this much more robust (and working) 
alternative.

For what it worth: (i.e. much more tests should be done)
Tested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Maybe, at least a Suggested-By: would be appreciated.

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Wed, 2020-04-08 at 22:23 +0200, Marion & Christophe JAILLET wrote:
> Le 08/04/2020 à 04:14, Joe Perches a écrit :
> > This works rather better:
> > 
> > Perhaps you could test?
> > ---
> > 
> > v2:
> > 
> > o Avoid pr_cont
> > o Use only last format line if split across multiple lines
> > 
> >   scripts/checkpatch.pl | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d64c67..f00a6c8 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5673,6 +5673,28 @@ sub process {
> >   			}
> >   		}
> >   
> > +# check for possible missing newlines at the end of common logging functions
> > +		if (defined($stat) &&
> > +		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> > +		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> > +			my $cnt = statement_rawlines($stat);
> > +			my $extracted_string = "";
> > +			for (my $i = 0; $i < $cnt; $i++) {
> > +				next if ($lines[$linenr + $i - 1] !~ /$String\s*[,\)]/);
> > +				$extracted_string = get_quoted_string($lines[$linenr + $i - 1],
> > +								      $rawlines[$linenr + $i - 1]);
> > +				last if ($extracted_string ne "");
> > +			}
> > +			if ($extracted_string ne "" && $extracted_string !~ /\\n"$/) {
> > +				my $herectx = $here . "\n";
> > +				for (my $n = 0; $n < $cnt; $n++) {
> > +					$herectx .=  raw_line($linenr, $n) . "\n";
> > +				}
> > +				WARN("MISSING_FORMAT_NEWLINE",
> > +				     "Possible missing '\\n' at the end of a logging message format string\n" . $herectx);
> > +			}
> > +		}
> > +
> >   # check for logging functions with KERN_<LEVEL>
> >   		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
> >   		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {
> > 
> For what I wanted to check and according to the few tests I've made, it 
> looks fine.
> 
> Thank you very much for sharing this much more robust (and working) 
> alternative.
> 
> For what it worth: (i.e. much more tests should be done)
> Tested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Then I think you really haven't tested it very thoroughly.

For instance:

$ git ls-files -- 'drivers/*.[ch]' | \
  xargs ./scripts/checkpatch.pl -f --quiet --no-summary --types=MISSING_FORMAT_NEWLINE

emits many false positives.

Some types of false positives:

o Many of the formats seem to end in a ':' or a ' '
  maybe those should be excluded
   #86: FILE: drivers/android/binder_alloc_selftest.c:86:
   +	pr_err("free seq: ");

   o Split string formats should be excluded better
     as only the first string fragment is checked:
   #1001: FILE: drivers/ata/pata_octeon_cf.c:1001:
   +	dev_info(&pdev->dev, "version " DRV_VERSION" %d bit%s.\n",
   +		 is_16bit ? 16 : 8,
   +		 cf_port->is_true_ide ? ", True IDE" : "");

   probably a few others, including a desire to check
   if a pr_cont is below the use within a few lines.

   > Maybe, at least a Suggested-By: would be appreciated.

No worries, when it's cooked, it'll have that.

cheers, Joe

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 09/04/2020 à 05:10, Joe Perches a écrit :
> On Wed, 2020-04-08 at 22:23 +0200, Marion & Christophe JAILLET wrote:
>> Le 08/04/2020 à 04:14, Joe Perches a écrit :
>>> This works rather better:
>>>
>>> Perhaps you could test?
>>> ---
>>>
>>> v2:
>>>
>>> o Avoid pr_cont
>>> o Use only last format line if split across multiple lines
>>>
>>>    scripts/checkpatch.pl | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index d64c67..f00a6c8 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -5673,6 +5673,28 @@ sub process {
>>>    			}
>>>    		}
>>>    
>>> +# check for possible missing newlines at the end of common logging functions
>>> +		if (defined($stat) &&
>>> +		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
>>> +		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
>>> +			my $cnt = statement_rawlines($stat);
>>> +			my $extracted_string = "";
>>> +			for (my $i = 0; $i < $cnt; $i++) {
>>> +				next if ($lines[$linenr + $i - 1] !~ /$String\s*[,\)]/);
>>> +				$extracted_string = get_quoted_string($lines[$linenr + $i - 1],
>>> +								      $rawlines[$linenr + $i - 1]);
>>> +				last if ($extracted_string ne "");
>>> +			}
>>> +			if ($extracted_string ne "" && $extracted_string !~ /\\n"$/) {
>>> +				my $herectx = $here . "\n";
>>> +				for (my $n = 0; $n < $cnt; $n++) {
>>> +					$herectx .=  raw_line($linenr, $n) . "\n";
>>> +				}
>>> +				WARN("MISSING_FORMAT_NEWLINE",
>>> +				     "Possible missing '\\n' at the end of a logging message format string\n" . $herectx);
>>> +			}
>>> +		}
>>> +
>>>    # check for logging functions with KERN_<LEVEL>
>>>    		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
>>>    		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {
>>>
>> For what I wanted to check and according to the few tests I've made, it
>> looks fine.
>>
>> Thank you very much for sharing this much more robust (and working)
>> alternative.
>>
>> For what it worth: (i.e. much more tests should be done)
>> Tested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Then I think you really haven't tested it very thoroughly.
>
> For instance:
>
> $ git ls-files -- 'drivers/*.[ch]' | \
>    xargs ./scripts/checkpatch.pl -f --quiet --no-summary --types=MISSING_FORMAT_NEWLINE
>
> emits many false positives.
>
> Some types of false positives:
>
> o Many of the formats seem to end in a ':' or a ' '
>    maybe those should be excluded
>     #86: FILE: drivers/android/binder_alloc_selftest.c:86:
>     +	pr_err("free seq: ");
>
>     o Split string formats should be excluded better
>       as only the first string fragment is checked:
>     #1001: FILE: drivers/ata/pata_octeon_cf.c:1001:
>     +	dev_info(&pdev->dev, "version " DRV_VERSION" %d bit%s.\n",
>     +		 is_16bit ? 16 : 8,
>     +		 cf_port->is_true_ide ? ", True IDE" : "");
>
>     probably a few others, including a desire to check
>     if a pr_cont is below the use within a few lines.
>
>     > Maybe, at least a Suggested-By: would be appreciated.
>
> No worries, when it's cooked, it'll have that.
>
> cheers, Joe
>
>
I think that, at least printk(), WARN() and co, and panic() should also 
be handled the same way.


A few files (5 according to my grep) also have something like:
    #define pr_fmt(fmt) "bcache: %s()" fmt "\n", __func__
and then sometimes a mix of pr_xxx() with either trailing \n or not.

Maybe those should be handled manually to be consistent and avoid a "\n" 
in pr_fmt which is not widely used in other files

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 09/04/2020 à 05:10, Joe Perches a écrit :
> On Wed, 2020-04-08 at 22:23 +0200, Marion & Christophe JAILLET wrote:
>> Le 08/04/2020 à 04:14, Joe Perches a écrit :
>>> This works rather better:
>>>
>>> Perhaps you could test?
>>> ---
>>>
>>> v2:
>>>
>>> o Avoid pr_cont
>>> o Use only last format line if split across multiple lines
>>>
>>>    scripts/checkpatch.pl | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index d64c67..f00a6c8 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -5673,6 +5673,28 @@ sub process {
>>>    			}
>>>    		}
>>>    
>>> +# check for possible missing newlines at the end of common logging functions
>>> +		if (defined($stat) &&
>>> +		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
>>> +		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
>>> +			my $cnt = statement_rawlines($stat);
>>> +			my $extracted_string = "";
>>> +			for (my $i = 0; $i < $cnt; $i++) {
>>> +				next if ($lines[$linenr + $i - 1] !~ /$String\s*[,\)]/);
>>> +				$extracted_string = get_quoted_string($lines[$linenr + $i - 1],
>>> +								      $rawlines[$linenr + $i - 1]);
>>> +				last if ($extracted_string ne "");
>>> +			}
>>> +			if ($extracted_string ne "" && $extracted_string !~ /\\n"$/) {
>>> +				my $herectx = $here . "\n";
>>> +				for (my $n = 0; $n < $cnt; $n++) {
>>> +					$herectx .=  raw_line($linenr, $n) . "\n";
>>> +				}
>>> +				WARN("MISSING_FORMAT_NEWLINE",
>>> +				     "Possible missing '\\n' at the end of a logging message format string\n" . $herectx);
>>> +			}
>>> +		}
>>> +
>>>    # check for logging functions with KERN_<LEVEL>
>>>    		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
>>>    		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {
>>>
>> For what I wanted to check and according to the few tests I've made, it
>> looks fine.
>>
>> Thank you very much for sharing this much more robust (and working)
>> alternative.
>>
>> For what it worth: (i.e. much more tests should be done)
>> Tested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Then I think you really haven't tested it very thoroughly.
>
> For instance:
>
> $ git ls-files -- 'drivers/*.[ch]' | \
>    xargs ./scripts/checkpatch.pl -f --quiet --no-summary --types=MISSING_FORMAT_NEWLINE
>
> emits many false positives.
>
> Some types of false positives:
>
> o Many of the formats seem to end in a ':' or a ' '
>    maybe those should be excluded
>     #86: FILE: drivers/android/binder_alloc_selftest.c:86:
>     +	pr_err("free seq: ");
>
>     o Split string formats should be excluded better
>       as only the first string fragment is checked:
>     #1001: FILE: drivers/ata/pata_octeon_cf.c:1001:
>     +	dev_info(&pdev->dev, "version " DRV_VERSION" %d bit%s.\n",
>     +		 is_16bit ? 16 : 8,
>     +		 cf_port->is_true_ide ? ", True IDE" : "");
>
>     probably a few others, including a desire to check
>     if a pr_cont is below the use within a few lines.
>
>     > Maybe, at least a Suggested-By: would be appreciated.
>
> No worries, when it's cooked, it'll have that.
>
> cheers, Joe
>
>
I think that, at least printk(), WARN() and co, and panic() should also 
be handled the same way.


A few files (5 according to my grep) also have something like:
    #define pr_fmt(fmt) "bcache: %s()" fmt "\n", __func__
and then sometimes a mix of pr_xxx() with either trailing \n or not.

Maybe those should be handled manually to be consistent and avoid a "\n" 
in pr_fmt which is not widely used in other files

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Thu, 2020-04-09 at 09:24 +0200, Christophe JAILLET wrote:
> I think that, at least printk(), WARN() and co, and panic() should also 
> be handled the same way.

Maybe.

> A few files (5 according to my grep) also have something like:
>     #define pr_fmt(fmt) "bcache: %s()" fmt "\n", __func__
> and then sometimes a mix of pr_xxx() with either trailing \n or not.

Didn't know about those.

> Maybe those should be handled manually to be consistent and avoid a "\n" 
> in pr_fmt which is not widely used in other files

More likely the pr_fmt should have the \n removed and added
to the uses.

$ git grep -P 'define\s+pr_fmt.*\\n'
drivers/clocksource/timer-davinci.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
drivers/md/bcache/bcache.h:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
drivers/md/bcache/bset.c:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
lib/math/prime_numbers.c:#define pr_fmt(fmt) "prime numbers: " fmt "\n"
lib/percpu-refcount.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
lib/test_hash.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt "\n"
tools/usb/usbip/libsrc/usbip_common.h:#define pr_fmt(fmt)       "%s: %s: " fmt "\n", PROGNAME

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 09/04/2020 à 17:29, Joe Perches a écrit :
> On Thu, 2020-04-09 at 09:24 +0200, Christophe JAILLET wrote:
>> I think that, at least printk(), WARN() and co, and panic() should also
>> be handled the same way.
> Maybe.
>
>> A few files (5 according to my grep) also have something like:
>>      #define pr_fmt(fmt) "bcache: %s()" fmt "\n", __func__
>> and then sometimes a mix of pr_xxx() with either trailing \n or not.
> Didn't know about those.
>
>> Maybe those should be handled manually to be consistent and avoid a "\n"
>> in pr_fmt which is not widely used in other files
> More likely the pr_fmt should have the \n removed and added
> to the uses.
Yes agreed.
> $ git grep -P 'define\s+pr_fmt.*\\n'
> drivers/clocksource/timer-davinci.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
https://marc.info/?l=linux-kernel&m=158642435216181&w=4 (which has been 
acked)

> lib/math/prime_numbers.c:#define pr_fmt(fmt) "prime numbers: " fmt "\n"
> lib/test_hash.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt "\n"
Patches proposed.
> lib/percpu-refcount.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
In this file, there are some WARN_ON.
Are these log functions also influenced by pr_fmt?
> drivers/md/bcache/bcache.h:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
> drivers/md/bcache/bset.c:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
> tools/usb/usbip/libsrc/usbip_common.h:#define pr_fmt(fmt)       "%s: %s: " fmt "\n", PROGNAME

Tricky because all files that include it have to be checked.
I won't touch these ones.

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 09/04/2020 à 17:29, Joe Perches a écrit :
> On Thu, 2020-04-09 at 09:24 +0200, Christophe JAILLET wrote:
>> I think that, at least printk(), WARN() and co, and panic() should also
>> be handled the same way.
> Maybe.
>
>> A few files (5 according to my grep) also have something like:
>>      #define pr_fmt(fmt) "bcache: %s()" fmt "\n", __func__
>> and then sometimes a mix of pr_xxx() with either trailing \n or not.
> Didn't know about those.
>
>> Maybe those should be handled manually to be consistent and avoid a "\n"
>> in pr_fmt which is not widely used in other files
> More likely the pr_fmt should have the \n removed and added
> to the uses.
Yes agreed.
> $ git grep -P 'define\s+pr_fmt.*\\n'
> drivers/clocksource/timer-davinci.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
https://marc.info/?l=linux-kernel&m=158642435216181&w=4 (which has been 
acked)

> lib/math/prime_numbers.c:#define pr_fmt(fmt) "prime numbers: " fmt "\n"
> lib/test_hash.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt "\n"
Patches proposed.
> lib/percpu-refcount.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
In this file, there are some WARN_ON.
Are these log functions also influenced by pr_fmt?
> drivers/md/bcache/bcache.h:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
> drivers/md/bcache/bset.c:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
> tools/usb/usbip/libsrc/usbip_common.h:#define pr_fmt(fmt)       "%s: %s: " fmt "\n", PROGNAME

Tricky because all files that include it have to be checked.
I won't touch these ones.

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Thu, 2020-04-09 at 19:34 +0200, Christophe JAILLET wrote:
> Le 09/04/2020 à 17:29, Joe Perches a écrit :
[]
> > lib/percpu-refcount.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
> In this file, there are some WARN_ON.
> Are these log functions also influenced by pr_fmt?

No.

> > drivers/md/bcache/bcache.h:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
> > drivers/md/bcache/bset.c:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
> > tools/usb/usbip/libsrc/usbip_common.h:#define pr_fmt(fmt)       "%s: %s: " fmt "\n", PROGNAME
> 
> Tricky because all files that include it have to be checked.
> I won't touch these ones.

What a pity I do not know the French equivalent
for the children's taunt of "chicken!"...

cheers, Joe

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 09/04/2020 à 19:50, Joe Perches a écrit :
> On Thu, 2020-04-09 at 19:34 +0200, Christophe JAILLET wrote:
>> Le 09/04/2020 à 17:29, Joe Perches a écrit :
> []
>>> lib/percpu-refcount.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
>> In this file, there are some WARN_ON.
>> Are these log functions also influenced by pr_fmt?
> No.
Ok, thx.
>
>>> drivers/md/bcache/bcache.h:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
>>> drivers/md/bcache/bset.c:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
>>> tools/usb/usbip/libsrc/usbip_common.h:#define pr_fmt(fmt)       "%s: %s: " fmt "\n", PROGNAME
>> Tricky because all files that include it have to be checked.
>> I won't touch these ones.
> What a pity I do not know the French equivalent
> for the children's taunt of "chicken!"...

I don't know why the french one is wet, but the translation would be 
"poule mouillée".

In fact, I don't really see the need to modify many files just for some 
kind of style.
(same reason why I think that checkpatch is a better place for a test 
than submitting hundreds of patches based on coccinelle)


 From your point of view, does auditing and fixing these missing \n make 
sense?
Wouldn't it just be a lot of noise for a small benefit?


CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 09/04/2020 à 19:50, Joe Perches a écrit :
> On Thu, 2020-04-09 at 19:34 +0200, Christophe JAILLET wrote:
>> Le 09/04/2020 à 17:29, Joe Perches a écrit :
> []
>>> lib/percpu-refcount.c:#define pr_fmt(fmt) "%s: " fmt "\n", __func__
>> In this file, there are some WARN_ON.
>> Are these log functions also influenced by pr_fmt?
> No.
Ok, thx.
>
>>> drivers/md/bcache/bcache.h:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
>>> drivers/md/bcache/bset.c:#define pr_fmt(fmt) "bcache: %s() " fmt "\n", __func__
>>> tools/usb/usbip/libsrc/usbip_common.h:#define pr_fmt(fmt)       "%s: %s: " fmt "\n", PROGNAME
>> Tricky because all files that include it have to be checked.
>> I won't touch these ones.
> What a pity I do not know the French equivalent
> for the children's taunt of "chicken!"...

I don't know why the french one is wet, but the translation would be 
"poule mouillée".

In fact, I don't really see the need to modify many files just for some 
kind of style.
(same reason why I think that checkpatch is a better place for a test 
than submitting hundreds of patches based on coccinelle)


 From your point of view, does auditing and fixing these missing \n make 
sense?
Wouldn't it just be a lot of noise for a small benefit?


CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Thu, 2020-04-09 at 20:52 +0200, Christophe JAILLET wrote:
> In fact, I don't really see the need to modify many files just for some 
> kind of style.
> (same reason why I think that checkpatch is a better place for a test 
> than submitting hundreds of patches based on coccinelle)
> 
> From your point of view, does auditing and fixing these missing \n make 
> sense?

Not all that much no.  Even the existing conversions
of formats missing newlines isn't all that important.

It's only a consideration for relatively unmaintained
old drivers and arches that still use printk without a
KERN_<LEVEL> where a message might either be interleaved
with a pr_<level> style message without a terminating
newline or by old style messages that should actually
instead be coalesced because the printks don't have
any KERN_<LEVEL>.

> Wouldn't it just be a lot of noise for a small benefit?

Much of the noise has already been filtered out by patches
and the ambient noise is already at a relatively low
level.

Quiet is good though and I think the noise reduction
is useful and quite painless.

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Dan Carpenter]

On Wed, Apr 08, 2020 at 08:10:23PM -0700, Joe Perches wrote:
> Some types of false positives:
> 
> o Many of the formats seem to end in a ':' or a ' '
>   maybe those should be excluded
>    #86: FILE: drivers/android/binder_alloc_selftest.c:86:
>    +	pr_err("free seq: ");
> 

Yeah.  I also ignored strings that end in a space in my version.

>    o Split string formats should be excluded better
>      as only the first string fragment is checked:
>    #1001: FILE: drivers/ata/pata_octeon_cf.c:1001:
>    +	dev_info(&pdev->dev, "version " DRV_VERSION" %d bit%s.\n",

I ignored anything with a newline in it anywhere.

>    +		 is_16bit ? 16 : 8,
>    +		 cf_port->is_true_ide ? ", True IDE" : "");
> 
>    probably a few others, including a desire to check
>    if a pr_cont is below the use within a few lines.

Seems tricky to implement...  I think if you can't make it work it's
still usefull even with a some false positives.

regards,
dan carpenter

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 08/04/2020 à 04:14, Joe Perches a écrit :
> This works rather better:
>
> Perhaps you could test?
> ---
>
> v2:
>
> o Avoid pr_cont
> o Use only last format line if split across multiple lines
>
>   scripts/checkpatch.pl | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d64c67..f00a6c8 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5673,6 +5673,28 @@ sub process {
>   			}
>   		}
>   
> +# check for possible missing newlines at the end of common logging functions
> +		if (defined($stat) &&
> +		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> +		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> +			my $cnt = statement_rawlines($stat);
> +			my $extracted_string = "";
> +			for (my $i = 0; $i < $cnt; $i++) {
> +				next if ($lines[$linenr + $i - 1] !~ /$String\s*[,\)]/);
> +				$extracted_string = get_quoted_string($lines[$linenr + $i - 1],
> +								      $rawlines[$linenr + $i - 1]);
> +				last if ($extracted_string ne "");
> +			}
> +			if ($extracted_string ne "" && $extracted_string !~ /\\n"$/) {
> +				my $herectx = $here . "\n";
> +				for (my $n = 0; $n < $cnt; $n++) {
> +					$herectx .=  raw_line($linenr, $n) . "\n";
> +				}
> +				WARN("MISSING_FORMAT_NEWLINE",
> +				     "Possible missing '\\n' at the end of a logging message format string\n" . $herectx);
> +			}
> +		}
> +
>   # check for logging functions with KERN_<LEVEL>
>   		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
>   		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {


Hi,

I'm looking at some modification done in the last month that could have 
been spotted by the above script.

     ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c

correctly spots the 3 first cases, but the 3 last (line 202, 210 and 
217) are missed.
I don't understand why.

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Fri, 2020-04-10 at 19:35 +0200, Christophe JAILLET wrote:
> Le 08/04/2020 à 04:14, Joe Perches a écrit :
> > This works rather better:
> > Perhaps you could test?
[]
> I'm looking at some modification done in the last month that could have 
> been spotted by the above script.
> 
>      ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c
> 
> correctly spots the 3 first cases, but the 3 last (line 202, 210 and 
> 217) are missed.
> I don't understand why.

It has to do with checkpatch's single statement parsing.

This case:

	if (foo)
		dev_warn(...);

is parsed as a single statement but

	if (foo) {
		dev_warn(...);
	};

is parsed as multiple statements so for the
second case

		dev_warn(...);

is analyzed as a separate statement.

The regex match for this missing newline test expects
that each printk is a separate statement so the first
case doesn't match.

Clearly the regex can be improved here.

cheers, Joe

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Fri, 2020-04-10 at 12:46 -0700, Joe Perches wrote:
> On Fri, 2020-04-10 at 19:35 +0200, Christophe JAILLET wrote:
> > Le 08/04/2020 à 04:14, Joe Perches a écrit :
> > > This works rather better:
> > > Perhaps you could test?
> []
> > I'm looking at some modification done in the last month that could have 
> > been spotted by the above script.
> > 
> >      ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c
> > 
> > correctly spots the 3 first cases, but the 3 last (line 202, 210 and 
> > 217) are missed.
> > I don't understand why.
> 
> It has to do with checkpatch's single statement parsing.
> 
> This case:
> 
> 	if (foo)
> 		dev_warn(...);
> 
> is parsed as a single statement but
> 
> 	if (foo) {
> 		dev_warn(...);
> 	};
> 
> is parsed as multiple statements so for the
> second case
> 
> 		dev_warn(...);
> 
> is analyzed as a separate statement.
> 
> The regex match for this missing newline test expects
> that each printk is a separate statement so the first
> case doesn't match.
> 
> Clearly the regex can be improved here.

So on top of the original patch:
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f00a6c8..54eaa7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5675,8 +5675,8 @@ sub process {
 
 # check for possible missing newlines at the end of common logging functions
 		if (defined($stat) &&
-		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
-		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
+		    $stat =~ /^\+\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
+		    $2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
 			my $cnt = statement_rawlines($stat);
 			my $extracted_string = "";
 			for (my $i = 0; $i < $cnt; $i++) {

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 10/04/2020 à 21:53, Joe Perches a écrit :
> On Fri, 2020-04-10 at 12:46 -0700, Joe Perches wrote:
>> On Fri, 2020-04-10 at 19:35 +0200, Christophe JAILLET wrote:
>>> Le 08/04/2020 à 04:14, Joe Perches a écrit :
>>>> This works rather better:
>>>> Perhaps you could test?
>> []
>>> I'm looking at some modification done in the last month that could have
>>> been spotted by the above script.
>>>
>>>       ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c
>>>
>>> correctly spots the 3 first cases, but the 3 last (line 202, 210 and
>>> 217) are missed.
>>> I don't understand why.
>> It has to do with checkpatch's single statement parsing.
>>
>> This case:
>>
>> 	if (foo)
>> 		dev_warn(...);
>>
>> is parsed as a single statement but
>>
>> 	if (foo) {
>> 		dev_warn(...);
>> 	};
>>
>> is parsed as multiple statements so for the
>> second case
>>
>> 		dev_warn(...);
>>
>> is analyzed as a separate statement.
>>
>> The regex match for this missing newline test expects
>> that each printk is a separate statement so the first
>> case doesn't match.
>>
>> Clearly the regex can be improved here.
> So on top of the original patch:
> ---
>   scripts/checkpatch.pl | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f00a6c8..54eaa7 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5675,8 +5675,8 @@ sub process {
>   
>   # check for possible missing newlines at the end of common logging functions
>   		if (defined($stat) &&
> -		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> -		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> +		    $stat =~ /^\+\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> +		    $2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
>   			my $cnt = statement_rawlines($stat);
>   			my $extracted_string = "";
>   			for (my $i = 0; $i < $cnt; $i++) {

Hi Joe,

This fixes the use case for  drivers/usb/phy/phy-jz4770.c

     ./scripts/checkpatch.pl -f drivers/usb/gadget/udc/tegra-xudc.c

is missing line 691.

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 10/04/2020 à 21:53, Joe Perches a écrit :
> On Fri, 2020-04-10 at 12:46 -0700, Joe Perches wrote:
>> On Fri, 2020-04-10 at 19:35 +0200, Christophe JAILLET wrote:
>>> Le 08/04/2020 à 04:14, Joe Perches a écrit :
>>>> This works rather better:
>>>> Perhaps you could test?
>> []
>>> I'm looking at some modification done in the last month that could have
>>> been spotted by the above script.
>>>
>>>       ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c
>>>
>>> correctly spots the 3 first cases, but the 3 last (line 202, 210 and
>>> 217) are missed.
>>> I don't understand why.
>> It has to do with checkpatch's single statement parsing.
>>
>> This case:
>>
>> 	if (foo)
>> 		dev_warn(...);
>>
>> is parsed as a single statement but
>>
>> 	if (foo) {
>> 		dev_warn(...);
>> 	};
>>
>> is parsed as multiple statements so for the
>> second case
>>
>> 		dev_warn(...);
>>
>> is analyzed as a separate statement.
>>
>> The regex match for this missing newline test expects
>> that each printk is a separate statement so the first
>> case doesn't match.
>>
>> Clearly the regex can be improved here.
> So on top of the original patch:
> ---
>   scripts/checkpatch.pl | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f00a6c8..54eaa7 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5675,8 +5675,8 @@ sub process {
>   
>   # check for possible missing newlines at the end of common logging functions
>   		if (defined($stat) &&
> -		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> -		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> +		    $stat =~ /^\+\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> +		    $2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
>   			my $cnt = statement_rawlines($stat);
>   			my $extracted_string = "";
>   			for (my $i = 0; $i < $cnt; $i++) {

Hi Joe,

This fixes the use case for  drivers/usb/phy/phy-jz4770.c

     ./scripts/checkpatch.pl -f drivers/usb/gadget/udc/tegra-xudc.c

is missing line 691.

CJ

Andy? checkpatch $stat question (was: Re: [PATCH] checkpatch: check for missing \n at the end of logging message)

[Joe Perches]

Andy Whitcroft:  checkpatch internals question for you below:

On Sat, 2020-04-11 at 08:48 +0200, Christophe JAILLET wrote:
> Le 10/04/2020 à 21:53, Joe Perches a écrit :
> > On Fri, 2020-04-10 at 12:46 -0700, Joe Perches wrote:
> > > On Fri, 2020-04-10 at 19:35 +0200, Christophe JAILLET wrote:
> > > > Le 08/04/2020 à 04:14, Joe Perches a écrit :
> > > > > This works rather better:
> > > > > Perhaps you could test?
> > > []
> > > > I'm looking at some modification done in the last month that could have
> > > > been spotted by the above script.
> > > > 
> > > >       ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c
> > > > 
> > > > correctly spots the 3 first cases, but the 3 last (line 202, 210 and
> > > > 217) are missed.
> > > > I don't understand why.
> > > It has to do with checkpatch's single statement parsing.
> > > 
> > > This case:
> > > 
> > > 	if (foo)
> > > 		dev_warn(...);
> > > 
> > > is parsed as a single statement but
> > > 
> > > 	if (foo) {
> > > 		dev_warn(...);
> > > 	};
> > > 
> > > is parsed as multiple statements so for the
> > > second case
> > > 
> > > 		dev_warn(...);
> > > 
> > > is analyzed as a separate statement.
> > > 
> > > The regex match for this missing newline test expects
> > > that each printk is a separate statement so the first
> > > case doesn't match.
> > > 
> > > Clearly the regex can be improved here.
> > So on top of the original patch:
> > ---
> >   scripts/checkpatch.pl | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index f00a6c8..54eaa7 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5675,8 +5675,8 @@ sub process {
> >   
> >   # check for possible missing newlines at the end of common logging functions
> >   		if (defined($stat) &&
> > -		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> > -		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> > +		    $stat =~ /^\+\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> > +		    $2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> >   			my $cnt = statement_rawlines($stat);
> >   			my $extracted_string = "";
> >   			for (my $i = 0; $i < $cnt; $i++) {
> 
> Hi Joe,
> 
> This fixes the use case for  drivers/usb/phy/phy-jz4770.c
> 
>      ./scripts/checkpatch.pl -f drivers/usb/gadget/udc/tegra-xudc.c
> 
> is missing line 691.

Turns out checkpatch also considers a close brace
followed by another statement block a single statement
so that test could be:

		    $stat =~ /^\+\s*\}?\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
		    $2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {

But that seems odd and I wonder if Andy agrees
so I have a question for Andy about the use of
ctx_statement_block and its implementation.

It seems that $suppress_statement should be updated
when the cond return value does not start with { so
that code like

struct foo *
function(
	 int arg1,
	 long arg2,
	 struct bar *baz)
{
	[implementation...]
}

allows skipping the creation of a new $stat for each
line of the function arguments.  Instead there would
be just 2 $stat blocks created, one for the function
with its args and implementation, another for just
the implementation with braces.

Perhaps this is proper, but perhaps the $line_nr_next
return value should be updated in ctx_statement_block
instead.

Also a $stat that starts with a close brace is odd
and probably not good.

Thoughts?

---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 54eaa7..8ef95b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3558,7 +3558,10 @@ sub process {
 			($stat, $cond, $line_nr_next, $remain_next, $off_next) =
 				ctx_statement_block($linenr, $realcnt, 0);
 			$stat =~ s/\n./\n /g;
-			$cond =~ s/\n./\n /g;
+			my $condcnt = $cond =~ s/\n./\n /g;
+			if ($cond !~ /^.\s*\}/) {
+				$suppress_statement = $linenr + $condcnt;
+			}
 
 #print "linenr<$linenr> <$stat>\n";
 			# If this statement has no statement boundaries within

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 10/04/2020 à 21:53, Joe Perches a écrit :
> On Fri, 2020-04-10 at 12:46 -0700, Joe Perches wrote:
>> On Fri, 2020-04-10 at 19:35 +0200, Christophe JAILLET wrote:
>>> Le 08/04/2020 à 04:14, Joe Perches a écrit :
>>>> This works rather better:
>>>> Perhaps you could test?
>> []
>>> I'm looking at some modification done in the last month that could have
>>> been spotted by the above script.
>>>
>>>       ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c
>>>
>>> correctly spots the 3 first cases, but the 3 last (line 202, 210 and
>>> 217) are missed.
>>> I don't understand why.
>> It has to do with checkpatch's single statement parsing.
>>
>> This case:
>>
>> 	if (foo)
>> 		dev_warn(...);
>>
>> is parsed as a single statement but
>>
>> 	if (foo) {
>> 		dev_warn(...);
>> 	};
>>
>> is parsed as multiple statements so for the
>> second case
>>
>> 		dev_warn(...);
>>
>> is analyzed as a separate statement.
>>
>> The regex match for this missing newline test expects
>> that each printk is a separate statement so the first
>> case doesn't match.
>>
>> Clearly the regex can be improved here.
> So on top of the original patch:
> ---
>   scripts/checkpatch.pl | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f00a6c8..54eaa7 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5675,8 +5675,8 @@ sub process {
>   
>   # check for possible missing newlines at the end of common logging functions
>   		if (defined($stat) &&
> -		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> -		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> +		    $stat =~ /^\+\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> +		    $2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
>   			my $cnt = statement_rawlines($stat);
>   			my $extracted_string = "";
>   			for (my $i = 0; $i < $cnt; $i++) {

Hi,

	./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c

is missing line 189, even if it looks like a construction correctly spotted in some other files:
	if (foo) {
		dev_err(...);
		...
	};

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 10/04/2020 à 21:53, Joe Perches a écrit :
> On Fri, 2020-04-10 at 12:46 -0700, Joe Perches wrote:
>> On Fri, 2020-04-10 at 19:35 +0200, Christophe JAILLET wrote:
>>> Le 08/04/2020 à 04:14, Joe Perches a écrit :
>>>> This works rather better:
>>>> Perhaps you could test?
>> []
>>> I'm looking at some modification done in the last month that could have
>>> been spotted by the above script.
>>>
>>>       ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c
>>>
>>> correctly spots the 3 first cases, but the 3 last (line 202, 210 and
>>> 217) are missed.
>>> I don't understand why.
>> It has to do with checkpatch's single statement parsing.
>>
>> This case:
>>
>> 	if (foo)
>> 		dev_warn(...);
>>
>> is parsed as a single statement but
>>
>> 	if (foo) {
>> 		dev_warn(...);
>> 	};
>>
>> is parsed as multiple statements so for the
>> second case
>>
>> 		dev_warn(...);
>>
>> is analyzed as a separate statement.
>>
>> The regex match for this missing newline test expects
>> that each printk is a separate statement so the first
>> case doesn't match.
>>
>> Clearly the regex can be improved here.
> So on top of the original patch:
> ---
>   scripts/checkpatch.pl | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f00a6c8..54eaa7 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5675,8 +5675,8 @@ sub process {
>   
>   # check for possible missing newlines at the end of common logging functions
>   		if (defined($stat) &&
> -		    $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> -		    $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> +		    $stat =~ /^\+\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> +		    $2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
>   			my $cnt = statement_rawlines($stat);
>   			my $extracted_string = "";
>   			for (my $i = 0; $i < $cnt; $i++) {

Hi,

	./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c

is missing line 189, even if it looks like a construction correctly spotted in some other files:
	if (foo) {
		dev_err(...);
		...
	};

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Marion & Christophe JAILLET]

Le 11/04/2020 à 09:12, Christophe JAILLET a écrit :
>
> Hi,
>
>     ./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c
>
> is missing line 189, even if it looks like a construction correctly 
> spotted in some other files:
>     if (foo) {
>         dev_err(...);
>         ...
>     };
>
> CJ
>
>
Oops, my mistake, this one is correctly spotted :)

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Sat, 2020-04-11 at 09:12 +0200, Christophe JAILLET wrote:
> 	./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c
> 
> is missing line 189, even if it looks like a construction correctly spotted in some other files:
> 	if (foo) {
> 		dev_err(...);
> 		...
> 	};

Hi Christophe, many thanks for doing more testing

Are you sure about this one?  I get:

$ ./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c
WARNING: Possible missing '\n' at the end of a logging message format string
#189: FILE: drivers/soc/kendryte/k210-sysctl.c:189:
+		dev_err(&pdev->dev, "failed to register clk");

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 11/04/2020 à 12:17, Joe Perches a écrit :
> On Sat, 2020-04-11 at 09:12 +0200, Christophe JAILLET wrote:
>> 	./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c
>>
>> is missing line 189, even if it looks like a construction correctly spotted in some other files:
>> 	if (foo) {
>> 		dev_err(...);
>> 		...
>> 	};
> Hi Christophe, many thanks for doing more testing
>
> Are you sure about this one?  I get:
>
> $ ./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c
> WARNING: Possible missing '\n' at the end of a logging message format string
> #189: FILE: drivers/soc/kendryte/k210-sysctl.c:189:
> +		dev_err(&pdev->dev, "failed to register clk");
No, this one is correctly spotted.
(I added the missing \n, then ran checkpatch.pl, then wondered why it 
was not spotting the issue!)

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Christophe JAILLET]

Le 11/04/2020 à 12:17, Joe Perches a écrit :
> On Sat, 2020-04-11 at 09:12 +0200, Christophe JAILLET wrote:
>> 	./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c
>>
>> is missing line 189, even if it looks like a construction correctly spotted in some other files:
>> 	if (foo) {
>> 		dev_err(...);
>> 		...
>> 	};
> Hi Christophe, many thanks for doing more testing
>
> Are you sure about this one?  I get:
>
> $ ./scripts/checkpatch.pl -f drivers/soc/kendryte/k210-sysctl.c
> WARNING: Possible missing '\n' at the end of a logging message format string
> #189: FILE: drivers/soc/kendryte/k210-sysctl.c:189:
> +		dev_err(&pdev->dev, "failed to register clk");
No, this one is correctly spotted.
(I added the missing \n, then ran checkpatch.pl, then wondered why it 
was not spotting the issue!)

CJ

Re: [PATCH] checkpatch: check for missing \n at the end of logging message

[Joe Perches]

On Tue, 2020-04-07 at 22:49 +0200, Christophe JAILLET wrote:
> Strings logged with pr_xxx and dev_xxx often lack a trailing '\n'.
> Introduce new tests to try to catch them early.

Unintentional resend?

> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This is more a PoC for now.
> 
> Regex could be improved, merged, ...
> We could also check for surrounding pr_cont...
> 
> This patch is based on idea from [1]. coccinelle spots too many places
> where \n are missing (~ 2800 with the heuristic I've used).
> Fixing them would be painful.
> I instead propose to teach checkpatch.pl about it to try to spot cases
> early and avoid introducing new cases.
> 
> [1]: https://marc.info/?l=kernel-janitors&m=158619533629657&w=4
> ---
>  scripts/checkpatch.pl | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c392ab8ea12e..792804bd6ad9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5676,6 +5676,16 @@ sub process {
>  			}
>  		}
>  
> +# check for missing \n at the end of logging function
> +		if ($line =~ /\bpr_(emerg|alert|crit|err|warning|warn|notice|info|debug|dbg)\s*\("([^"]*(?<!\\n))"/) {
> +			WARN("MISSING NL",
> +			     "Possible missing '\\n' at the end of a log message\n" . $hereprev);
> +		}
> +		if ($line =~ /\bdev_(emerg|alert|crit|err|warning|warn|notice|info|debug|dbg)\s*\([^,]*,\s*"([^"]*(?<!\\n))"/) {
> +			WARN("MISSING NL",
> +			     "Possible missing '\\n' at the end of a log message\n" . $hereprev);
> +		}
> +
>  # check for logging functions with KERN_<LEVEL>
>  		if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
>  		    $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {