[PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

Check for lines with a log function using a relatively strict regular
expression catching only printk, dev_* and pr_* functions. Once
one is found, accumulate further lines for any functions that
are split over multiple lines. Stop accumulating the log function line
when we find a ';' or match the full format string.

A get_log_fmt() function is used to find either the first or
second argument (depending on the log function matched) as the
format string. If the format argument ends in '\n"', then we conclude
that the logging call is correct. If the line ends in '%s"', then we
only print a CHK warning as the developer *may* have the newline in an
argument string. Otherwise we print a warning that the line is
likely missing a new line.

Because we are trying to match multiple lines, we must also match
lines that are part of the context of the patch. In order to ensure
we don't print a message on log lines that are only part of context,
we track that at least one line was added using the $log_func_changed
flag.

To handle continuations, during pre-processing we search for all
pr_cont and KERN_CONT instances and create a hash set of the line
numbers of all preceding log function calls. The hash set is then used
to mask out any calls that are likely followed by continuations. This
should handle many cases, however a small number of false positives are
likely going to occur in cases where a patch's context does not cover
the following KERN_CONT. This is acceptable, in my opinion, seeing
KERN_CONT is already discouraged and will create it's own warnings that
the developer has to reason about.

I've created a small test suite to test this change which can be found
on github[1]. I've also run this on the kernel source and found more
than 6000 violations. I reviewed a random sampling of these for false
positives and have not found any.

Thanks to Joe for describing how to properly test a change such as this.

[1] https://github.com/lsgunth/checkpatch-tests

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Joe Perches <joe@perches.com>
---

Here's my second attempt at this. As described in the commit log,
I've made it a bit more sophisticated and fixed a number of issues with
how the patch format is processed. The testing has also improved
significantly having run it on the entire kernel source as well as
a suite of test cases. The patch isn't entirely perfect (per the note
about false positives) but I feel it's close enough that its benefits
outweighs this. (As it only annoys developers who are working with a
discouraged feature.)

As mentioned in the commit log, I've found more than 6000 instances
that look like this error. I've seen a few cases that appear to have
intended to use KERN_CONT but didn't and are thus caught by this.
If there are any heroic kernel janitors that are interested in tackling
these issues, the list can be found here:

https://github.com/lsgunth/checkpatch-tests/blob/master/results/v4.14

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8b80bac055e4..cdd767af6566 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -460,6 +460,34 @@ our $logFunctions = qr{(?x:
 	seq_vprintf|seq_printf|seq_puts
 )};

