LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings
@ 2008-02-22 13:26 Andi Kleen
  2008-02-22 13:53 ` Stefan Richter
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Andi Kleen @ 2008-02-22 13:26 UTC (permalink / raw)
  To: linux-kernel, torvalds

RFC: Update coding standard to avoid split up printk format strings

Common occurrence: You see some error message in the kernel log you don't
understand, Standard way to handle this is to grep the kernel source code 
for that error message and then look at the code and figure out what
is wrong from that.

Unfortunately that is often far more difficult than needed because 
with the strict 80 character limit printk format strings are often split up
over multiple lines. One example for this would be

	  printk(KERN_CRIT "protocol %04x is "
                          "buggy, dev %s\n",
                          skb2->protocol, dev->name);

from net/core/dev.c. If you see this obscure message what would you grep
for? "protocol.*is buggy" will not find it and "is buggy" neither and "buggy"
or "protocol" gives you a zillion of hits that you have review all
by hand. All quite unsatisfactory.

Worse the coding standard actually recommends to split up printks like
this, but I think it is one of the most counter0productive suggestions it
has.

There are two reasonable ways to handle this. Either put the format 
string on an own line with backwards indentation or ignore the column 80
limitation. I think both are better actually than having ungreppable
error messages.

This patch updates the coding standard for to allow this.

Of course it only applies to the printk format string, any other 
printk arguments should be still split up over multiple lines as before.
That is fine because they rarely need to be grepped for.

I expect this proposal will likely be somewhat controversal and I don't expect
universal agreement (and it's likely a bike shed paint topic like
all coding standard issues); but let's see what happens.

I think it would be a valuable improvement of the coding standard.

At some point checkpatch.pl would need to be updated to know about this
exception too, that would be the next step.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/Documentation/CodingStyle
===================================================================
--- linux.orig/Documentation/CodingStyle
+++ linux/Documentation/CodingStyle
@@ -83,20 +83,32 @@ preferred limit.
 Statements longer than 80 columns will be broken into sensible chunks.
 Descendants are always substantially shorter than the parent and are placed
 substantially to the right. The same applies to function headers with a long
-argument list. Long strings are as well broken into shorter strings. The
-only exception to this is where exceeding 80 columns significantly increases
-readability and does not hide information.
+argument list.
 
-void fun(int a, int b, int c)
-{
-	if (condition)
-		printk(KERN_WARNING "Warning this is a long printk with "
-						"3 parameters a: %u b: %u "
-						"c: %u \n", a, b, c);
-	else
-		next_statement;
+It is not recommended to break printk format strings into smaller strings.
+The problem with doing this is that it makes it much harder to grep
+for the error messages in the source if they are split up over multiple
+lines. And grepping for error messages is fairly important for debugging.
+So for the special case of printk format strings (or formatting any other
+user visible error message) the normal 80 character column rule
+does not apply. Or alternatively it is ok to violate the indentation
+rule for the format string only if that makes the end not exceed
+80 characters. For example
+
+void function(void)
+{
+	if (...) {
+		if (...) {
+			printk(
+ "very very long formatting string with argument %d and argument %d\n",
+				a, b);
+		}
+	}
 }
 
+You should still break up the further printk arguments if they exceed 80
+characters though.
+
 		Chapter 3: Placing Braces and Spaces
 
 The other issue that always comes up in C styling is the placement of

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings
  2008-02-22 13:26 [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings Andi Kleen
@ 2008-02-22 13:53 ` Stefan Richter
  2008-02-22 14:01   ` Andi Kleen
  2008-02-22 15:02 ` Alan Cox
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Stefan Richter @ 2008-02-22 13:53 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-kernel, torvalds