+
+our $logNewLineFirstArg = qr{(?x:
+	printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
+	pr_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)
+)};
+
+our $logNewLineSecondArg = qr{(?x:
+	dev_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|WARN)(?:_ratelimited|_once|)
+)};
+
+our $logNewLineFunctions = qr{(?x:
+    $logNewLineFirstArg |
+    $logNewLineSecondArg
+)};
+
+sub get_log_fmt {
+	my ($line, $rawline) = @_;
+
+	if ($line =~ m/\b$logNewLineFirstArg\(([^,)]+)[,)]/g) {
+		return substr($rawline, $-[1], $+[1] - $-[1]);
+	} elsif($line =~ m/\b$logNewLineSecondArg\([^,]+,([^,)]+)[,)]/g) {
+		return substr($rawline, $-[1], $+[1] - $-[1]);
+	} else {
+		return "";
+	}
+}
+
+
 our $signature_tags = qr{(?xi:
 	Signed-off-by:|
 	Acked-by:|
@@ -2224,6 +2252,13 @@ sub process {

 	my $camelcase_file_seeded = 0;

+	my $in_log_function = 0;
+	my $log_func_changed = 0;
+	my $log_func_line = "";
+	my $log_func_rawline = "";
+	my $log_last_linenr = -1;
+	my %log_func_continued = ();
+
 	sanitise_line_reset();
 	my $line;
 	foreach my $rawline (@rawlines) {
@@ -2287,6 +2322,15 @@ sub process {
 			# simplify matching -- only bother with positive lines.
 			$line = sanitise_line($rawline);
 		}
+
+		if ($log_last_linenr > 0 && $line =~ /(KERN_CONT|pr_cont)/) {
+			$log_func_continued{$log_last_linenr} = ();
+		}
+
+		if ($line =~ /\b$logNewLineFunctions\(/) {
+			$log_last_linenr = $linenr;
+		}
+
 		push(@lines, $line);

 		if ($realcnt > 1) {
@@ -2321,6 +2365,7 @@ sub process {
 		    $line =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@(.*)/) {
 			my $context = $4;
 			$is_patch = 1;
+			$in_log_function = 0;
 			$first_line = $linenr + 1;
 			$realline=$1-1;
 			if (defined $2) {
@@ -3505,6 +3550,41 @@ sub process {
 		}
 		$prev_values = substr($curr_values, -1);

+# check for logging functions with lines that don't end in a '\n"'
+		if ($line =~ /\b$logNewLineFunctions\(/ &&
+		    !exists $log_func_continued{$linenr}) {
+			$in_log_function = 1;
+			$log_func_changed = 0;
+			$log_func_rawline = "";
+			$log_func_line = "";
+		}
+		if ($in_log_function) {
+			if ($line =~ /^\+/)  {
+				$log_func_changed = 1;
+			}
+
+			$log_func_rawline .= $rawline;
+			$log_func_line .= $line;
+			my $fmt_string = get_log_fmt($log_func_line,
+						     $log_func_rawline);
+
+			if ($fmt_string && !$log_func_changed) {
+				$in_log_function = 0;
+			} elsif ($fmt_string =~ /\\n"$/) {
+				$in_log_function = 0;
+			} elsif ($fmt_string =~ /%s"$/) {
+				CHK("LOGGING_MISSING_NEWLINE",
+				    "Log message may not end in new line (\\n)\n" . $herecurr);
+				$in_log_function = 0;
+			} elsif ($fmt_string =~ /"$/) {
+				WARN("LOGGING_MISSING_NEWLINE",
+				     "Log messages should end in a line feed (\\n)\n" . $herecurr);
+				$in_log_function = 0;
+			} elsif ($line =~ /;$/) {
+				$in_log_function = 0;
+			}
+		}
+
 #ignore lines not being added
 		next if ($line =~ /^[^\+]/);

--
2.11.0

[PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

Check for lines with a log function using a relatively strict regular
expression catching only printk, dev_* and pr_* functions. Once
one is found, accumulate further lines for any functions that
are split over multiple lines. Stop accumulating the log function line
when we find a ';' or match the full format string.

A get_log_fmt() function is used to find either the first or
second argument (depending on the log function matched) as the
format string. If the format argument ends in '\n"', then we conclude
that the logging call is correct. If the line ends in '%s"', then we
only print a CHK warning as the developer *may* have the newline in an
argument string. Otherwise we print a warning that the line is
likely missing a new line.

Because we are trying to match multiple lines, we must also match
lines that are part of the context of the patch. In order to ensure
we don't print a message on log lines that are only part of context,
we track that at least one line was added using the $log_func_changed
flag.

To handle continuations, during pre-processing we search for all
pr_cont and KERN_CONT instances and create a hash set of the line
numbers of all preceding log function calls. The hash set is then used
to mask out any calls that are likely followed by continuations. This
should handle many cases, however a small number of false positives are
likely going to occur in cases where a patch's context does not cover
the following KERN_CONT. This is acceptable, in my opinion, seeing
KERN_CONT is already discouraged and will create it's own warnings that
the developer has to reason about.

I've created a small test suite to test this change which can be found
on github[1]. I've also run this on the kernel source and found more
than 6000 violations. I reviewed a random sampling of these for false
positives and have not found any.

Thanks to Joe for describing how to properly test a change such as this.

[1] https://github.com/lsgunth/checkpatch-tests

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Joe Perches <joe@perches.com>
---

Here's my second attempt at this. As described in the commit log,
I've made it a bit more sophisticated and fixed a number of issues with
how the patch format is processed. The testing has also improved
significantly having run it on the entire kernel source as well as
a suite of test cases. The patch isn't entirely perfect (per the note
about false positives) but I feel it's close enough that its benefits
outweighs this. (As it only annoys developers who are working with a
discouraged feature.)

As mentioned in the commit log, I've found more than 6000 instances
that look like this error. I've seen a few cases that appear to have
intended to use KERN_CONT but didn't and are thus caught by this.
If there are any heroic kernel janitors that are interested in tackling
these issues, the list can be found here:

https://github.com/lsgunth/checkpatch-tests/blob/master/results/v4.14

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8b80bac055e4..cdd767af6566 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -460,6 +460,34 @@ our $logFunctions = qr{(?x:
 	seq_vprintf|seq_printf|seq_puts
 )};

+
+our $logNewLineFirstArg = qr{(?x:
+	printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
+	pr_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)
+)};
+
+our $logNewLineSecondArg = qr{(?x:
+	dev_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|WARN)(?:_ratelimited|_once|)
+)};
+
+our $logNewLineFunctions = qr{(?x:
+    $logNewLineFirstArg |
+    $logNewLineSecondArg
+)};
+
+sub get_log_fmt {
+	my ($line, $rawline) = @_;
+
+	if ($line =~ m/\b$logNewLineFirstArg\(([^,)]+)[,)]/g) {
+		return substr($rawline, $-[1], $+[1] - $-[1]);
+	} elsif($line =~ m/\b$logNewLineSecondArg\([^,]+,([^,)]+)[,)]/g) {
+		return substr($rawline, $-[1], $+[1] - $-[1]);
+	} else {
+		return "";
+	}
+}
+
+
 our $signature_tags = qr{(?xi:
 	Signed-off-by:|
 	Acked-by:|
@@ -2224,6 +2252,13 @@ sub process {

 	my $camelcase_file_seeded = 0;

+	my $in_log_function = 0;
+	my $log_func_changed = 0;
+	my $log_func_line = "";
+	my $log_func_rawline = "";
+	my $log_last_linenr = -1;
+	my %log_func_continued = ();
+
 	sanitise_line_reset();
 	my $line;
 	foreach my $rawline (@rawlines) {
@@ -2287,6 +2322,15 @@ sub process {
 			# simplify matching -- only bother with positive lines.
 			$line = sanitise_line($rawline);
 		}
+
+		if ($log_last_linenr > 0 && $line =~ /(KERN_CONT|pr_cont)/) {
+			$log_func_continued{$log_last_linenr} = ();
+		}
+
+		if ($line =~ /\b$logNewLineFunctions\(/) {
+			$log_last_linenr = $linenr;
+		}
+
 		push(@lines, $line);

 		if ($realcnt > 1) {
@@ -2321,6 +2365,7 @@ sub process {
 		    $line =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@(.*)/) {
 			my $context = $4;
 			$is_patch = 1;
+			$in_log_function = 0;
 			$first_line = $linenr + 1;
 			$realline=$1-1;
 			if (defined $2) {
@@ -3505,6 +3550,41 @@ sub process {
 		}
 		$prev_values = substr($curr_values, -1);

+# check for logging functions with lines that don't end in a '\n"'
+		if ($line =~ /\b$logNewLineFunctions\(/ &&
+		    !exists $log_func_continued{$linenr}) {
+			$in_log_function = 1;
+			$log_func_changed = 0;
+			$log_func_rawline = "";
+			$log_func_line = "";
+		}
+		if ($in_log_function) {
+			if ($line =~ /^\+/)  {
+				$log_func_changed = 1;
+			}
+
+			$log_func_rawline .= $rawline;
+			$log_func_line .= $line;
+			my $fmt_string = get_log_fmt($log_func_line,
+						     $log_func_rawline);
+
+			if ($fmt_string && !$log_func_changed) {
+				$in_log_function = 0;
+			} elsif ($fmt_string =~ /\\n"$/) {
+				$in_log_function = 0;
+			} elsif ($fmt_string =~ /%s"$/) {
+				CHK("LOGGING_MISSING_NEWLINE",
+				    "Log message may not end in new line (\\n)\n" . $herecurr);
+				$in_log_function = 0;
+			} elsif ($fmt_string =~ /"$/) {
+				WARN("LOGGING_MISSING_NEWLINE",
+				     "Log messages should end in a line feed (\\n)\n" . $herecurr);
+				$in_log_function = 0;
+			} elsif ($line =~ /;$/) {
+				$in_log_function = 0;
+			}
+		}
+
 #ignore lines not being added
 		next if ($line =~ /^[^\+]/);

--
2.11.0

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sat, 25 Nov 2017, Logan Gunthorpe wrote:

> Check for lines with a log function using a relatively strict regular
> expression catching only printk, dev_* and pr_* functions. Once
> one is found, accumulate further lines for any functions that
> are split over multiple lines.

I don't understand at all the second sentence.  Are you staying with the
same call, or moving on to other calls?  Also, it would be the call that
is split over multiple lines, not the function split over multiple lines.

I think this would have been much easier with Cocccinelle where the code
is parsed and the control-flow graph is available to see whether there is
a pr_cont afterwards.  But if it works, then it is surely good enough.

julia

> Stop accumulating the log function line
> when we find a ';' or match the full format string.
>
> A get_log_fmt() function is used to find either the first or
> second argument (depending on the log function matched) as the
> format string. If the format argument ends in '\n"', then we conclude
> that the logging call is correct. If the line ends in '%s"', then we
> only print a CHK warning as the developer *may* have the newline in an
> argument string. Otherwise we print a warning that the line is
> likely missing a new line.
>
> Because we are trying to match multiple lines, we must also match
> lines that are part of the context of the patch. In order to ensure
> we don't print a message on log lines that are only part of context,
> we track that at least one line was added using the $log_func_changed
> flag.
>
> To handle continuations, during pre-processing we search for all
> pr_cont and KERN_CONT instances and create a hash set of the line
> numbers of all preceding log function calls. The hash set is then used
> to mask out any calls that are likely followed by continuations. This
> should handle many cases, however a small number of false positives are
> likely going to occur in cases where a patch's context does not cover
> the following KERN_CONT. This is acceptable, in my opinion, seeing
> KERN_CONT is already discouraged and will create it's own warnings that
> the developer has to reason about.
>
> I've created a small test suite to test this change which can be found
> on github[1]. I've also run this on the kernel source and found more
> than 6000 violations. I reviewed a random sampling of these for false
> positives and have not found any.
>
> Thanks to Joe for describing how to properly test a change such as this.
>
> [1] https://github.com/lsgunth/checkpatch-tests
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Cc: Andy Whitcroft <apw@canonical.com>
> Cc: Joe Perches <joe@perches.com>
> ---
>
> Here's my second attempt at this. As described in the commit log,
> I've made it a bit more sophisticated and fixed a number of issues with
> how the patch format is processed. The testing has also improved
> significantly having run it on the entire kernel source as well as
> a suite of test cases. The patch isn't entirely perfect (per the note
> about false positives) but I feel it's close enough that its benefits
> outweighs this. (As it only annoys developers who are working with a
> discouraged feature.)
>
> As mentioned in the commit log, I've found more than 6000 instances
> that look like this error. I've seen a few cases that appear to have
> intended to use KERN_CONT but didn't and are thus caught by this.
> If there are any heroic kernel janitors that are interested in tackling
> these issues, the list can be found here:
>
> https://github.com/lsgunth/checkpatch-tests/blob/master/results/v4.14
>
>  scripts/checkpatch.pl | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 8b80bac055e4..cdd767af6566 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -460,6 +460,34 @@ our $logFunctions = qr{(?x:
>  	seq_vprintf|seq_printf|seq_puts
>  )};
>
> +
> +our $logNewLineFirstArg = qr{(?x:
> +	printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
> +	pr_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)
> +)};
> +
> +our $logNewLineSecondArg = qr{(?x:
> +	dev_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|WARN)(?:_ratelimited|_once|)
> +)};
> +
> +our $logNewLineFunctions = qr{(?x:
> +    $logNewLineFirstArg |
> +    $logNewLineSecondArg
> +)};
> +
> +sub get_log_fmt {
> +	my ($line, $rawline) = @_;
> +
> +	if ($line =~ m/\b$logNewLineFirstArg\(([^,)]+)[,)]/g) {
> +		return substr($rawline, $-[1], $+[1] - $-[1]);
> +	} elsif($line =~ m/\b$logNewLineSecondArg\([^,]+,([^,)]+)[,)]/g) {
> +		return substr($rawline, $-[1], $+[1] - $-[1]);
> +	} else {
> +		return "";
> +	}
> +}
> +
> +
>  our $signature_tags = qr{(?xi:
>  	Signed-off-by:|
>  	Acked-by:|
> @@ -2224,6 +2252,13 @@ sub process {
>
>  	my $camelcase_file_seeded = 0;
>
> +	my $in_log_function = 0;
> +	my $log_func_changed = 0;
> +	my $log_func_line = "";
> +	my $log_func_rawline = "";
> +	my $log_last_linenr = -1;
> +	my %log_func_continued = ();
> +
>  	sanitise_line_reset();
>  	my $line;
>  	foreach my $rawline (@rawlines) {
> @@ -2287,6 +2322,15 @@ sub process {
>  			# simplify matching -- only bother with positive lines.
>  			$line = sanitise_line($rawline);
>  		}
> +
> +		if ($log_last_linenr > 0 && $line =~ /(KERN_CONT|pr_cont)/) {
> +			$log_func_continued{$log_last_linenr} = ();
> +		}
> +
> +		if ($line =~ /\b$logNewLineFunctions\(/) {
> +			$log_last_linenr = $linenr;
> +		}
> +
>  		push(@lines, $line);
>
>  		if ($realcnt > 1) {
> @@ -2321,6 +2365,7 @@ sub process {
>  		    $line =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@(.*)/) {
>  			my $context = $4;
>  			$is_patch = 1;
> +			$in_log_function = 0;
>  			$first_line = $linenr + 1;
>  			$realline=$1-1;
>  			if (defined $2) {
> @@ -3505,6 +3550,41 @@ sub process {
>  		}
>  		$prev_values = substr($curr_values, -1);
>
> +# check for logging functions with lines that don't end in a '\n"'
> +		if ($line =~ /\b$logNewLineFunctions\(/ &&
> +		    !exists $log_func_continued{$linenr}) {
> +			$in_log_function = 1;
> +			$log_func_changed = 0;
> +			$log_func_rawline = "";
> +			$log_func_line = "";
> +		}
> +		if ($in_log_function) {
> +			if ($line =~ /^\+/)  {
> +				$log_func_changed = 1;
> +			}
> +
> +			$log_func_rawline .= $rawline;
> +			$log_func_line .= $line;
> +			my $fmt_string = get_log_fmt($log_func_line,
> +						     $log_func_rawline);
> +
> +			if ($fmt_string && !$log_func_changed) {
> +				$in_log_function = 0;
> +			} elsif ($fmt_string =~ /\\n"$/) {
> +				$in_log_function = 0;
> +			} elsif ($fmt_string =~ /%s"$/) {
> +				CHK("LOGGING_MISSING_NEWLINE",
> +				    "Log message may not end in new line (\\n)\n" . $herecurr);
> +				$in_log_function = 0;
> +			} elsif ($fmt_string =~ /"$/) {
> +				WARN("LOGGING_MISSING_NEWLINE",
> +				     "Log messages should end in a line feed (\\n)\n" . $herecurr);
> +				$in_log_function = 0;
> +			} elsif ($line =~ /;$/) {
> +				$in_log_function = 0;
> +			}
> +		}
> +
>  #ignore lines not being added
>  		next if ($line =~ /^[^\+]/);
>
> --
> 2.11.0
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sat, 25 Nov 2017, Logan Gunthorpe wrote:

> Check for lines with a log function using a relatively strict regular
> expression catching only printk, dev_* and pr_* functions. Once
> one is found, accumulate further lines for any functions that
> are split over multiple lines.

I don't understand at all the second sentence.  Are you staying with the
same call, or moving on to other calls?  Also, it would be the call that
is split over multiple lines, not the function split over multiple lines.

I think this would have been much easier with Cocccinelle where the code
is parsed and the control-flow graph is available to see whether there is
a pr_cont afterwards.  But if it works, then it is surely good enough.

julia

> Stop accumulating the log function line
> when we find a ';' or match the full format string.
>
> A get_log_fmt() function is used to find either the first or
> second argument (depending on the log function matched) as the
> format string. If the format argument ends in '\n"', then we conclude
> that the logging call is correct. If the line ends in '%s"', then we
> only print a CHK warning as the developer *may* have the newline in an
> argument string. Otherwise we print a warning that the line is
> likely missing a new line.
>
> Because we are trying to match multiple lines, we must also match
> lines that are part of the context of the patch. In order to ensure
> we don't print a message on log lines that are only part of context,
> we track that at least one line was added using the $log_func_changed
> flag.
>
> To handle continuations, during pre-processing we search for all
> pr_cont and KERN_CONT instances and create a hash set of the line
> numbers of all preceding log function calls. The hash set is then used
> to mask out any calls that are likely followed by continuations. This
> should handle many cases, however a small number of false positives are
> likely going to occur in cases where a patch's context does not cover
> the following KERN_CONT. This is acceptable, in my opinion, seeing
> KERN_CONT is already discouraged and will create it's own warnings that
> the developer has to reason about.
>
> I've created a small test suite to test this change which can be found
> on github[1]. I've also run this on the kernel source and found more
> than 6000 violations. I reviewed a random sampling of these for false
> positives and have not found any.
>
> Thanks to Joe for describing how to properly test a change such as this.
>
> [1] https://github.com/lsgunth/checkpatch-tests
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Cc: Andy Whitcroft <apw@canonical.com>
> Cc: Joe Perches <joe@perches.com>
> ---
>
> Here's my second attempt at this. As described in the commit log,
> I've made it a bit more sophisticated and fixed a number of issues with
> how the patch format is processed. The testing has also improved
> significantly having run it on the entire kernel source as well as
> a suite of test cases. The patch isn't entirely perfect (per the note
> about false positives) but I feel it's close enough that its benefits
> outweighs this. (As it only annoys developers who are working with a
> discouraged feature.)
>
> As mentioned in the commit log, I've found more than 6000 instances
> that look like this error. I've seen a few cases that appear to have
> intended to use KERN_CONT but didn't and are thus caught by this.
> If there are any heroic kernel janitors that are interested in tackling
> these issues, the list can be found here:
>
> https://github.com/lsgunth/checkpatch-tests/blob/master/results/v4.14
>
>  scripts/checkpatch.pl | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 8b80bac055e4..cdd767af6566 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -460,6 +460,34 @@ our $logFunctions = qr{(?x:
>  	seq_vprintf|seq_printf|seq_puts
>  )};
>
> +
> +our $logNewLineFirstArg = qr{(?x:
> +	printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
> +	pr_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)
> +)};
> +
> +our $logNewLineSecondArg = qr{(?x:
> +	dev_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|WARN)(?:_ratelimited|_once|)
> +)};
> +
> +our $logNewLineFunctions = qr{(?x:
> +    $logNewLineFirstArg |
> +    $logNewLineSecondArg
> +)};
> +
> +sub get_log_fmt {
> +	my ($line, $rawline) = @_;
> +
> +	if ($line =~ m/\b$logNewLineFirstArg\(([^,)]+)[,)]/g) {
> +		return substr($rawline, $-[1], $+[1] - $-[1]);
> +	} elsif($line =~ m/\b$logNewLineSecondArg\([^,]+,([^,)]+)[,)]/g) {
> +		return substr($rawline, $-[1], $+[1] - $-[1]);
> +	} else {
> +		return "";
> +	}
> +}
> +
> +
>  our $signature_tags = qr{(?xi:
>  	Signed-off-by:|
>  	Acked-by:|
> @@ -2224,6 +2252,13 @@ sub process {
>
>  	my $camelcase_file_seeded = 0;
>
> +	my $in_log_function = 0;
> +	my $log_func_changed = 0;
> +	my $log_func_line = "";
> +	my $log_func_rawline = "";
> +	my $log_last_linenr = -1;
> +	my %log_func_continued = ();
> +
>  	sanitise_line_reset();
>  	my $line;
>  	foreach my $rawline (@rawlines) {
> @@ -2287,6 +2322,15 @@ sub process {
>  			# simplify matching -- only bother with positive lines.
>  			$line = sanitise_line($rawline);
>  		}
> +
> +		if ($log_last_linenr > 0 && $line =~ /(KERN_CONT|pr_cont)/) {
> +			$log_func_continued{$log_last_linenr} = ();
> +		}
> +
> +		if ($line =~ /\b$logNewLineFunctions\(/) {
> +			$log_last_linenr = $linenr;
> +		}
> +
>  		push(@lines, $line);
>
>  		if ($realcnt > 1) {
> @@ -2321,6 +2365,7 @@ sub process {
>  		    $line =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@(.*)/) {
>  			my $context = $4;
>  			$is_patch = 1;
> +			$in_log_function = 0;
>  			$first_line = $linenr + 1;
>  			$realline=$1-1;
>  			if (defined $2) {
> @@ -3505,6 +3550,41 @@ sub process {
>  		}
>  		$prev_values = substr($curr_values, -1);
>
> +# check for logging functions with lines that don't end in a '\n"'
> +		if ($line =~ /\b$logNewLineFunctions\(/ &&
> +		    !exists $log_func_continued{$linenr}) {
> +			$in_log_function = 1;
> +			$log_func_changed = 0;
> +			$log_func_rawline = "";
> +			$log_func_line = "";
> +		}
> +		if ($in_log_function) {
> +			if ($line =~ /^\+/)  {
> +				$log_func_changed = 1;
> +			}
> +
> +			$log_func_rawline .= $rawline;
> +			$log_func_line .= $line;
> +			my $fmt_string = get_log_fmt($log_func_line,
> +						     $log_func_rawline);
> +
> +			if ($fmt_string && !$log_func_changed) {
> +				$in_log_function = 0;
> +			} elsif ($fmt_string =~ /\\n"$/) {
> +				$in_log_function = 0;
> +			} elsif ($fmt_string =~ /%s"$/) {
> +				CHK("LOGGING_MISSING_NEWLINE",
> +				    "Log message may not end in new line (\\n)\n" . $herecurr);
> +				$in_log_function = 0;
> +			} elsif ($fmt_string =~ /"$/) {
> +				WARN("LOGGING_MISSING_NEWLINE",
> +				     "Log messages should end in a line feed (\\n)\n" . $herecurr);
> +				$in_log_function = 0;
> +			} elsif ($line =~ /;$/) {
> +				$in_log_function = 0;
> +			}
> +		}
> +
>  #ignore lines not being added
>  		next if ($line =~ /^[^\+]/);
>
> --
> 2.11.0
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 06:51 +0100, Julia Lawall wrote:
> 
> On Sat, 25 Nov 2017, Logan Gunthorpe wrote:
> 
> > Check for lines with a log function using a relatively strict regular
> > expression catching only printk, dev_* and pr_* functions. Once
> > one is found, accumulate further lines for any functions that
> > are split over multiple lines.
> 
> I don't understand at all the second sentence.  Are you staying with the
> same call, or moving on to other calls?  Also, it would be the call that
> is split over multiple lines, not the function split over multiple lines.
> 
> I think this would have been much easier with Cocccinelle where the code
> is parsed and the control-flow graph is available to see whether there is
> a pr_cont afterwards.  But if it works, then it is surely good enough.

It doesn't really work.

Many of the messages aren't missing newlines.

I only looked a the first few dozen instances, but many of
them aren't really missing newlines, but are now missing a
KERN_CONT annotation.

This is because Linus changed how printk worked awhile
ago in

commit 4bcc595ccd80decb4245096e3d1258989c50ed41
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat Oct 8 20:32:40 2016 -0700

printk: reinstate KERN_CONT for printing continuation lines

Most of that commit message is BS, but the net effect is
that now printks must have a KERN_<LEVEL> marker or a
newline is inserted before the format.

Also, this patch logic will be very confused by patch
blocks and not files.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 06:51 +0100, Julia Lawall wrote:
> 
> On Sat, 25 Nov 2017, Logan Gunthorpe wrote:
> 
> > Check for lines with a log function using a relatively strict regular
> > expression catching only printk, dev_* and pr_* functions. Once
> > one is found, accumulate further lines for any functions that
> > are split over multiple lines.
> 
> I don't understand at all the second sentence.  Are you staying with the
> same call, or moving on to other calls?  Also, it would be the call that
> is split over multiple lines, not the function split over multiple lines.
> 
> I think this would have been much easier with Cocccinelle where the code
> is parsed and the control-flow graph is available to see whether there is
> a pr_cont afterwards.  But if it works, then it is surely good enough.

It doesn't really work.

Many of the messages aren't missing newlines.

I only looked a the first few dozen instances, but many of
them aren't really missing newlines, but are now missing a
KERN_CONT annotation.

This is because Linus changed how printk worked awhile
ago in

commit 4bcc595ccd80decb4245096e3d1258989c50ed41
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat Oct 8 20:32:40 2016 -0700

printk: reinstate KERN_CONT for printing continuation lines

Most of that commit message is BS, but the net effect is
that now printks must have a KERN_<LEVEL> marker or a
newline is inserted before the format.

Also, this patch logic will be very confused by patch
blocks and not files.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 25/11/17 10:51 PM, Julia Lawall wrote:
> I don't understand at all the second sentence.  Are you staying with the
> same call, or moving on to other calls?  Also, it would be the call that
> is split over multiple lines, not the function split over multiple lines.

Yes, you are correct it should be "call" instead of "function".

> I think this would have been much easier with Cocccinelle where the code
> is parsed and the control-flow graph is available to see whether there is
> a pr_cont afterwards.  But if it works, then it is surely good enough.

I don't disagree at all. However, to my knowledge, not a lot of kernel
developers run a set of coccinelle scripts on their change sets. The
point is to catch these mistakes before the patch is submitted.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 25/11/17 10:51 PM, Julia Lawall wrote:
> I don't understand at all the second sentence.  Are you staying with the
> same call, or moving on to other calls?  Also, it would be the call that
> is split over multiple lines, not the function split over multiple lines.

Yes, you are correct it should be "call" instead of "function".

> I think this would have been much easier with Cocccinelle where the code
> is parsed and the control-flow graph is available to see whether there is
> a pr_cont afterwards.  But if it works, then it is surely good enough.

I don't disagree at all. However, to my knowledge, not a lot of kernel
developers run a set of coccinelle scripts on their change sets. The
point is to catch these mistakes before the patch is submitted.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 25/11/17 10:51 PM, Julia Lawall wrote:
> > I don't understand at all the second sentence.  Are you staying with the
> > same call, or moving on to other calls?  Also, it would be the call that
> > is split over multiple lines, not the function split over multiple lines.
>
> Yes, you are correct it should be "call" instead of "function".
>
> > I think this would have been much easier with Cocccinelle where the code
> > is parsed and the control-flow graph is available to see whether there is
> > a pr_cont afterwards.  But if it works, then it is surely good enough.
>
> I don't disagree at all. However, to my knowledge, not a lot of kernel
> developers run a set of coccinelle scripts on their change sets. The
> point is to catch these mistakes before the patch is submitted.

I don't know.  In any case, a Coccinelle script would get run by the 0-day
build testing service, which checks lots of trees.  Perhaps both are
useful, since Joe had some conerns about the amount of relevant context
available in a patch.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 25/11/17 10:51 PM, Julia Lawall wrote:
> > I don't understand at all the second sentence.  Are you staying with the
> > same call, or moving on to other calls?  Also, it would be the call that
> > is split over multiple lines, not the function split over multiple lines.
>
> Yes, you are correct it should be "call" instead of "function".
>
> > I think this would have been much easier with Cocccinelle where the code
> > is parsed and the control-flow graph is available to see whether there is
> > a pr_cont afterwards.  But if it works, then it is surely good enough.
>
> I don't disagree at all. However, to my knowledge, not a lot of kernel
> developers run a set of coccinelle scripts on their change sets. The
> point is to catch these mistakes before the patch is submitted.

I don't know.  In any case, a Coccinelle script would get run by the 0-day
build testing service, which checks lots of trees.  Perhaps both are
useful, since Joe had some conerns about the amount of relevant context
available in a patch.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 25/11/17 11:01 PM, Joe Perches wrote:
> It doesn't really work.

That's rather hyperbolic and I don't appreciate the tone.

> Many of the messages aren't missing newlines.
> 
> I only looked a the first few dozen instances, but many of
> them aren't really missing newlines, but are now missing a
> KERN_CONT annotation.

True, and I mentioned that. However, these instances are still incorrect
and deserve a warning. I don't see how any tool (even one written in
Coccinelle) could determine whether the programmer intended to have a
new line or intended for the next line to be a continuation. But if the
developer gets a warning they'd at least look at it. Given that
KERN_CONT usage is discouraged, I think warning that a new line is
required is acceptable.

> Most of that commit message is BS, but the net effect is
> that now printks must have a KERN_<LEVEL> marker or a
> newline is inserted before the format.

Yes, that just means that all the instances that should be using
KERN_CONT are now *actually* broken. It's also part of what created the
mess in the first place as it makes the final new line seem to not be
required. As a result, there are now plenty of places in the kernel that
are inconsistent. That's why this patch is needed.

> Also, this patch logic will be very confused by patch
> blocks and not files.

No, it's not. (With the exception of the false positives I noted due to
KERN_CONTs not making it into the patch context.) I've tested this. I
even created some pathological tests[1] to ensure crossing hunk
boundaries and other weirdness works correctly. There may also be false
negatives in extreme cases if the entire call site doesn't fit in the
patch context but it's still better than nothing and will catch all
newly added calls. So if you're going to make such statements I suggest
you back it up with evidence.

Logan

[1]
https://github.com/lsgunth/checkpatch-tests/blob/master/examples/test2.patch

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 25/11/17 11:01 PM, Joe Perches wrote:
> It doesn't really work.

That's rather hyperbolic and I don't appreciate the tone.

> Many of the messages aren't missing newlines.
> 
> I only looked a the first few dozen instances, but many of
> them aren't really missing newlines, but are now missing a
> KERN_CONT annotation.

True, and I mentioned that. However, these instances are still incorrect
and deserve a warning. I don't see how any tool (even one written in
Coccinelle) could determine whether the programmer intended to have a
new line or intended for the next line to be a continuation. But if the
developer gets a warning they'd at least look at it. Given that
KERN_CONT usage is discouraged, I think warning that a new line is
required is acceptable.

> Most of that commit message is BS, but the net effect is
> that now printks must have a KERN_<LEVEL> marker or a
> newline is inserted before the format.

Yes, that just means that all the instances that should be using
KERN_CONT are now *actually* broken. It's also part of what created the
mess in the first place as it makes the final new line seem to not be
required. As a result, there are now plenty of places in the kernel that
are inconsistent. That's why this patch is needed.

> Also, this patch logic will be very confused by patch
> blocks and not files.

No, it's not. (With the exception of the false positives I noted due to
KERN_CONTs not making it into the patch context.) I've tested this. I
even created some pathological tests[1] to ensure crossing hunk
boundaries and other weirdness works correctly. There may also be false
negatives in extreme cases if the entire call site doesn't fit in the
patch context but it's still better than nothing and will catch all
newly added calls. So if you're going to make such statements I suggest
you back it up with evidence.

Logan

[1]
https://github.com/lsgunth/checkpatch-tests/blob/master/examples/test2.patch

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 10:09 AM, Julia Lawall wrote:
> I don't know.  In any case, a Coccinelle script would get run by the 0-day
> build testing service, which checks lots of trees.  Perhaps both are
> useful, since Joe had some conerns about the amount of relevant context
> available in a patch.

Yup, both could certainly be useful. A coccinelle script would likely be
able to catch a few false negatives that might pass through the
checkpatch script. It'll likely have similar difficulties with
KERN_CONTs though.

Also, I don't really know, but it might be tough enabling a script to
run on 0-day with the ~6000 potential errors already existing.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 10:09 AM, Julia Lawall wrote:
> I don't know.  In any case, a Coccinelle script would get run by the 0-day
> build testing service, which checks lots of trees.  Perhaps both are
> useful, since Joe had some conerns about the amount of relevant context
> available in a patch.

Yup, both could certainly be useful. A coccinelle script would likely be
able to catch a few false negatives that might pass through the
checkpatch script. It'll likely have similar difficulties with
KERN_CONTs though.

Also, I don't really know, but it might be tough enabling a script to
run on 0-day with the ~6000 potential errors already existing.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 26/11/17 10:09 AM, Julia Lawall wrote:
> > I don't know.  In any case, a Coccinelle script would get run by the 0-day
> > build testing service, which checks lots of trees.  Perhaps both are
> > useful, since Joe had some conerns about the amount of relevant context
> > available in a patch.
>
> Yup, both could certainly be useful. A coccinelle script would likely be
> able to catch a few false negatives that might pass through the
> checkpatch script. It'll likely have similar difficulties with
> KERN_CONTs though.

Not sure why.  I just assume that a printk that has no KERN_ is adding a
newline, which is my understanding of Joe's comment.

The main limitation that is likely to remain in my script is that
Coccinelle doesn't always understand ifdefs properly.  So
#ifdef
printk("xxx");
#else
printk("yyy");
#endif
pr_cont("zzz");

may give a warning about the first printk.

> Also, I don't really know, but it might be tough enabling a script to
> run on 0-day with the ~6000 potential errors already existing.

0-day only runs on changed files and only reports on changed code, to
the best of my understanding, so I don't think it is a problem.

julia

>
> Logan
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 26/11/17 10:09 AM, Julia Lawall wrote:
> > I don't know.  In any case, a Coccinelle script would get run by the 0-day
> > build testing service, which checks lots of trees.  Perhaps both are
> > useful, since Joe had some conerns about the amount of relevant context
> > available in a patch.
>
> Yup, both could certainly be useful. A coccinelle script would likely be
> able to catch a few false negatives that might pass through the
> checkpatch script. It'll likely have similar difficulties with
> KERN_CONTs though.

Not sure why.  I just assume that a printk that has no KERN_ is adding a
newline, which is my understanding of Joe's comment.

The main limitation that is likely to remain in my script is that
Coccinelle doesn't always understand ifdefs properly.  So
#ifdef
printk("xxx");
#else
printk("yyy");
#endif
pr_cont("zzz");

may give a warning about the first printk.

> Also, I don't really know, but it might be tough enabling a script to
> run on 0-day with the ~6000 potential errors already existing.

0-day only runs on changed files and only reports on changed code, to
the best of my understanding, so I don't think it is a problem.

julia

>
> Logan
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 11:17 AM, Julia Lawall wrote:
> The main limitation that is likely to remain in my script is that
> Coccinelle doesn't always understand ifdefs properly.  So
> #ifdef
> printk("xxx");
> #else
> printk("yyy");
> #endif
> pr_cont("zzz");
> 
> may give a warning about the first printk.

Yes, that's another clever corner case I didn't think of. My patch will
have the same issue. But that's terrible style[1] and I should hope it's
quite rare.

> 0-day only runs on changed files and only reports on changed code, to
> the best of my understanding, so I don't think it is a problem.

Oh, cool.

Logan

[1]
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 11:17 AM, Julia Lawall wrote:
> The main limitation that is likely to remain in my script is that
> Coccinelle doesn't always understand ifdefs properly.  So
> #ifdef
> printk("xxx");
> #else
> printk("yyy");
> #endif
> pr_cont("zzz");
> 
> may give a warning about the first printk.

Yes, that's another clever corner case I didn't think of. My patch will
have the same issue. But that's terrible style[1] and I should hope it's
quite rare.

> 0-day only runs on changed files and only reports on changed code, to
> the best of my understanding, so I don't think it is a problem.

Oh, cool.

Logan

[1]
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 10:38 -0700, Logan Gunthorpe wrote:
> 
> On 25/11/17 11:01 PM, Joe Perches wrote:
> > It doesn't really work.
> 
> That's rather hyperbolic and I don't appreciate the tone.

What I wrote is neither hyperbolic nor tonal.

I'm rather familiar with checkpatch.

High false positive messages are avoided by checkpatch and
new tests with high false positive rates are not accepted.

This proposal has a very high false positive rate.

So, as written.  nack.

Keep at it though.
Maybe something useful will be produced with more effort.

cheers, Joe

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 10:38 -0700, Logan Gunthorpe wrote:
> 
> On 25/11/17 11:01 PM, Joe Perches wrote:
> > It doesn't really work.
> 
> That's rather hyperbolic and I don't appreciate the tone.

What I wrote is neither hyperbolic nor tonal.

I'm rather familiar with checkpatch.

High false positive messages are avoided by checkpatch and
new tests with high false positive rates are not accepted.

This proposal has a very high false positive rate.

So, as written.  nack.

Keep at it though.
Maybe something useful will be produced with more effort.

cheers, Joe

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote:
> My semantic patch and results are below.  The semantic patch has some
> features that may or may not be desired:
> 
> 1.  It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding
> functions that are sometimes used with a format string ending with a
> newline.  To reduce false positives, such a function is ignored if it is
> sometimes used with a string that ends in a space.  This could lead to
> false positives where actually one of the calls has a \n that it should
> not have.
> 
> 2.  Coccinelle puts multipart strings on a single line.  So the rule goes
> a little further and eliminates the multipartness.  Basically "xxx " "yyy"
> becomes "xxx yyy" regardless of the length of the result.

What about the semi-common string concatenation "foo" #var "bar" ?

> 3.  Some prints appear not to end with a newline because they end with \n.
> where .\n was likely intended.  Instead of creating \n.\n, the semantic
> patch just moves the .to the left of the . And if there was .\n. it just
> drops the final period.

That may be a problem if the sentence is "something...\n"

There seem to be many false positives in here too.

cheers, Joe

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote:
> My semantic patch and results are below.  The semantic patch has some
> features that may or may not be desired:
> 
> 1.  It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding
> functions that are sometimes used with a format string ending with a
> newline.  To reduce false positives, such a function is ignored if it is
> sometimes used with a string that ends in a space.  This could lead to
> false positives where actually one of the calls has a \n that it should
> not have.
> 
> 2.  Coccinelle puts multipart strings on a single line.  So the rule goes
> a little further and eliminates the multipartness.  Basically "xxx " "yyy"
> becomes "xxx yyy" regardless of the length of the result.

What about the semi-common string concatenation "foo" #var "bar" ?

> 3.  Some prints appear not to end with a newline because they end with \n.
> where .\n was likely intended.  Instead of creating \n.\n, the semantic
> patch just moves the .to the left of the . And if there was .\n. it just
> drops the final period.

That may be a problem if the sentence is "something...\n"

There seem to be many false positives in here too.

cheers, Joe

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 19:17 +0100, Julia Lawall wrote:
> I just assume that a printk that has no KERN_ is adding a
> newline, which is my understanding of Joe's comment.

More precisely:

Any printk without an initial KERN_CONT prepends a newline
if the last printk content char emitted that is not part
of a printk timestamp/header was not a newline.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 19:17 +0100, Julia Lawall wrote:
> I just assume that a printk that has no KERN_ is adding a
> newline, which is my understanding of Joe's comment.

More precisely:

Any printk without an initial KERN_CONT prepends a newline
if the last printk content char emitted that is not part
of a printk timestamp/header was not a newline.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 03:29 PM, Joe Perches wrote:
> This proposal has a very high false positive rate.

The only actual false positive you've pointed out is the one that is
just incorrect in the wrong way (the author should have used a KERN_CONT
but did not). I could easily do something similar to what Julia proposes
and produce a different warning if the string ends in a space. But I
don't see any bullet proof way for any script to deduce what the author
actually meant.

> Keep at it though.
> Maybe something useful will be produced with more effort.

Unless there's more actually constructive criticism I don't think I'll
be getting any further with this.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 03:29 PM, Joe Perches wrote:
> This proposal has a very high false positive rate.

The only actual false positive you've pointed out is the one that is
just incorrect in the wrong way (the author should have used a KERN_CONT
but did not). I could easily do something similar to what Julia proposes
and produce a different warning if the string ends in a space. But I
don't see any bullet proof way for any script to deduce what the author
actually meant.

> Keep at it though.
> Maybe something useful will be produced with more effort.

Unless there's more actually constructive criticism I don't think I'll
be getting any further with this.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Joe Perches wrote:

> On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote:
> > My semantic patch and results are below.  The semantic patch has some
> > features that may or may not be desired:
> >
> > 1.  It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding
> > functions that are sometimes used with a format string ending with a
> > newline.  To reduce false positives, such a function is ignored if it is
> > sometimes used with a string that ends in a space.  This could lead to
> > false positives where actually one of the calls has a \n that it should
> > not have.
> >
> > 2.  Coccinelle puts multipart strings on a single line.  So the rule goes
> > a little further and eliminates the multipartness.  Basically "xxx " "yyy"
> > becomes "xxx yyy" regardless of the length of the result.
>
> What about the semi-common string concatenation "foo" #var "bar" ?

I don't think this is an issue.  There is no " " pattern in this.  It's
true that if the pieces were on separate lines, Coccinelle will now put
them on a single line.  I'm not sure I want to bother with this.

> > 3.  Some prints appear not to end with a newline because they end with \n.
> > where .\n was likely intended.  Instead of creating \n.\n, the semantic
> > patch just moves the .to the left of the . And if there was .\n. it just
> > drops the final period.
>
> That may be a problem if the sentence is "something...\n"

I think I was not clear.  The sentence ends in ".\n.".

> There seem to be many false positives in here too.

Could you point to something specifically?  I saw a lot of cases with
prints followed by returns and gotos.  I guess those are not likely false
positives.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Joe Perches wrote:

> On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote:
> > My semantic patch and results are below.  The semantic patch has some
> > features that may or may not be desired:
> >
> > 1.  It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding
> > functions that are sometimes used with a format string ending with a
> > newline.  To reduce false positives, such a function is ignored if it is
> > sometimes used with a string that ends in a space.  This could lead to
> > false positives where actually one of the calls has a \n that it should
> > not have.
> >
> > 2.  Coccinelle puts multipart strings on a single line.  So the rule goes
> > a little further and eliminates the multipartness.  Basically "xxx " "yyy"
> > becomes "xxx yyy" regardless of the length of the result.
>
> What about the semi-common string concatenation "foo" #var "bar" ?

I don't think this is an issue.  There is no " " pattern in this.  It's
true that if the pieces were on separate lines, Coccinelle will now put
them on a single line.  I'm not sure I want to bother with this.

> > 3.  Some prints appear not to end with a newline because they end with \n.
> > where .\n was likely intended.  Instead of creating \n.\n, the semantic
> > patch just moves the .to the left of the . And if there was .\n. it just
> > drops the final period.
>
> That may be a problem if the sentence is "something...\n"

I think I was not clear.  The sentence ends in ".\n.".

> There seem to be many false positives in here too.

Could you point to something specifically?  I saw a lot of cases with
prints followed by returns and gotos.  I guess those are not likely false
positives.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 26/11/17 03:29 PM, Joe Perches wrote:
> > This proposal has a very high false positive rate.
>
> The only actual false positive you've pointed out is the one that is
> just incorrect in the wrong way (the author should have used a KERN_CONT
> but did not). I could easily do something similar to what Julia proposes
> and produce a different warning if the string ends in a space.

I don't have a different warning if the string ends in a space.  I have a
different warning when one possible control-flow path is fine and another
control-flow path is not.  The space thing relates to guessing whether
some other printing API function needs a newline or not.

julia

> But I
> don't see any bullet proof way for any script to deduce what the author
> actually meant.
>
> > Keep at it though.
> > Maybe something useful will be produced with more effort.
>
> Unless there's more actually constructive criticism I don't think I'll
> be getting any further with this.
>
> Logan
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 26/11/17 03:29 PM, Joe Perches wrote:
> > This proposal has a very high false positive rate.
>
> The only actual false positive you've pointed out is the one that is
> just incorrect in the wrong way (the author should have used a KERN_CONT
> but did not). I could easily do something similar to what Julia proposes
> and produce a different warning if the string ends in a space.

I don't have a different warning if the string ends in a space.  I have a
different warning when one possible control-flow path is fine and another
control-flow path is not.  The space thing relates to guessing whether
some other printing API function needs a newline or not.

julia

> But I
> don't see any bullet proof way for any script to deduce what the author
> actually meant.
>
> > Keep at it though.
> > Maybe something useful will be produced with more effort.
>
> Unless there's more actually constructive criticism I don't think I'll
> be getting any further with this.
>
> Logan
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 11:11 PM, Julia Lawall wrote:
> I don't have a different warning if the string ends in a space.  I have a
> different warning when one possible control-flow path is fine and another
> control-flow path is not.  The space thing relates to guessing whether
> some other printing API function needs a newline or not.

Understood. For checkpatch, there only is warnings (of various types)
and I was referring to the guessing you mentioned. So if we see that a
call has no new line and the following one isn't a KERN_CONT, then based
on whether there's a space or not we potentially could have one of two
warnings:

WARNING: Log messages should end in a new line (\n)

or

WARNING: Given that your log message ends in a space and not a new line,
did you maybe mean to put a KERN_CONT or pr_cont somewhere in there?? By
the way, though, KERN_CONT use is discouraged and will create its own
warning when you fix it.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 11:11 PM, Julia Lawall wrote:
> I don't have a different warning if the string ends in a space.  I have a
> different warning when one possible control-flow path is fine and another
> control-flow path is not.  The space thing relates to guessing whether
> some other printing API function needs a newline or not.

Understood. For checkpatch, there only is warnings (of various types)
and I was referring to the guessing you mentioned. So if we see that a
call has no new line and the following one isn't a KERN_CONT, then based
on whether there's a space or not we potentially could have one of two
warnings:

WARNING: Log messages should end in a new line (\n)

or

WARNING: Given that your log message ends in a space and not a new line,
did you maybe mean to put a KERN_CONT or pr_cont somewhere in there?? By
the way, though, KERN_CONT use is discouraged and will create its own
warning when you fix it.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 26/11/17 11:11 PM, Julia Lawall wrote:
> > I don't have a different warning if the string ends in a space.  I have a
> > different warning when one possible control-flow path is fine and another
> > control-flow path is not.  The space thing relates to guessing whether
> > some other printing API function needs a newline or not.
>
> Understood. For checkpatch, there only is warnings (of various types)
> and I was referring to the guessing you mentioned. So if we see that a
> call has no new line and the following one isn't a KERN_CONT, then based
> on whether there's a space or not we potentially could have one of two
> warnings:
>
> WARNING: Log messages should end in a new line (\n)
>
> or
>
> WARNING: Given that your log message ends in a space and not a new line,
> did you maybe mean to put a KERN_CONT or pr_cont somewhere in there?? By
> the way, though, KERN_CONT use is discouraged and will create its own
> warning when you fix it.

It would probably be better not to mention the KERN_CONT possibility at
all.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 26/11/17 11:11 PM, Julia Lawall wrote:
> > I don't have a different warning if the string ends in a space.  I have a
> > different warning when one possible control-flow path is fine and another
> > control-flow path is not.  The space thing relates to guessing whether
> > some other printing API function needs a newline or not.
>
> Understood. For checkpatch, there only is warnings (of various types)
> and I was referring to the guessing you mentioned. So if we see that a
> call has no new line and the following one isn't a KERN_CONT, then based
> on whether there's a space or not we potentially could have one of two
> warnings:
>
> WARNING: Log messages should end in a new line (\n)
>
> or
>
> WARNING: Given that your log message ends in a space and not a new line,
> did you maybe mean to put a KERN_CONT or pr_cont somewhere in there?? By
> the way, though, KERN_CONT use is discouraged and will create its own
> warning when you fix it.

It would probably be better not to mention the KERN_CONT possibility at
all.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Joe Perches wrote:

> On Sun, 2017-11-26 at 19:17 +0100, Julia Lawall wrote:
> > I just assume that a printk that has no KERN_ is adding a
> > newline, which is my understanding of Joe's comment.
>
> More precisely:
>
> Any printk without an initial KERN_CONT prepends a newline
> if the last printk content char emitted that is not part
> of a printk timestamp/header was not a newline.

Ah, I misunderstood.  I thought it was any printk that has no KERN
indicator at all.  That I can fix.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Joe Perches wrote:

> On Sun, 2017-11-26 at 19:17 +0100, Julia Lawall wrote:
> > I just assume that a printk that has no KERN_ is adding a
> > newline, which is my understanding of Joe's comment.
>
> More precisely:
>
> Any printk without an initial KERN_CONT prepends a newline
> if the last printk content char emitted that is not part
> of a printk timestamp/header was not a newline.

Ah, I misunderstood.  I thought it was any printk that has no KERN
indicator at all.  That I can fix.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 11:34 PM, Julia Lawall wrote:
> It would probably be better not to mention the KERN_CONT possibility at
> all.

Oh? I don't disagree... but what are we supposed to do in these cases?
The way v2 of my patch works it just says that there is a missing new
line. But Joe calls that a false positive. So if we can't report that
it's missing a new line and we can't say it looks like it needs a
KERN_CONT, then what can we do? The case is obviously wrong in some way
or another so we probably shouldn't just ignore it.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 11:34 PM, Julia Lawall wrote:
> It would probably be better not to mention the KERN_CONT possibility at
> all.

Oh? I don't disagree... but what are we supposed to do in these cases?
The way v2 of my patch works it just says that there is a missing new
line. But Joe calls that a false positive. So if we can't report that
it's missing a new line and we can't say it looks like it needs a
KERN_CONT, then what can we do? The case is obviously wrong in some way
or another so we probably shouldn't just ignore it.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Mon, 27 Nov 2017, Julia Lawall wrote:

>
>
> On Sun, 26 Nov 2017, Joe Perches wrote:
>
> > On Sun, 2017-11-26 at 19:17 +0100, Julia Lawall wrote:
> > > I just assume that a printk that has no KERN_ is adding a
> > > newline, which is my understanding of Joe's comment.
> >
> > More precisely:
> >
> > Any printk without an initial KERN_CONT prepends a newline
> > if the last printk content char emitted that is not part
> > of a printk timestamp/header was not a newline.
>
> Ah, I misunderstood.  I thought it was any printk that has no KERN
> indicator at all.  That I can fix.

Although I guess that in that case the whole exercise is pointless?
Because every print will at runtime be followed by another print, which
will add either the newline or a continuation.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Mon, 27 Nov 2017, Julia Lawall wrote:

>
>
> On Sun, 26 Nov 2017, Joe Perches wrote:
>
> > On Sun, 2017-11-26 at 19:17 +0100, Julia Lawall wrote:
> > > I just assume that a printk that has no KERN_ is adding a
> > > newline, which is my understanding of Joe's comment.
> >
> > More precisely:
> >
> > Any printk without an initial KERN_CONT prepends a newline
> > if the last printk content char emitted that is not part
> > of a printk timestamp/header was not a newline.
>
> Ah, I misunderstood.  I thought it was any printk that has no KERN
> indicator at all.  That I can fix.

Although I guess that in that case the whole exercise is pointless?
Because every print will at runtime be followed by another print, which
will add either the newline or a continuation.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 11:42 PM, Julia Lawall wrote:
> Although I guess that in that case the whole exercise is pointless?
> Because every print will at runtime be followed by another print, which
> will add either the newline or a continuation.

Yes, in practice the '\n' at the end of every log line is optional based
on what the code actually does. Nothing bad happens if you omit one. But
reviewers still point out that they are required. (That's what started
me on this mess -- because I'd rather know what the correct thing is
before I commit the code for the first time, and not months after the
code reached mainline.)

The reviewers have a really good point though: if a significant fraction
of the log calls have no new line and a majority have them, then making
any kind of change in this area could break things. Not to mention the
ugliness of the inconsistencies everywhere. Also, the more cases that
are "wrong" that get into the kernel the more it confuses people trying
to learn what the "right" thing is.

Honestly, though, I have no dog in this race. I just thought it would be
useful.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 26/11/17 11:42 PM, Julia Lawall wrote:
> Although I guess that in that case the whole exercise is pointless?
> Because every print will at runtime be followed by another print, which
> will add either the newline or a continuation.

Yes, in practice the '\n' at the end of every log line is optional based
on what the code actually does. Nothing bad happens if you omit one. But
reviewers still point out that they are required. (That's what started
me on this mess -- because I'd rather know what the correct thing is
before I commit the code for the first time, and not months after the
code reached mainline.)

The reviewers have a really good point though: if a significant fraction
of the log calls have no new line and a majority have them, then making
any kind of change in this area could break things. Not to mention the
ugliness of the inconsistencies everywhere. Also, the more cases that
are "wrong" that get into the kernel the more it confuses people trying
to learn what the "right" thing is.

Honestly, though, I have no dog in this race. I just thought it would be
useful.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 26/11/17 11:42 PM, Julia Lawall wrote:
> > Although I guess that in that case the whole exercise is pointless?
> > Because every print will at runtime be followed by another print, which
> > will add either the newline or a continuation.
>
> Yes, in practice the '\n' at the end of every log line is optional based
> on what the code actually does. Nothing bad happens if you omit one. But
> reviewers still point out that they are required. (That's what started
> me on this mess -- because I'd rather know what the correct thing is
> before I commit the code for the first time, and not months after the
> code reached mainline.)
>
> The reviewers have a really good point though: if a significant fraction
> of the log calls have no new line and a majority have them, then making
> any kind of change in this area could break things. Not to mention the
> ugliness of the inconsistencies everywhere. Also, the more cases that
> are "wrong" that get into the kernel the more it confuses people trying
> to learn what the "right" thing is.

The problem is probably mostly for the non-standard functions, where a lot
of function/macro unfolding is often required to see what is really going
on.

julia


>
> Honestly, though, I have no dog in this race. I just thought it would be
> useful.
>
> Logan
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Sun, 26 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 26/11/17 11:42 PM, Julia Lawall wrote:
> > Although I guess that in that case the whole exercise is pointless?
> > Because every print will at runtime be followed by another print, which
> > will add either the newline or a continuation.
>
> Yes, in practice the '\n' at the end of every log line is optional based
> on what the code actually does. Nothing bad happens if you omit one. But
> reviewers still point out that they are required. (That's what started
> me on this mess -- because I'd rather know what the correct thing is
> before I commit the code for the first time, and not months after the
> code reached mainline.)
>
> The reviewers have a really good point though: if a significant fraction
> of the log calls have no new line and a majority have them, then making
> any kind of change in this area could break things. Not to mention the
> ugliness of the inconsistencies everywhere. Also, the more cases that
> are "wrong" that get into the kernel the more it confuses people trying
> to learn what the "right" thing is.

The problem is probably mostly for the non-standard functions, where a lot
of function/macro unfolding is often required to see what is really going
on.

julia


>
> Honestly, though, I have no dog in this race. I just thought it would be
> useful.
>
> Logan
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 23:40 -0700, Logan Gunthorpe wrote:
> 
> On 26/11/17 11:34 PM, Julia Lawall wrote:
> > It would probably be better not to mention the KERN_CONT possibility at
> > all.
> 
> Oh? I don't disagree... but what are we supposed to do in these cases?
> The way v2 of my patch works it just says that there is a missing new
> line. But Joe calls that a false positive. So if we can't report that
> it's missing a new line and we can't say it looks like it needs a
> KERN_CONT, then what can we do? The case is obviously wrong in some way
> or another so we probably shouldn't just ignore it.

checkpatch already reports printks without KERN_<level>

# printk should use KERN_* levels
		if ($line =~ /\bprintk\s*\(\s*(?!KERN_[A-Z]+\b)/) {
			WARN("PRINTK_WITHOUT_KERN_LEVEL",
			     "printk() should include KERN_<LEVEL> facility level\n" . $herecurr);
		}

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Sun, 2017-11-26 at 23:40 -0700, Logan Gunthorpe wrote:
> 
> On 26/11/17 11:34 PM, Julia Lawall wrote:
> > It would probably be better not to mention the KERN_CONT possibility at
> > all.
> 
> Oh? I don't disagree... but what are we supposed to do in these cases?
> The way v2 of my patch works it just says that there is a missing new
> line. But Joe calls that a false positive. So if we can't report that
> it's missing a new line and we can't say it looks like it needs a
> KERN_CONT, then what can we do? The case is obviously wrong in some way
> or another so we probably shouldn't just ignore it.

checkpatch already reports printks without KERN_<level>

# printk should use KERN_* levels
		if ($line =~ /\bprintk\s*\(\s*(?!KERN_[A-Z]+\b)/) {
			WARN("PRINTK_WITHOUT_KERN_LEVEL",
			     "printk() should include KERN_<LEVEL> facility level\n" . $herecurr);
		}

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Mon, 27 Nov 2017, Joe Perches wrote:

> On Sun, 2017-11-26 at 23:40 -0700, Logan Gunthorpe wrote:
> >
> > On 26/11/17 11:34 PM, Julia Lawall wrote:
> > > It would probably be better not to mention the KERN_CONT possibility at
> > > all.
> >
> > Oh? I don't disagree... but what are we supposed to do in these cases?
> > The way v2 of my patch works it just says that there is a missing new
> > line. But Joe calls that a false positive. So if we can't report that
> > it's missing a new line and we can't say it looks like it needs a
> > KERN_CONT, then what can we do? The case is obviously wrong in some way
> > or another so we probably shouldn't just ignore it.

I meant why not only suggest pr_cont?

julia

>
> checkpatch already reports printks without KERN_<level>
>
> # printk should use KERN_* levels
> 		if ($line =~ /\bprintk\s*\(\s*(?!KERN_[A-Z]+\b)/) {
> 			WARN("PRINTK_WITHOUT_KERN_LEVEL",
> 			     "printk() should include KERN_<LEVEL> facility level\n" . $herecurr);
> 		}
>
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Mon, 27 Nov 2017, Joe Perches wrote:

> On Sun, 2017-11-26 at 23:40 -0700, Logan Gunthorpe wrote:
> >
> > On 26/11/17 11:34 PM, Julia Lawall wrote:
> > > It would probably be better not to mention the KERN_CONT possibility at
> > > all.
> >
> > Oh? I don't disagree... but what are we supposed to do in these cases?
> > The way v2 of my patch works it just says that there is a missing new
> > line. But Joe calls that a false positive. So if we can't report that
> > it's missing a new line and we can't say it looks like it needs a
> > KERN_CONT, then what can we do? The case is obviously wrong in some way
> > or another so we probably shouldn't just ignore it.

I meant why not only suggest pr_cont?

julia

>
> checkpatch already reports printks without KERN_<level>
>
> # printk should use KERN_* levels
> 		if ($line =~ /\bprintk\s*\(\s*(?!KERN_[A-Z]+\b)/) {
> 			WARN("PRINTK_WITHOUT_KERN_LEVEL",
> 			     "printk() should include KERN_<LEVEL> facility level\n" . $herecurr);
> 		}
>
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 07:42 +0100, Julia Lawall wrote:
> On Mon, 27 Nov 2017, Julia Lawall wrote:
> > On Sun, 26 Nov 2017, Joe Perches wrote:
> > > On Sun, 2017-11-26 at 19:17 +0100, Julia Lawall wrote:
> > > > I just assume that a printk that has no KERN_ is adding a
> > > > newline, which is my understanding of Joe's comment.
> > > 
> > > More precisely:
> > > 
> > > Any printk without an initial KERN_CONT prepends a newline
> > > if the last printk content char emitted that is not part
> > > of a printk timestamp/header was not a newline.
> > 
> > Ah, I misunderstood.  I thought it was any printk that has no KERN
> > indicator at all.  That I can fix.
> 
> Although I guess that in that case the whole exercise is pointless?
> Because every print will at runtime be followed by another print, which
> will add either the newline or a continuation.

Kinda yes and no.

A printk without a newline termination is not emitted
as output until the next printk call.

This can cause issues on quiescent systems as the printk
is not emitted for potentially a very long time.

Also, any thread interleaving can still cause misformatted
output.

and:

All the historical printks without KERN_CONT worked well
until the commit that broke them by requiring KERN_CONT.

But now these consecutive calls to printk which used to be
emitted on on a single line are printed on multiple lines.

The title of the commit is wrong as KERN_CONT was not
necessary before this change.
---
commit 4bcc595ccd80decb4245096e3d1258989c50ed41
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat Oct 8 20:32:40 2016 -0700

printk: reinstate KERN_CONT for printing continuation lines
---

So IMO it's _somewhat_ useful to try to update the printks
without either
KERN_CONT or with a KERN_<LEVEL> but
without a newline.

As the above commit is about a year old, most of the cases
in the code that are actually likely have been fixed by now.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 07:42 +0100, Julia Lawall wrote:
> On Mon, 27 Nov 2017, Julia Lawall wrote:
> > On Sun, 26 Nov 2017, Joe Perches wrote:
> > > On Sun, 2017-11-26 at 19:17 +0100, Julia Lawall wrote:
> > > > I just assume that a printk that has no KERN_ is adding a
> > > > newline, which is my understanding of Joe's comment.
> > > 
> > > More precisely:
> > > 
> > > Any printk without an initial KERN_CONT prepends a newline
> > > if the last printk content char emitted that is not part
> > > of a printk timestamp/header was not a newline.
> > 
> > Ah, I misunderstood.  I thought it was any printk that has no KERN
> > indicator at all.  That I can fix.
> 
> Although I guess that in that case the whole exercise is pointless?
> Because every print will at runtime be followed by another print, which
> will add either the newline or a continuation.

Kinda yes and no.

A printk without a newline termination is not emitted
as output until the next printk call.

This can cause issues on quiescent systems as the printk
is not emitted for potentially a very long time.

Also, any thread interleaving can still cause misformatted
output.

and:

All the historical printks without KERN_CONT worked well
until the commit that broke them by requiring KERN_CONT.

But now these consecutive calls to printk which used to be
emitted on on a single line are printed on multiple lines.

The title of the commit is wrong as KERN_CONT was not
necessary before this change.
---
commit 4bcc595ccd80decb4245096e3d1258989c50ed41
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat Oct 8 20:32:40 2016 -0700

printk: reinstate KERN_CONT for printing continuation lines
---

So IMO it's _somewhat_ useful to try to update the printks
without either
KERN_CONT or with a KERN_<LEVEL> but
without a newline.

As the above commit is about a year old, most of the cases
in the code that are actually likely have been fixed by now.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 09:52 +0100, Julia Lawall wrote:
> On Mon, 27 Nov 2017, Joe Perches wrote:
> > On Sun, 2017-11-26 at 23:40 -0700, Logan Gunthorpe wrote:
> > > On 26/11/17 11:34 PM, Julia Lawall wrote:
> > > > It would probably be better not to mention the KERN_CONT possibility at
> > > > all.
> > > 
> > > Oh? I don't disagree... but what are we supposed to do in these cases?
> > > The way v2 of my patch works it just says that there is a missing new
> > > line. But Joe calls that a false positive. So if we can't report that
> > > it's missing a new line and we can't say it looks like it needs a
> > > KERN_CONT, then what can we do? The case is obviously wrong in some way
> > > or another so we probably shouldn't just ignore it.
> 
> I meant why not only suggest pr_cont?

Because checkpatch cannot know if the printk is missing a
KERN_CONT or another different KERN_<LEVEL>.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 09:52 +0100, Julia Lawall wrote:
> On Mon, 27 Nov 2017, Joe Perches wrote:
> > On Sun, 2017-11-26 at 23:40 -0700, Logan Gunthorpe wrote:
> > > On 26/11/17 11:34 PM, Julia Lawall wrote:
> > > > It would probably be better not to mention the KERN_CONT possibility at
> > > > all.
> > > 
> > > Oh? I don't disagree... but what are we supposed to do in these cases?
> > > The way v2 of my patch works it just says that there is a missing new
> > > line. But Joe calls that a false positive. So if we can't report that
> > > it's missing a new line and we can't say it looks like it needs a
> > > KERN_CONT, then what can we do? The case is obviously wrong in some way
> > > or another so we probably shouldn't just ignore it.
> 
> I meant why not only suggest pr_cont?

Because checkpatch cannot know if the printk is missing a
KERN_CONT or another different KERN_<LEVEL>.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 07:08 +0100, Julia Lawall wrote:
> 
> On Sun, 26 Nov 2017, Joe Perches wrote:
> 
> > On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote:
> > > My semantic patch and results are below.  The semantic patch has some
> > > features that may or may not be desired:
> > > 
> > > 1.  It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding
> > > functions that are sometimes used with a format string ending with a
> > > newline.  To reduce false positives, such a function is ignored if it is
> > > sometimes used with a string that ends in a space.  This could lead to
> > > false positives where actually one of the calls has a \n that it should
> > > not have.
> > > 
> > > 2.  Coccinelle puts multipart strings on a single line.  So the rule goes
> > > a little further and eliminates the multipartness.  Basically "xxx " "yyy"
> > > becomes "xxx yyy" regardless of the length of the result.
> > 
> > What about the semi-common string concatenation "foo" #var "bar" ?
> 
> I don't think this is an issue.  There is no " " pattern in this.  It's
> true that if the pieces were on separate lines, Coccinelle will now put
> them on a single line.  I'm not sure I want to bother with this.
> 
> > > 3.  Some prints appear not to end with a newline because they end with \n.
> > > where .\n was likely intended.  Instead of creating \n.\n, the semantic
> > > patch just moves the .to the left of the . And if there was .\n. it just
> > > drops the final period.
> > 
> > That may be a problem if the sentence is "something...\n"
> 
> I think I was not clear.  The sentence ends in ".\n.".
> 
> > There seem to be many false positives in here too.
> 
> Could you point to something specifically?  I saw a lot of cases with
> prints followed by returns and gotos.  I guess those are not likely false
> positives.

random entries, as your original post is 2.6M (and didn't get to lkml)
and I only sampled it at a few places.

[]
diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c
--- a/lib/locking-selftest.c
+++ b/lib/locking-selftest.c
@@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v

 static inline void print_testname(const char *testname)
 {
-	printk("%33s:", testname);
+	printk("%33s:\n", testname);
 }

[]

diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug *
 	vaf.fmt = fmt;
 	vaf.va = &args;

-	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
+	printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf),
+	       &vaf);

 	va_end(args);
 }

[]

diff -u -p a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c
--- a/drivers/tty/serial/ioc4_serial.c
+++ b/drivers/tty/serial/ioc4_serial.c
@@ -2858,8 +2858,7 @@ ioc4_serial_attach_one(struct ioc4_drive
 				"sgi-ioc4serial", soft)) {
 		control->ic_irq = idd->idd_pdev->irq;
 	} else {
-		printk(KERN_WARNING
-		    "%s : request_irq fails for IRQ 0x%x\n ",
+		printk(KERN_WARNING "%s : request_irq fails for IRQ 0x%x\n \n",
 			__func__, idd->idd_pdev->irq);
 	}
 	ret = ioc4_attach_local(idd);

[]

below: the gig_dbg macro and _many_ other append a newline to a format

diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
--- a/drivers/isdn/gigaset/ev-layer.c
+++ b/drivers/isdn/gigaset/ev-layer.c
@@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta
 	unsigned next, tail;
 	struct event_t *event;

-	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
+	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid);

 	spin_lock_irqsave(&cs->ev_lock, flags);

etc...

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 07:08 +0100, Julia Lawall wrote:
> 
> On Sun, 26 Nov 2017, Joe Perches wrote:
> 
> > On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote:
> > > My semantic patch and results are below.  The semantic patch has some
> > > features that may or may not be desired:
> > > 
> > > 1.  It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding
> > > functions that are sometimes used with a format string ending with a
> > > newline.  To reduce false positives, such a function is ignored if it is
> > > sometimes used with a string that ends in a space.  This could lead to
> > > false positives where actually one of the calls has a \n that it should
> > > not have.
> > > 
> > > 2.  Coccinelle puts multipart strings on a single line.  So the rule goes
> > > a little further and eliminates the multipartness.  Basically "xxx " "yyy"
> > > becomes "xxx yyy" regardless of the length of the result.
> > 
> > What about the semi-common string concatenation "foo" #var "bar" ?
> 
> I don't think this is an issue.  There is no " " pattern in this.  It's
> true that if the pieces were on separate lines, Coccinelle will now put
> them on a single line.  I'm not sure I want to bother with this.
> 
> > > 3.  Some prints appear not to end with a newline because they end with \n.
> > > where .\n was likely intended.  Instead of creating \n.\n, the semantic
> > > patch just moves the .to the left of the . And if there was .\n. it just
> > > drops the final period.
> > 
> > That may be a problem if the sentence is "something...\n"
> 
> I think I was not clear.  The sentence ends in ".\n.".
> 
> > There seem to be many false positives in here too.
> 
> Could you point to something specifically?  I saw a lot of cases with
> prints followed by returns and gotos.  I guess those are not likely false
> positives.

random entries, as your original post is 2.6M (and didn't get to lkml)
and I only sampled it at a few places.

[]
diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c
--- a/lib/locking-selftest.c
+++ b/lib/locking-selftest.c
@@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v

 static inline void print_testname(const char *testname)
 {
-	printk("%33s:", testname);
+	printk("%33s:\n", testname);
 }

[]

diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug *
 	vaf.fmt = fmt;
 	vaf.va = &args;

-	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
+	printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf),
+	       &vaf);

 	va_end(args);
 }

[]

diff -u -p a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c
--- a/drivers/tty/serial/ioc4_serial.c
+++ b/drivers/tty/serial/ioc4_serial.c
@@ -2858,8 +2858,7 @@ ioc4_serial_attach_one(struct ioc4_drive
 				"sgi-ioc4serial", soft)) {
 		control->ic_irq = idd->idd_pdev->irq;
 	} else {
-		printk(KERN_WARNING
-		    "%s : request_irq fails for IRQ 0x%x\n ",
+		printk(KERN_WARNING "%s : request_irq fails for IRQ 0x%x\n \n",
 			__func__, idd->idd_pdev->irq);
 	}
 	ret = ioc4_attach_local(idd);

[]

below: the gig_dbg macro and _many_ other append a newline to a format

diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
--- a/drivers/isdn/gigaset/ev-layer.c
+++ b/drivers/isdn/gigaset/ev-layer.c
@@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta
 	unsigned next, tail;
 	struct event_t *event;

-	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
+	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid);

 	spin_lock_irqsave(&cs->ev_lock, flags);

etc...

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Mon, 27 Nov 2017, Joe Perches wrote:

> On Mon, 2017-11-27 at 07:08 +0100, Julia Lawall wrote:
> >
> > On Sun, 26 Nov 2017, Joe Perches wrote:
> >
> > > On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote:
> > > > My semantic patch and results are below.  The semantic patch has some
> > > > features that may or may not be desired:
> > > >
> > > > 1.  It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding
> > > > functions that are sometimes used with a format string ending with a
> > > > newline.  To reduce false positives, such a function is ignored if it is
> > > > sometimes used with a string that ends in a space.  This could lead to
> > > > false positives where actually one of the calls has a \n that it should
> > > > not have.
> > > >
> > > > 2.  Coccinelle puts multipart strings on a single line.  So the rule goes
> > > > a little further and eliminates the multipartness.  Basically "xxx " "yyy"
> > > > becomes "xxx yyy" regardless of the length of the result.
> > >
> > > What about the semi-common string concatenation "foo" #var "bar" ?
> >
> > I don't think this is an issue.  There is no " " pattern in this.  It's
> > true that if the pieces were on separate lines, Coccinelle will now put
> > them on a single line.  I'm not sure I want to bother with this.
> >
> > > > 3.  Some prints appear not to end with a newline because they end with \n.
> > > > where .\n was likely intended.  Instead of creating \n.\n, the semantic
> > > > patch just moves the .to the left of the . And if there was .\n. it just
> > > > drops the final period.
> > >
> > > That may be a problem if the sentence is "something...\n"
> >
> > I think I was not clear.  The sentence ends in ".\n.".
> >
> > > There seem to be many false positives in here too.
> >
> > Could you point to something specifically?  I saw a lot of cases with
> > prints followed by returns and gotos.  I guess those are not likely false
> > positives.
>
> random entries, as your original post is 2.6M (and didn't get to lkml)
> and I only sampled it at a few places.
>
> []
> diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c
> --- a/lib/locking-selftest.c
> +++ b/lib/locking-selftest.c
> @@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v
>
>  static inline void print_testname(const char *testname)
>  {
> -	printk("%33s:", testname);
> +	printk("%33s:\n", testname);
>  }

Sure.

>
> []
>
> diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug *
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
>
> -	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
> +	printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf),
> +	       &vaf);
>
>  	va_end(args);
>  }

OK, one could look for va_end.

> []
>
> diff -u -p a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c
> --- a/drivers/tty/serial/ioc4_serial.c
> +++ b/drivers/tty/serial/ioc4_serial.c
> @@ -2858,8 +2858,7 @@ ioc4_serial_attach_one(struct ioc4_drive
>  				"sgi-ioc4serial", soft)) {
>  		control->ic_irq = idd->idd_pdev->irq;
>  	} else {
> -		printk(KERN_WARNING
> -		    "%s : request_irq fails for IRQ 0x%x\n ",
> +		printk(KERN_WARNING "%s : request_irq fails for IRQ 0x%x\n \n",
>  			__func__, idd->idd_pdev->irq);
>  	}
>  	ret = ioc4_attach_local(idd);

Like the \n. problem.

> []
>
> below: the gig_dbg macro and _many_ other append a newline to a format

Still, it seems that the file must contain a bug elsewhere, because the
presence of this report means that some other call ended in a newline.  It
could be reasonable to highlight the least popular.

thanks,
julia

> diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
> --- a/drivers/isdn/gigaset/ev-layer.c
> +++ b/drivers/isdn/gigaset/ev-layer.c
> @@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta
>  	unsigned next, tail;
>  	struct event_t *event;
>
> -	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
> +	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid);
>
>  	spin_lock_irqsave(&cs->ev_lock, flags);
>
> etc...
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Mon, 27 Nov 2017, Joe Perches wrote:

> On Mon, 2017-11-27 at 07:08 +0100, Julia Lawall wrote:
> >
> > On Sun, 26 Nov 2017, Joe Perches wrote:
> >
> > > On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote:
> > > > My semantic patch and results are below.  The semantic patch has some
> > > > features that may or may not be desired:
> > > >
> > > > 1.  It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding
> > > > functions that are sometimes used with a format string ending with a
> > > > newline.  To reduce false positives, such a function is ignored if it is
> > > > sometimes used with a string that ends in a space.  This could lead to
> > > > false positives where actually one of the calls has a \n that it should
> > > > not have.
> > > >
> > > > 2.  Coccinelle puts multipart strings on a single line.  So the rule goes
> > > > a little further and eliminates the multipartness.  Basically "xxx " "yyy"
> > > > becomes "xxx yyy" regardless of the length of the result.
> > >
> > > What about the semi-common string concatenation "foo" #var "bar" ?
> >
> > I don't think this is an issue.  There is no " " pattern in this.  It's
> > true that if the pieces were on separate lines, Coccinelle will now put
> > them on a single line.  I'm not sure I want to bother with this.
> >
> > > > 3.  Some prints appear not to end with a newline because they end with \n.
> > > > where .\n was likely intended.  Instead of creating \n.\n, the semantic
> > > > patch just moves the .to the left of the . And if there was .\n. it just
> > > > drops the final period.
> > >
> > > That may be a problem if the sentence is "something...\n"
> >
> > I think I was not clear.  The sentence ends in ".\n.".
> >
> > > There seem to be many false positives in here too.
> >
> > Could you point to something specifically?  I saw a lot of cases with
> > prints followed by returns and gotos.  I guess those are not likely false
> > positives.
>
> random entries, as your original post is 2.6M (and didn't get to lkml)
> and I only sampled it at a few places.
>
> []
> diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c
> --- a/lib/locking-selftest.c
> +++ b/lib/locking-selftest.c
> @@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v
>
>  static inline void print_testname(const char *testname)
>  {
> -	printk("%33s:", testname);
> +	printk("%33s:\n", testname);
>  }

Sure.

>
> []
>
> diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug *
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
>
> -	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
> +	printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf),
> +	       &vaf);
>
>  	va_end(args);
>  }

OK, one could look for va_end.

> []
>
> diff -u -p a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c
> --- a/drivers/tty/serial/ioc4_serial.c
> +++ b/drivers/tty/serial/ioc4_serial.c
> @@ -2858,8 +2858,7 @@ ioc4_serial_attach_one(struct ioc4_drive
>  				"sgi-ioc4serial", soft)) {
>  		control->ic_irq = idd->idd_pdev->irq;
>  	} else {
> -		printk(KERN_WARNING
> -		    "%s : request_irq fails for IRQ 0x%x\n ",
> +		printk(KERN_WARNING "%s : request_irq fails for IRQ 0x%x\n \n",
>  			__func__, idd->idd_pdev->irq);
>  	}
>  	ret = ioc4_attach_local(idd);

Like the \n. problem.

> []
>
> below: the gig_dbg macro and _many_ other append a newline to a format

Still, it seems that the file must contain a bug elsewhere, because the
presence of this report means that some other call ended in a newline.  It
could be reasonable to highlight the least popular.

thanks,
julia

> diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
> --- a/drivers/isdn/gigaset/ev-layer.c
> +++ b/drivers/isdn/gigaset/ev-layer.c
> @@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta
>  	unsigned next, tail;
>  	struct event_t *event;
>
> -	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
> +	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid);
>
>  	spin_lock_irqsave(&cs->ev_lock, flags);
>
> etc...
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:32 +0100, Julia Lawall wrote:
> On Mon, 27 Nov 2017, Joe Perches wrote:
[]
> > below: the gig_dbg macro and _many_ other append a newline to a format
> 
> Still, it seems that the file must contain a bug elsewhere, because the
> presence of this report means that some other call ended in a newline.  It
> could be reasonable to highlight the least popular.
> 
> thanks,
> julia
> 
> > diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
> > --- a/drivers/isdn/gigaset/ev-layer.c
> > +++ b/drivers/isdn/gigaset/ev-layer.c
> > @@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta
> >  	unsigned next, tail;
> >  	struct event_t *event;
> > 
> > -	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
> > +	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid);
> > 
> >  	spin_lock_irqsave(&cs->ev_lock, flags);
> > 
> > etc...

Sure, but more than anything, this shows the difficulty
of automating these changes.

Variability of style is high and highlighting was might
be defective produces a lot of noise.

Perhaps the S/N ratio of your script is somewhat better
than the other proposal.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:32 +0100, Julia Lawall wrote:
> On Mon, 27 Nov 2017, Joe Perches wrote:
[]
> > below: the gig_dbg macro and _many_ other append a newline to a format
> 
> Still, it seems that the file must contain a bug elsewhere, because the
> presence of this report means that some other call ended in a newline.  It
> could be reasonable to highlight the least popular.
> 
> thanks,
> julia
> 
> > diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
> > --- a/drivers/isdn/gigaset/ev-layer.c
> > +++ b/drivers/isdn/gigaset/ev-layer.c
> > @@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta
> >  	unsigned next, tail;
> >  	struct event_t *event;
> > 
> > -	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
> > +	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid);
> > 
> >  	spin_lock_irqsave(&cs->ev_lock, flags);
> > 
> > etc...

Sure, but more than anything, this shows the difficulty
of automating these changes.

Variability of style is high and highlighting was might
be defective produces a lot of noise.

Perhaps the S/N ratio of your script is somewhat better
than the other proposal.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 01:52 AM, Julia Lawall wrote:
> I meant why not only suggest pr_cont?

You could. Not much difference though, both are discouraged.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 01:52 AM, Julia Lawall wrote:
> I meant why not only suggest pr_cont?

You could. Not much difference though, both are discouraged.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 02:25 AM, Joe Perches wrote:
> []
> diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c
> --- a/lib/locking-selftest.c
> +++ b/lib/locking-selftest.c
> @@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v
> 
>   static inline void print_testname(const char *testname)
>   {
> -	printk("%33s:", testname);
> +	printk("%33s:\n", testname);
>   }

Coincidentally, the checkpatch script does not hit this false positive. 
This because a pr_cont actually follows in a #define. It doesn't feel 
right but checkpatch does do the correct thing on this one.

> diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug *
>   	vaf.fmt = fmt;
>   	vaf.va = &args;
> 
> -	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
> +	printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf),
> +	       &vaf);
> 
>   	va_end(args);
>   }

This is a valid false positive that I also missed. However, it can 
actually be very easily ignored by checking if the format string ends in 
%pV. There were about 100 cases in my results that match this.

> 
> diff -u -p a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c
> --- a/drivers/tty/serial/ioc4_serial.c
> +++ b/drivers/tty/serial/ioc4_serial.c
> @@ -2858,8 +2858,7 @@ ioc4_serial_attach_one(struct ioc4_drive
>   				"sgi-ioc4serial", soft)) {
>   		control->ic_irq = idd->idd_pdev->irq;
>   	} else {
> -		printk(KERN_WARNING
> -		    "%s : request_irq fails for IRQ 0x%x\n ",
> +		printk(KERN_WARNING "%s : request_irq fails for IRQ 0x%x\n \n",
>   			__func__, idd->idd_pdev->irq);
>   	}
>   	ret = ioc4_attach_local(idd);
> 
> []

This isn't an issue for the checkpatch script as it doesn't try to fix 
it, it only warns that it is wrong (as it is actually wrong).


> diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
> --- a/drivers/isdn/gigaset/ev-layer.c
> +++ b/drivers/isdn/gigaset/ev-layer.c
> @@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta
>   	unsigned next, tail;
>   	struct event_t *event;
> 
> -	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
> +	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid);
> 
>   	spin_lock_irqsave(&cs->ev_lock, flags);

This is why I elected to only look at printk, pr_xxx, and dev_xxx in v2 
of my patch. So this is another false positive the checkpatch script 
would not hit.


Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 02:25 AM, Joe Perches wrote:
> []
> diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c
> --- a/lib/locking-selftest.c
> +++ b/lib/locking-selftest.c
> @@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v
> 
>   static inline void print_testname(const char *testname)
>   {
> -	printk("%33s:", testname);
> +	printk("%33s:\n", testname);
>   }

Coincidentally, the checkpatch script does not hit this false positive. 
This because a pr_cont actually follows in a #define. It doesn't feel 
right but checkpatch does do the correct thing on this one.

> diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug *
>   	vaf.fmt = fmt;
>   	vaf.va = &args;
> 
> -	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
> +	printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf),
> +	       &vaf);
> 
>   	va_end(args);
>   }

This is a valid false positive that I also missed. However, it can 
actually be very easily ignored by checking if the format string ends in 
%pV. There were about 100 cases in my results that match this.

> 
> diff -u -p a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c
> --- a/drivers/tty/serial/ioc4_serial.c
> +++ b/drivers/tty/serial/ioc4_serial.c
> @@ -2858,8 +2858,7 @@ ioc4_serial_attach_one(struct ioc4_drive
>   				"sgi-ioc4serial", soft)) {
>   		control->ic_irq = idd->idd_pdev->irq;
>   	} else {
> -		printk(KERN_WARNING
> -		    "%s : request_irq fails for IRQ 0x%x\n ",
> +		printk(KERN_WARNING "%s : request_irq fails for IRQ 0x%x\n \n",
>   			__func__, idd->idd_pdev->irq);
>   	}
>   	ret = ioc4_attach_local(idd);
> 
> []

This isn't an issue for the checkpatch script as it doesn't try to fix 
it, it only warns that it is wrong (as it is actually wrong).


> diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
> --- a/drivers/isdn/gigaset/ev-layer.c
> +++ b/drivers/isdn/gigaset/ev-layer.c
> @@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta
>   	unsigned next, tail;
>   	struct event_t *event;
> 
> -	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
> +	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid);
> 
>   	spin_lock_irqsave(&cs->ev_lock, flags);

This is why I elected to only look at printk, pr_xxx, and dev_xxx in v2 
of my patch. So this is another false positive the checkpatch script 
would not hit.


Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 01:28 AM, Joe Perches wrote:
> checkpatch already reports printks without KERN_<level>
> 
> # printk should use KERN_* levels
> 		if ($line =~ /\bprintk\s*\(\s*(?!KERN_[A-Z]+\b)/) {
> 			WARN("PRINTK_WITHOUT_KERN_LEVEL",
> 			     "printk() should include KERN_<LEVEL> facility level\n" . $herecurr);
> 		}
> 

Yes, but that kind of misses the mark in a similar way a new line 
warning misses the mark. Consider:

printk("blahblah: ");
printk("blah\n");

Check patch will report that both lines are missing a KERN_ level, but 
actually the second line is meant to be a continuation. So someone fixes 
it, naively:

printk(KERN_INFO, "blahblah: ");
printk(KERN_INFO, "blah\n");

Now, checkpatch will not warn on either and it looks like they fixed it, 
even though it's pretty clear that it's not correct either way. With, my 
patch, it will report a missing new line on the first line. If someone 
looked at it, they may realize it's actually missing a KERN_CONT. If we 
use the space heuristic, in this case, the warning might also suggest it 
may be missing a _cont. But the huristic isn't great... maybe the author 
actually meant:

printk(KERN_INFO, "blahblah:\n");
printk(KERN_INFO, "   blah\n");

But I wouldn't say a warning in this case is a bad thing because it 
forces someone to look at something that is obviously wrong in some way.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 01:28 AM, Joe Perches wrote:
> checkpatch already reports printks without KERN_<level>
> 
> # printk should use KERN_* levels
> 		if ($line =~ /\bprintk\s*\(\s*(?!KERN_[A-Z]+\b)/) {
> 			WARN("PRINTK_WITHOUT_KERN_LEVEL",
> 			     "printk() should include KERN_<LEVEL> facility level\n" . $herecurr);
> 		}
> 

Yes, but that kind of misses the mark in a similar way a new line 
warning misses the mark. Consider:

printk("blahblah: ");
printk("blah\n");

Check patch will report that both lines are missing a KERN_ level, but 
actually the second line is meant to be a continuation. So someone fixes 
it, naively:

printk(KERN_INFO, "blahblah: ");
printk(KERN_INFO, "blah\n");

Now, checkpatch will not warn on either and it looks like they fixed it, 
even though it's pretty clear that it's not correct either way. With, my 
patch, it will report a missing new line on the first line. If someone 
looked at it, they may realize it's actually missing a KERN_CONT. If we 
use the space heuristic, in this case, the warning might also suggest it 
may be missing a _cont. But the huristic isn't great... maybe the author 
actually meant:

printk(KERN_INFO, "blahblah:\n");
printk(KERN_INFO, "   blah\n");

But I wouldn't say a warning in this case is a bad thing because it 
forces someone to look at something that is obviously wrong in some way.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:07 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 02:25 AM, Joe Perches wrote:
> > []
> > diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c
> > --- a/lib/locking-selftest.c
> > +++ b/lib/locking-selftest.c
> > @@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v
> > 
> >   static inline void print_testname(const char *testname)
> >   {
> > -	printk("%33s:", testname);
> > +	printk("%33s:\n", testname);
> >   }
> 
> Coincidentally, the checkpatch script does not hit this false positive. 
> This because a pr_cont actually follows in a #define. It doesn't feel 
> right but checkpatch does do the correct thing on this one.
> 
> > diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug *
> >   	vaf.fmt = fmt;
> >   	vaf.va = &args;
> > 
> > -	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
> > +	printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf),
> > +	       &vaf);
> > 
> >   	va_end(args);
> >   }
> 
> This is a valid false positive that I also missed. However, it can 
> actually be very easily ignored by checking if the format string ends in 
> %pV. There were about 100 cases in my results that match this.