Andi Kleen wrote:
> --- linux.orig/Documentation/CodingStyle
> +++ linux/Documentation/CodingStyle
> @@ -83,20 +83,32 @@ preferred limit.
>  Statements longer than 80 columns will be broken into sensible chunks.
>  Descendants are always substantially shorter than the parent and are placed
>  substantially to the right. The same applies to function headers with a long
> -argument list. Long strings are as well broken into shorter strings. The
> -only exception to this is where exceeding 80 columns significantly increases
> -readability and does not hide information.
> +argument list.
>  
> -void fun(int a, int b, int c)
> -{
> -	if (condition)
> -		printk(KERN_WARNING "Warning this is a long printk with "
> -						"3 parameters a: %u b: %u "
> -						"c: %u \n", a, b, c);
> -	else
> -		next_statement;
> +It is not recommended to break printk format strings into smaller strings.

Instead of a new recommendation (from now on we recommend something
contrary to what we required up until yesterday --- let's go unwrap
strings everywhere in the kernel now), how about simply saying that
printk format strings are not subject to the 80 column rule?
Or keep the old text and insert after "increases readability":  "or
helps full-text searching".  And delete the example code.

> +The problem with doing this is that it makes it much harder to grep
> +for the error messages in the source if they are split up over multiple
> +lines. And grepping for error messages is fairly important for debugging.
> +So for the special case of printk format strings (or formatting any other
> +user visible error message) the normal 80 character column rule
> +does not apply. Or alternatively it is ok to violate the indentation
> +rule for the format string only if that makes the end not exceed
> +80 characters. For example
> +
> +void function(void)
> +{
> +	if (...) {
> +		if (...) {
> +			printk(
> + "very very long formatting string with argument %d and argument %d\n",
> +				a, b);
> +		}
> +	}
>  }

Here is one vote against this indentation exception.

PS:  Could someone implement this for checkpatch.pl:

	WARN_ON(hunk_is_in("Documentation/CodingStyle") &&
		lines_added > lines_removed);
-- 
Stefan Richter
-=====-==--- --=- =-==-
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings
  2008-02-22 13:53 ` Stefan Richter
@ 2008-02-22 14:01   ` Andi Kleen
  0 siblings, 0 replies; 8+ messages in thread
From: Andi Kleen @ 2008-02-22 14:01 UTC (permalink / raw)
  To: Stefan Richter; +Cc: Andi Kleen, linux-kernel, torvalds

> Instead of a new recommendation (from now on we recommend something
> contrary to what we required up until yesterday --- let's go unwrap
> strings everywhere in the kernel now), how about simply saying that
> printk format strings are not subject to the 80 column rule?

I personally slightly prefer the "unindent" solution over the ">80char"
solution, but it is not a very strong preference and both would be ok
for me.

-Andi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings
  2008-02-22 13:26 [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings Andi Kleen
  2008-02-22 13:53 ` Stefan Richter
@ 2008-02-22 15:02 ` Alan Cox
  2008-02-22 16:58 ` Joe Perches
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Alan Cox @ 2008-02-22 15:02 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-kernel, torvalds


> At some point checkpatch.pl would need to be updated to know about this
> exception too, that would be the next step.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>

Acked-by: Alan Cox <alan@redhat.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings
  2008-02-22 13:26 [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings Andi Kleen
  2008-02-22 13:53 ` Stefan Richter
  2008-02-22 15:02 ` Alan Cox
@ 2008-02-22 16:58 ` Joe Perches
  2008-02-23 12:55 ` Christer Weinigel
  2008-02-25  5:30 ` Andy Whitcroft
  4 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2008-02-22 16:58 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-kernel, torvalds, git

On Fri, 2008-02-22 at 14:26 +0100, Andi Kleen wrote:
> RFC: Update coding standard to avoid split up printk format strings

Perhaps it's more useful to have git become more content aware.

If git could track user specified file glob pattern ("*.[ch]$")
changes by statement terminator (";") and git could format
output by a user specified pass (lindent), many of the bike shed
paint discussions and a lot of commits would never occur.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings
  2008-02-22 13:26 [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings Andi Kleen
                   ` (2 preceding siblings ...)
  2008-02-22 16:58 ` Joe Perches
@ 2008-02-23 12:55 ` Christer Weinigel
  2008-02-25  5:29   ` Andy Whitcroft
  2008-02-25  5:30 ` Andy Whitcroft
  4 siblings, 1 reply; 8+ messages in thread
From: Christer Weinigel @ 2008-02-23 12:55 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-kernel, torvalds

Andi Kleen wrote:
> RFC: Update coding standard to avoid split up printk format strings

While we're talking about checkpatch.pl, I'd definitely like to teach 
checkpatch about "list_for_each" and friends.

list_for_each is flow control, not a function call.  I find it much 
easier to see that something is a loop when there is a space between the 
name and the parenthesis rather than when they are smashed together.

old patch follows

   /Christer

checkpatch complains about the following:

WARNING: no space between function name and open parenthesis '('
#520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
+       list_for_each_entry (transfer, &message->transfers, transfer_list) {

which I think is a bit bogus since it actually is a for statement in
disguise.  The following patch adds list_for_each to the list of things
that look like functions that it shouldn't complain about.

Index: linux-2.6.23/scripts/checkpatch.pl
===================================================================
--- linux-2.6.23.orig/scripts/checkpatch.pl
+++ linux-2.6.23/scripts/checkpatch.pl
@@ -681,7 +681,7 @@ sub process {

  # check for spaces between functions and their parentheses.
  		if ($line =~ /($Ident)\s+\(/ &&
-		    $1 !~ 
/^(?:if|for|while|switch|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/ 
&&
+		    $1 !~ 
/^(?:if|for|while|switch|list_for_each.*|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/ 
&&
  		    $line !~ /$Type\s+\(/ && $line !~ /^.\#\s*define\b/) {
  			WARN("no space between function name and open parenthesis '('\n" . 
$herecurr);
  		}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings
  2008-02-23 12:55 ` Christer Weinigel
@ 2008-02-25  5:29   ` Andy Whitcroft
  0 siblings, 0 replies; 8+ messages in thread
From: Andy Whitcroft @ 2008-02-25  5:29 UTC (permalink / raw)
  To: Christer Weinigel; +Cc: Andi Kleen, linux-kernel, torvalds

On Sat, Feb 23, 2008 at 01:55:32PM +0100, Christer Weinigel wrote:
> Andi Kleen wrote:
> >RFC: Update coding standard to avoid split up printk format strings
> 
> While we're talking about checkpatch.pl, I'd definitely like to teach 
> checkpatch about "list_for_each" and friends.
> 
> list_for_each is flow control, not a function call.  I find it much 
> easier to see that something is a loop when there is a space between the 
> name and the parenthesis rather than when they are smashed together.
> 
> old patch follows
> 
>   /Christer
> 
> checkpatch complains about the following:
> 
> WARNING: no space between function name and open parenthesis '('
> #520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
> +       list_for_each_entry (transfer, &message->transfers, transfer_list) {
> 
> which I think is a bit bogus since it actually is a for statement in
> disguise.  The following patch adds list_for_each to the list of things
> that look like functions that it shouldn't complain about.
> 
> Index: linux-2.6.23/scripts/checkpatch.pl
> ===================================================================
> --- linux-2.6.23.orig/scripts/checkpatch.pl
> +++ linux-2.6.23/scripts/checkpatch.pl
> @@ -681,7 +681,7 @@ sub process {
> 
>  # check for spaces between functions and their parentheses.
>  		if ($line =~ /($Ident)\s+\(/ &&
> -		    $1 !~ 
> /^(?:if|for|while|switch|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/ 
> &&
> +		    $1 !~ 
> /^(?:if|for|while|switch|list_for_each.*|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/ 
> &&
>  		    $line !~ /$Type\s+\(/ && $line !~ /^.\#\s*define\b/) {
>  			WARN("no space between function name and open 
>  			parenthesis '('\n" . $herecurr);
>  		}

This has come up a few times.  In previous discussions there were
supporters for spaces as well as not.  Looking over the existing in kernel
use, the majority are without a space (from before checkpatch encouraged
that usage); and so it is that that won the day.

-apw

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings
  2008-02-22 13:26 [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings Andi Kleen
                   ` (3 preceding siblings ...)
  2008-02-23 12:55 ` Christer Weinigel
@ 2008-02-25  5:30 ` Andy Whitcroft
  4 siblings, 0 replies; 8+ messages in thread
From: Andy Whitcroft @ 2008-02-25  5:30 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-kernel, torvalds

On Fri, Feb 22, 2008 at 02:26:12PM +0100, Andi Kleen wrote:

> At some point checkpatch.pl would need to be updated to know about this
> exception too, that would be the next step.

Cirtainly we have exceptions for docstrings, so that shouldn't be a
problem if this were accepted.

-apw

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2008-02-25  5:29 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-22 13:26 [RFC/PATCH] Update coding standard to avoid ungrepable printk format strings Andi Kleen
2008-02-22 13:53 ` Stefan Richter
2008-02-22 14:01   ` Andi Kleen
2008-02-22 15:02 ` Alan Cox
2008-02-22 16:58 ` Joe Perches
2008-02-23 12:55 ` Christer Weinigel
2008-02-25  5:29   ` Andy Whitcroft
2008-02-25  5:30 ` Andy Whitcroft

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).