No, it can't be done that way.

$ git grep '%pV\\n"' | wc -l
56
$ git grep '%pV"' | wc -l
146

AFAIK: all of the above are correct as-is.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:07 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 02:25 AM, Joe Perches wrote:
> > []
> > diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c
> > --- a/lib/locking-selftest.c
> > +++ b/lib/locking-selftest.c
> > @@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v
> > 
> >   static inline void print_testname(const char *testname)
> >   {
> > -	printk("%33s:", testname);
> > +	printk("%33s:\n", testname);
> >   }
> 
> Coincidentally, the checkpatch script does not hit this false positive. 
> This because a pr_cont actually follows in a #define. It doesn't feel 
> right but checkpatch does do the correct thing on this one.
> 
> > diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug *
> >   	vaf.fmt = fmt;
> >   	vaf.va = &args;
> > 
> > -	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
> > +	printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf),
> > +	       &vaf);
> > 
> >   	va_end(args);
> >   }
> 
> This is a valid false positive that I also missed. However, it can 
> actually be very easily ignored by checking if the format string ends in 
> %pV. There were about 100 cases in my results that match this.

No, it can't be done that way.

$ git grep '%pV\\n"' | wc -l
56
$ git grep '%pV"' | wc -l
146

AFAIK: all of the above are correct as-is.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:20 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 01:28 AM, Joe Perches wrote:
> > checkpatch already reports printks without KERN_<level>
> > 
> > # printk should use KERN_* levels
> > 		if ($line =~ /\bprintk\s*\(\s*(?!KERN_[A-Z]+\b)/) {
> > 			WARN("PRINTK_WITHOUT_KERN_LEVEL",
> > 			     "printk() should include KERN_<LEVEL> facility level\n" . $herecurr);
> > 		}
> > 
> 
> Yes, but that kind of misses the mark in a similar way a new line 
> warning misses the mark. Consider:

It doesn't miss any mark.  It simply informs.
Correctness when fixing are a different world of problems.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:20 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 01:28 AM, Joe Perches wrote:
> > checkpatch already reports printks without KERN_<level>
> > 
> > # printk should use KERN_* levels
> > 		if ($line =~ /\bprintk\s*\(\s*(?!KERN_[A-Z]+\b)/) {
> > 			WARN("PRINTK_WITHOUT_KERN_LEVEL",
> > 			     "printk() should include KERN_<LEVEL> facility level\n" . $herecurr);
> > 		}
> > 
> 
> Yes, but that kind of misses the mark in a similar way a new line 
> warning misses the mark. Consider:

It doesn't miss any mark.  It simply informs.
Correctness when fixing are a different world of problems.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

>> This is a valid false positive that I also missed. However, it can
>> actually be very easily ignored by checking if the format string ends in
>> %pV. There were about 100 cases in my results that match this.
> 
> No, it can't be done that way.
> 
> $ git grep '%pV\\n"' | wc -l
> 56
> $ git grep '%pV"' | wc -l
> 146
> 
> AFAIK: all of the above are correct as-is.

Yes, I'm saying they are correct too. So the script would very easily 
notice this and not produce a warning. The first grep you did already 
weren't false positives because they ended in \\n and wouldn't have 
produced a warning. A very simple change to my patch ignores the second 
group. So what's wrong here?

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

>> This is a valid false positive that I also missed. However, it can
>> actually be very easily ignored by checking if the format string ends in
>> %pV. There were about 100 cases in my results that match this.
> 
> No, it can't be done that way.
> 
> $ git grep '%pV\\n"' | wc -l
> 56
> $ git grep '%pV"' | wc -l
> 146
> 
> AFAIK: all of the above are correct as-is.

Yes, I'm saying they are correct too. So the script would very easily 
notice this and not produce a warning. The first grep you did already 
weren't false positives because they ended in \\n and wouldn't have 
produced a warning. A very simple change to my patch ignores the second 
group. So what's wrong here?

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 10:28 AM, Joe Perches wrote:
> It doesn't miss any mark.  It simply informs.
> Correctness when fixing are a different world of problems.

So how come the double standard? Having a missing new line warning 
simply informs here too. In fact, if you read my example, it informs in 
broken places where the KERN_LEVEL warning does not.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 10:28 AM, Joe Perches wrote:
> It doesn't miss any mark.  It simply informs.
> Correctness when fixing are a different world of problems.

So how come the double standard? Having a missing new line warning 
simply informs here too. In fact, if you read my example, it informs in 
broken places where the KERN_LEVEL warning does not.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:33 -0700, Logan Gunthorpe wrote:
> > > This is a valid false positive that I also missed. However, it can
> > > actually be very easily ignored by checking if the format string ends in
> > > %pV. There were about 100 cases in my results that match this.
> > 
> > No, it can't be done that way.
> > 
> > $ git grep '%pV\\n"' | wc -l
> > 56
> > $ git grep '%pV"' | wc -l
> > 146
> > 
> > AFAIK: all of the above are correct as-is.
> 
> Yes, I'm saying they are correct too. So the script would very easily 
> notice this and not produce a warning. The first grep you did already 
> weren't false positives because they ended in \\n and wouldn't have 
> produced a warning. A very simple change to my patch ignores the second 
> group. So what's wrong here?

It can't be determined if the vaf.fmt contains a \n termination.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:33 -0700, Logan Gunthorpe wrote:
> > > This is a valid false positive that I also missed. However, it can
> > > actually be very easily ignored by checking if the format string ends in
> > > %pV. There were about 100 cases in my results that match this.
> > 
> > No, it can't be done that way.
> > 
> > $ git grep '%pV\\n"' | wc -l
> > 56
> > $ git grep '%pV"' | wc -l
> > 146
> > 
> > AFAIK: all of the above are correct as-is.
> 
> Yes, I'm saying they are correct too. So the script would very easily 
> notice this and not produce a warning. The first grep you did already 
> weren't false positives because they ended in \\n and wouldn't have 
> produced a warning. A very simple change to my patch ignores the second 
> group. So what's wrong here?

It can't be determined if the vaf.fmt contains a \n termination.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:35 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 10:28 AM, Joe Perches wrote:
> > It doesn't miss any mark.  It simply informs.
> > Correctness when fixing are a different world of problems.
> 
> So how come the double standard?

No double stardard.  One correctly informs that a bare
printk is not acceptable.

>  Having a missing new line warning 
> simply informs here too.

No it has a high false positive rate and newbies
act on that by submitting bad patches.'

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:35 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 10:28 AM, Joe Perches wrote:
> > It doesn't miss any mark.  It simply informs.
> > Correctness when fixing are a different world of problems.
> 
> So how come the double standard?

No double stardard.  One correctly informs that a bare
printk is not acceptable.

>  Having a missing new line warning 
> simply informs here too.

No it has a high false positive rate and newbies
act on that by submitting bad patches.'

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 10:41 AM, Joe Perches wrote:
> It can't be determined if the vaf.fmt contains a \n termination.

Yes, that's why we aren't warning. This could potentially be a false 
negative which you haven't said is a problem up until now. If we have to 
worry about false negatives too then your standards of perfection are 
impossible.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 10:41 AM, Joe Perches wrote:
> It can't be determined if the vaf.fmt contains a \n termination.

Yes, that's why we aren't warning. This could potentially be a false 
negative which you haven't said is a problem up until now. If we have to 
worry about false negatives too then your standards of perfection are 
impossible.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 10:42 AM, Joe Perches wrote:
> No double stardard.  One correctly informs that a bare
> printk is not acceptable.

The other correctly informs that a printk that isn't followed by a 
pr_cont or KERN_CONT is not correct.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 10:42 AM, Joe Perches wrote:
> No double stardard.  One correctly informs that a bare
> printk is not acceptable.

The other correctly informs that a printk that isn't followed by a 
pr_cont or KERN_CONT is not correct.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:44 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 10:42 AM, Joe Perches wrote:
> > No double stardard.  One correctly informs that a bare
> > printk is not acceptable.
> 
> The other correctly informs that a printk that isn't followed by a 
> pr_cont or KERN_CONT is not correct.

It may or not be correct.

Without inter-function call code flow analysis,
it's not possible to be correct.

If you can get the false positive & false negative
rate higher, I'll listen.

I think the Coccinelle script has a better chance
to be more correct.

cheers, Joe

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 10:44 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 10:42 AM, Joe Perches wrote:
> > No double stardard.  One correctly informs that a bare
> > printk is not acceptable.
> 
> The other correctly informs that a printk that isn't followed by a 
> pr_cont or KERN_CONT is not correct.

It may or not be correct.

Without inter-function call code flow analysis,
it's not possible to be correct.

If you can get the false positive & false negative
rate higher, I'll listen.

I think the Coccinelle script has a better chance
to be more correct.

cheers, Joe

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 11:57 AM, Joe Perches wrote:
> It may or not be correct.

It's absolutely not correct in that it either requires that a subsequent 
KERN_CONT/pr_cont or a '\n' at the end and it has neither.

> Without inter-function call code flow analysis,
> it's not possible to be correct.

But how many cases actually have the pr_cont/KERN_cont called in 
different functions? This appears to be exceedingly rare to me.

> If you can get the false positive & false negative
> rate higher, I'll listen.

The only two classes of false positives that you've pointed out or that 
I'm aware of:

1) The case where call did not either end in a '\n' or have a 
KERN_CONT/pr_cont in a subsequent call. I've been arguing (to deaf ears) 
that a warning is appropriate here and this is not a false positive 
because it absolutely is incorrect one way or the other. Coccinnelle 
will also suffer from this issue because it can no better decide whether 
the developer intended for the next call to be a continuation or for a 
'\n' to end the line.

2) Cases where the pr_cont/KERN_CONT is not in sufficient context for 
the script to detect. These are impossible to fix (and it's likely also 
impossible for Coccinelle to be 100% accurate here). However, I'd expect 
these to be *very* rare and I'm only actually aware of one case where 
this has actually happened (lib/locking-selftest.c:1189) and (mostly by 
luck) my v2 patch does not flag this where Coccinelle did. Not to 
mention that continuation usage is discouraged in new code so this 
should be even rarer on the majority of what checkpatch is used for.

(also 3. would be the %pV case, but I've removed those in what could be 
a v3 of the patch -- I'd also be happy to address other false positives 
classes if I could find them)

False negatives are much harder to quantify or improve. But given that I 
detect nearly 6000 errors in the existing kernel it can't be *that* 
high. Also, these false negatives do nothing to negate the benefit of 
having this functionality seeing the vast majority of developers are 
doing simple things with pr_* and dev_*.

Coccinelle may very well be able to do better at false negatives. But in 
this case, it would still be great to have both because checkpatch will 
flag a significant subset of the errors much earlier in the development 
cycle and save developers a bunch of time.

So, in my opinion, I think focusing too hard on the false negatives 
deprives developers of what could be a useful check.

> I think the Coccinelle script has a better chance
> to be more correct.

And yet, you have not pointed out any false positives that my patch 
gives which Coccinelle does/would not. It really feels to me like your 
biases are guiding your decision here and you aren't really looking at 
the results.

Another thought I've had is that the dev_ functions don't have any form 
of continuation. So we could potentially limit checkpatch to looking for 
those to avoid the issues with continuations. It's not high coverage but 
at least a lot of the driver patches would be checked with no chance of 
false positives. I think there would be value in doing that.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 11:57 AM, Joe Perches wrote:
> It may or not be correct.

It's absolutely not correct in that it either requires that a subsequent 
KERN_CONT/pr_cont or a '\n' at the end and it has neither.

> Without inter-function call code flow analysis,
> it's not possible to be correct.

But how many cases actually have the pr_cont/KERN_cont called in 
different functions? This appears to be exceedingly rare to me.

> If you can get the false positive & false negative
> rate higher, I'll listen.

The only two classes of false positives that you've pointed out or that 
I'm aware of:

1) The case where call did not either end in a '\n' or have a 
KERN_CONT/pr_cont in a subsequent call. I've been arguing (to deaf ears) 
that a warning is appropriate here and this is not a false positive 
because it absolutely is incorrect one way or the other. Coccinnelle 
will also suffer from this issue because it can no better decide whether 
the developer intended for the next call to be a continuation or for a 
'\n' to end the line.

2) Cases where the pr_cont/KERN_CONT is not in sufficient context for 
the script to detect. These are impossible to fix (and it's likely also 
impossible for Coccinelle to be 100% accurate here). However, I'd expect 
these to be *very* rare and I'm only actually aware of one case where 
this has actually happened (lib/locking-selftest.c:1189) and (mostly by 
luck) my v2 patch does not flag this where Coccinelle did. Not to 
mention that continuation usage is discouraged in new code so this 
should be even rarer on the majority of what checkpatch is used for.

(also 3. would be the %pV case, but I've removed those in what could be 
a v3 of the patch -- I'd also be happy to address other false positives 
classes if I could find them)

False negatives are much harder to quantify or improve. But given that I 
detect nearly 6000 errors in the existing kernel it can't be *that* 
high. Also, these false negatives do nothing to negate the benefit of 
having this functionality seeing the vast majority of developers are 
doing simple things with pr_* and dev_*.

Coccinelle may very well be able to do better at false negatives. But in 
this case, it would still be great to have both because checkpatch will 
flag a significant subset of the errors much earlier in the development 
cycle and save developers a bunch of time.

So, in my opinion, I think focusing too hard on the false negatives 
deprives developers of what could be a useful check.

> I think the Coccinelle script has a better chance
> to be more correct.

And yet, you have not pointed out any false positives that my patch 
gives which Coccinelle does/would not. It really feels to me like your 
biases are guiding your decision here and you aren't really looking at 
the results.

Another thought I've had is that the dev_ functions don't have any form 
of continuation. So we could potentially limit checkpatch to looking for 
those to avoid the issues with continuations. It's not high coverage but 
at least a lot of the driver patches would be checked with no chance of 
false positives. I think there would be value in doing that.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Mon, 27 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 27/11/17 11:57 AM, Joe Perches wrote:
> > It may or not be correct.
>
> It's absolutely not correct in that it either requires that a subsequent
> KERN_CONT/pr_cont or a '\n' at the end and it has neither.
>
> > Without inter-function call code flow analysis,
> > it's not possible to be correct.
>
> But how many cases actually have the pr_cont/KERN_cont called in different
> functions? This appears to be exceedingly rare to me.
>
> > If you can get the false positive & false negative
> > rate higher, I'll listen.
>
> The only two classes of false positives that you've pointed out or that I'm
> aware of:
>
> 1) The case where call did not either end in a '\n' or have a
> KERN_CONT/pr_cont in a subsequent call. I've been arguing (to deaf ears) that
> a warning is appropriate here and this is not a false positive because it
> absolutely is incorrect one way or the other. Coccinnelle will also suffer
> from this issue because it can no better decide whether the developer intended
> for the next call to be a continuation or for a '\n' to end the line.
>
> 2) Cases where the pr_cont/KERN_CONT is not in sufficient context for the
> script to detect. These are impossible to fix (and it's likely also impossible
> for Coccinelle to be 100% accurate here). However, I'd expect these to be
> *very* rare and I'm only actually aware of one case where this has actually
> happened (lib/locking-selftest.c:1189) and (mostly by luck) my v2 patch does
> not flag this where Coccinelle did. Not to mention that continuation usage is
> discouraged in new code so this should be even rarer on the majority of what
> checkpatch is used for.
>
> (also 3. would be the %pV case, but I've removed those in what could be a v3
> of the patch -- I'd also be happy to address other false positives classes if
> I could find them)
>
> False negatives are much harder to quantify or improve. But given that I
> detect nearly 6000 errors in the existing kernel it can't be *that* high.
> Also, these false negatives do nothing to negate the benefit of having this
> functionality seeing the vast majority of developers are doing simple things
> with pr_* and dev_*.
>
> Coccinelle may very well be able to do better at false negatives. But in this
> case, it would still be great to have both because checkpatch will flag a
> significant subset of the errors much earlier in the development cycle and
> save developers a bunch of time.
>
> So, in my opinion, I think focusing too hard on the false negatives deprives
> developers of what could be a useful check.
>
> > I think the Coccinelle script has a better chance
> > to be more correct.
>
> And yet, you have not pointed out any false positives that my patch gives
> which Coccinelle does/would not. It really feels to me like your biases are
> guiding your decision here and you aren't really looking at the results.
>
> Another thought I've had is that the dev_ functions don't have any form of
> continuation. So we could potentially limit checkpatch to looking for those to
> avoid the issues with continuations. It's not high coverage but at least a lot
> of the driver patches would be checked with no chance of false positives. I
> think there would be value in doing that.

Perhaps if there is a possible flow from one print to another within a
single function and in both cases the format string is at least say 25
characters (completely random value), then it is pretty likely that a
newline is intended.

Alternatively, if the first format string doesn't end in a space and the
second one doesn't begin with a space, then a newline is also likely
intended.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Julia Lawall]

On Mon, 27 Nov 2017, Logan Gunthorpe wrote:

>
>
> On 27/11/17 11:57 AM, Joe Perches wrote:
> > It may or not be correct.
>
> It's absolutely not correct in that it either requires that a subsequent
> KERN_CONT/pr_cont or a '\n' at the end and it has neither.
>
> > Without inter-function call code flow analysis,
> > it's not possible to be correct.
>
> But how many cases actually have the pr_cont/KERN_cont called in different
> functions? This appears to be exceedingly rare to me.
>
> > If you can get the false positive & false negative
> > rate higher, I'll listen.
>
> The only two classes of false positives that you've pointed out or that I'm
> aware of:
>
> 1) The case where call did not either end in a '\n' or have a
> KERN_CONT/pr_cont in a subsequent call. I've been arguing (to deaf ears) that
> a warning is appropriate here and this is not a false positive because it
> absolutely is incorrect one way or the other. Coccinnelle will also suffer
> from this issue because it can no better decide whether the developer intended
> for the next call to be a continuation or for a '\n' to end the line.
>
> 2) Cases where the pr_cont/KERN_CONT is not in sufficient context for the
> script to detect. These are impossible to fix (and it's likely also impossible
> for Coccinelle to be 100% accurate here). However, I'd expect these to be
> *very* rare and I'm only actually aware of one case where this has actually
> happened (lib/locking-selftest.c:1189) and (mostly by luck) my v2 patch does
> not flag this where Coccinelle did. Not to mention that continuation usage is
> discouraged in new code so this should be even rarer on the majority of what
> checkpatch is used for.
>
> (also 3. would be the %pV case, but I've removed those in what could be a v3
> of the patch -- I'd also be happy to address other false positives classes if
> I could find them)
>
> False negatives are much harder to quantify or improve. But given that I
> detect nearly 6000 errors in the existing kernel it can't be *that* high.
> Also, these false negatives do nothing to negate the benefit of having this
> functionality seeing the vast majority of developers are doing simple things
> with pr_* and dev_*.
>
> Coccinelle may very well be able to do better at false negatives. But in this
> case, it would still be great to have both because checkpatch will flag a
> significant subset of the errors much earlier in the development cycle and
> save developers a bunch of time.
>
> So, in my opinion, I think focusing too hard on the false negatives deprives
> developers of what could be a useful check.
>
> > I think the Coccinelle script has a better chance
> > to be more correct.
>
> And yet, you have not pointed out any false positives that my patch gives
> which Coccinelle does/would not. It really feels to me like your biases are
> guiding your decision here and you aren't really looking at the results.
>
> Another thought I've had is that the dev_ functions don't have any form of
> continuation. So we could potentially limit checkpatch to looking for those to
> avoid the issues with continuations. It's not high coverage but at least a lot
> of the driver patches would be checked with no chance of false positives. I
> think there would be value in doing that.

Perhaps if there is a possible flow from one print to another within a
single function and in both cases the format string is at least say 25
characters (completely random value), then it is pretty likely that a
newline is intended.

Alternatively, if the first format string doesn't end in a space and the
second one doesn't begin with a space, then a newline is also likely
intended.

julia

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 01:49 PM, Julia Lawall wrote:
> Perhaps if there is a possible flow from one print to another within a
> single function and in both cases the format string is at least say 25
> characters (completely random value), then it is pretty likely that a
> newline is intended.

This is on the edge of what I think could be done with checkpatch. I did 
a quick look through some of the cases I've noticed and haven't found 
any that this wouldn't pass. I'm not sure how you'd pick an appropriate 
threshold though. I do think it's quite clever, though.

> Alternatively, if the first format string doesn't end in a space and the
> second one doesn't begin with a space, then a newline is also likely
> intended.

This makes more sense to me but I don't expect it to be 100% accurate 
either. See, for example, drivers/char/dtlk.c:640 which has no space in 
either printk. You could maybe expand the rule to include ':' and '(' as 
the last character.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Logan Gunthorpe]

On 27/11/17 01:49 PM, Julia Lawall wrote:
> Perhaps if there is a possible flow from one print to another within a
> single function and in both cases the format string is at least say 25
> characters (completely random value), then it is pretty likely that a
> newline is intended.

This is on the edge of what I think could be done with checkpatch. I did 
a quick look through some of the cases I've noticed and haven't found 
any that this wouldn't pass. I'm not sure how you'd pick an appropriate 
threshold though. I do think it's quite clever, though.

> Alternatively, if the first format string doesn't end in a space and the
> second one doesn't begin with a space, then a newline is also likely
> intended.

This makes more sense to me but I don't expect it to be 100% accurate 
either. See, for example, drivers/char/dtlk.c:640 which has no space in 
either printk. You could maybe expand the rule to include ':' and '(' as 
the last character.

Logan

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 12:58 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 11:57 AM, Joe Perches wrote:
> > It may or not be correct.
> 
> It's absolutely not correct in that it either requires that a subsequent 
> KERN_CONT/pr_cont or a '\n' at the end and it has neither.

The warning described is simply not correct.

> > Without inter-function call code flow analysis,
> > it's not possible to be correct.
> 
> But how many cases actually have the pr_cont/KERN_cont called in 
> different functions? This appears to be exceedingly rare to me.

Probably more than 50.

> > If you can get the false positive & false negative
> > rate higher, I'll listen.

> The only two classes of false positives that you've pointed out or that 
> I'm aware of:
> 
> 1) The case where call did not either end in a '\n' or have a 
> KERN_CONT/pr_cont in a subsequent call.

or a bare printk.

>  I've been arguing (to deaf ears) 

wrong here too.

> that a warning is appropriate here and this is not a false positive 
> because it absolutely is incorrect one way or the other.

The checkpatch message itself has to be correct.
Classifying the defect properly is a requirement.

> Coccinnelle 
> will also suffer from this issue because it can no better decide whether 
> the developer intended for the next call to be a continuation or for a 
> '\n' to end the line.

Well, coccinelle could do a better job than a
line parser like checkpatch.

Line parsing is what makes the type of defect difficult
for a stupid parser, and checkpatch is one of those, to
be correct enough with a low enough false positive rate
to be useful.

Please be aware I have already written just about exactly
what you are trying to do more than once and discarded
the work because the defect report rate was just too high.

> 2) Cases where the pr_cont/KERN_CONT is not in sufficient context for 
> the script to detect. These are impossible to fix (and it's likely also 
> impossible for Coccinelle to be 100% accurate here). However, I'd expect 
> these to be *very* rare and I'm only actually aware of one case where 
> this has actually happened (lib/locking-selftest.c:1189) and (mostly by 
> luck) my v2 patch does not flag this where Coccinelle did. Not to 
> mention that continuation usage is discouraged in new code so this 
> should be even rarer on the majority of what checkpatch is used for.
> 
> (also 3. would be the %pV case, but I've removed those in what could be 
> a v3 of the patch -- I'd also be happy to address other false positives 
> classes if I could find them)

> False negatives are much harder to quantify or improve. But given that I 
> detect nearly 6000 errors

No, you don't detect errors, you detect matches.

If you look at your results a bit harder, you'll find many
false positives.

> And yet, you have not pointed out any false positives that my patch 
> gives which Coccinelle does/would not. It really feels to me like your 
> biases are guiding your decision here and you aren't really looking at 
> the results.

I know the kernel source code style very well.
You simply haven't looked very hard at your results.

> Another thought I've had is that the dev_ functions don't have any form 
> of continuation.

Untrue

> So we could potentially limit checkpatch to looking for 
> those to avoid the issues with continuations. It's not high coverage but 
> at least a lot of the driver patches would be checked with no chance of 
> false positives. I think there would be value in doing that.

For instance:

drivers/mfd/ipaq-micro.c:		dev_err(micro->dev,
drivers/mfd/ipaq-micro.c-			"unknown msg %d [%d] ", id, len);
drivers/mfd/ipaq-micro.c-		for (i = 0; i < len; ++i)
drivers/mfd/ipaq-micro.c-			pr_cont("0x%02x ", data[i]);
drivers/mfd/ipaq-micro.c-		pr_cont("\n");

$ git grep -A5 -P -w "\bdev_(warn|alert|crit|err|info|notice)" | \
  grep -B5 -P -w "printk|pr_cont"

will find some, but not all of these types of uses.

$ grep -A5 -rP --include=*.[ch] '\bdev_(warn|alert|crit|err|info|notice).*\"[^"]+(?<!n)"' * | \
  grep -B5 -w -P "(printk|pr_cont)"

will find fewer false positives, but miss some
multiline dev_<level> calls too.

Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line

[Joe Perches]

On Mon, 2017-11-27 at 12:58 -0700, Logan Gunthorpe wrote:
> 
> On 27/11/17 11:57 AM, Joe Perches wrote:
> > It may or not be correct.
> 
> It's absolutely not correct in that it either requires that a subsequent 
> KERN_CONT/pr_cont or a '\n' at the end and it has neither.

The warning described is simply not correct.

> > Without inter-function call code flow analysis,
> > it's not possible to be correct.
> 
> But how many cases actually have the pr_cont/KERN_cont called in 
> different functions? This appears to be exceedingly rare to me.

Probably more than 50.

> > If you can get the false positive & false negative
> > rate higher, I'll listen.

> The only two classes of false positives that you've pointed out or that 
> I'm aware of:
> 
> 1) The case where call did not either end in a '\n' or have a 
> KERN_CONT/pr_cont in a subsequent call.

or a bare printk.

>  I've been arguing (to deaf ears) 

wrong here too.

> that a warning is appropriate here and this is not a false positive 
> because it absolutely is incorrect one way or the other.

The checkpatch message itself has to be correct.
Classifying the defect properly is a requirement.

> Coccinnelle 
> will also suffer from this issue because it can no better decide whether 
> the developer intended for the next call to be a continuation or for a 
> '\n' to end the line.

Well, coccinelle could do a better job than a
line parser like checkpatch.

Line parsing is what makes the type of defect difficult
for a stupid parser, and checkpatch is one of those, to
be correct enough with a low enough false positive rate
to be useful.

Please be aware I have already written just about exactly
what you are trying to do more than once and discarded
the work because the defect report rate was just too high.

> 2) Cases where the pr_cont/KERN_CONT is not in sufficient context for 
> the script to detect. These are impossible to fix (and it's likely also 
> impossible for Coccinelle to be 100% accurate here). However, I'd expect 
> these to be *very* rare and I'm only actually aware of one case where 
> this has actually happened (lib/locking-selftest.c:1189) and (mostly by 
> luck) my v2 patch does not flag this where Coccinelle did. Not to 
> mention that continuation usage is discouraged in new code so this 
> should be even rarer on the majority of what checkpatch is used for.
> 
> (also 3. would be the %pV case, but I've removed those in what could be 
> a v3 of the patch -- I'd also be happy to address other false positives 
> classes if I could find them)

> False negatives are much harder to quantify or improve. But given that I 
> detect nearly 6000 errors

No, you don't detect errors, you detect matches.

If you look at your results a bit harder, you'll find many
false positives.

> And yet, you have not pointed out any false positives that my patch 
> gives which Coccinelle does/would not. It really feels to me like your 
> biases are guiding your decision here and you aren't really looking at 
> the results.

I know the kernel source code style very well.
You simply haven't looked very hard at your results.

> Another thought I've had is that the dev_ functions don't have any form 
> of continuation.

Untrue

> So we could potentially limit checkpatch to looking for 
> those to avoid the issues with continuations. It's not high coverage but 
> at least a lot of the driver patches would be checked with no chance of 
> false positives. I think there would be value in doing that.

For instance:

drivers/mfd/ipaq-micro.c:		dev_err(micro->dev,
drivers/mfd/ipaq-micro.c-			"unknown msg %d [%d] ", id, len);
drivers/mfd/ipaq-micro.c-		for (i = 0; i < len; ++i)
drivers/mfd/ipaq-micro.c-			pr_cont("0x%02x ", data[i]);
drivers/mfd/ipaq-micro.c-		pr_cont("\n");

$ git grep -A5 -P -w "\bdev_(warn|alert|crit|err|info|notice)" | \
  grep -B5 -P -w "printk|pr_cont"

will find some, but not all of these types of uses.

$ grep -A5 -rP --include=*.[ch] '\bdev_(warn|alert|crit|err|info|notice).*\"[^"]+(?<!n)"' * | \
  grep -B5 -w -P "(printk|pr_cont)"

will find fewer false positives, but miss some
multiline dev_<level> calls too.