checkpatch: Check for quoted strings broken across lines

PATCH In review Josh Triplett 4 participants 12 messages Feb 2, 2012 – Feb 3, 2012 Checkpatch

Share & tools

Josh Triplett

author

2012-02-02 20:06 UTC #0 lore raw

PATCH 1/1 checkpatch: Check for quoted strings broken across lines

Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.

Test case:

void context(void)

{
        pr_err("this string"
               " should produce a warning\n");
        pr_err("this multi-line string\n"
               "should not produce a warning\n");
        asm ("this asm\n\t"
             "should not produce a warning");
}

Results of checkpatch on that test case:

WARNING: quoted string split across lines
+ " should produce a warning\n");

total: 0 errors, 1 warnings, 9 lines checked

Signed-off-by Josh Triplett <[email protected]>

---
 scripts/checkpatch.pl |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

scripts/checkpatch.pl +11 -0

index e3bfcbe..ce4d740 100755
@@ -1737,6 +1737,17 @@ sub process {
 			     "line over 80 characters\n" . $herecurr);
 		}
 
+# Check for strings broken across lines (breaks greppability).  Make an
+# exception when the previous string ends in a newline (multiple lines in one
+# string constant) or \n\t (common in inline assembly to indent the instruction
+# on the following line).
+		if ($line =~ /^\+\s*"/ &&
+		    $prevline =~ /"\s*$/ &&
+		    $prevrawline !~ /\\n(?:\\t)*"\s*$/) {
+			WARN("SPLIT_STRING",
+			     "quoted string split across lines\n" . $herecurr);
+		}
+
 # check for spaces before a quoted newline
 		if ($rawline =~ /^.*\".*\s\\n/) {
 			WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE",

Signature · 1 line

1.7.9

Nick Bowler

2012-02-02 20:22 UTC #1 lore raw

Quoted 5 lines · Josh Triplett

Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.

[...]

scripts/checkpatch.pl sub process { · line 1743

Quoted 4 lines

scripts/checkpatch.pl +4 -0

+# Check for strings broken across lines (breaks greppability).  Make an
+# exception when the previous string ends in a newline (multiple lines in one
+# string constant) or \n\t (common in inline assembly to indent the instruction
+# on the following line).

There are tons of strings in the kernel that this makes checkpatch warn about where it probably shouldn't. For example, this one (from kernel/auditsc.c:1476):

audit_log_format(ab,
"oflag=0x%x mode=%#ho mq_flags=0x%lx mq_maxmsg=%ld " "mq_msgsize=%ld mq_curmsgs=%ld",

WARNING: quoted string split across lines
#1478: FILE: auditsc.c:1478:
+ "mq_msgsize=%ld mq_curmsgs=%ld",

Breaking "greppability" of this string is a non-issue, because this sort of string is not really greppable to begin with (and would certainly not be any easier to grep for if it were all on one line).

Cheers,

Signature · 1 line

Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Josh Triplett

author

2012-02-02 21:16 UTC #2 lore raw

Quoted 20 lines · Nick Bowler

Quoted 5 lines · Josh Triplett
Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.
[...]
scripts/checkpatch.pl sub process { · line 1743
Quoted 4 lines
scripts/checkpatch.pl +4 -0
+# Check for strings broken across lines (breaks greppability).  Make an
+# exception when the previous string ends in a newline (multiple lines in one
+# string constant) or \n\t (common in inline assembly to indent the instruction
+# on the following line).
There are tons of strings in the kernel that this makes checkpatch warn about where it probably shouldn't. For example, this one (from kernel/auditsc.c:1476):
audit_log_format(ab,
"oflag=0x%x mode=%#ho mq_flags=0x%lx mq_maxmsg=%ld " "mq_msgsize=%ld mq_curmsgs=%ld",

Quoted 7 lines

WARNING: quoted string split across lines
#1478: FILE: auditsc.c:1478:
+ "mq_msgsize=%ld mq_curmsgs=%ld",
Breaking "greppability" of this string is a non-issue, because this sort of string is not really greppable to begin with (and would certainly not be any easier to grep for if it were all on one line).

While I agree that in that particular case (heavy on the %formats and light on the text) you can't easily grep to begin with, the guideline from CodingStyle still applies, as does the standard guideline about checkpatch (namely "don't go globally fixing everything it says, but fix it in new or changed code").

I certainly don't think joining those lines would *hurt*. Making that change blindly across the entire kernel doesn't seem worth it, but changing it on new code seems like a good idea.

In theory checkpatch could try to heuristically ignore cases where the split in the string occurs immediately before or after a %format, but I don't fancy writing a regex to match valid printf format specifiers. :)

I still think this patch provides a net win, and I don't think the example you mentioned represents a false positive.

- Josh Triplett

Nick Bowler

2012-02-02 22:09 UTC #3 lore raw

Quoted 25 lines · Josh Triplett

Quoted 24 lines · Nick Bowler
Quoted 5 lines · Josh Triplett
Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.
[...]
scripts/checkpatch.pl sub process { · line 1743
Quoted 4 lines
scripts/checkpatch.pl +4 -0
+# Check for strings broken across lines (breaks greppability).  Make an
+# exception when the previous string ends in a newline (multiple lines in one
+# string constant) or \n\t (common in inline assembly to indent the instruction
+# on the following line).
There are tons of strings in the kernel that this makes checkpatch warn about where it probably shouldn't. For example, this one (from kernel/auditsc.c:1476):
audit_log_format(ab,
"oflag=0x%x mode=%#ho mq_flags=0x%lx mq_maxmsg=%ld " "mq_msgsize=%ld mq_curmsgs=%ld",
WARNING: quoted string split across lines
#1478: FILE: auditsc.c:1478:
+ "mq_msgsize=%ld mq_curmsgs=%ld",

Quoted 7 lines

Quoted 3 lines
Breaking "greppability" of this string is a non-issue, because this sort of string is not really greppable to begin with (and would certainly not be any easier to grep for if it were all on one line).
While I agree that in that particular case (heavy on the %formats and light on the text) you can't easily grep to begin with, the guideline from CodingStyle still applies,

The guideline from CodingStyle talks about "user-visible strings". I don't think this string counts, because it contains code that is not displayed to the user (hence, the string is not user-visible). That is the reason why it's not greppable in the first place.

There are hundreds, if not thousands of similar strings in the kernel.

Quoted 5 lines

as does the standard guideline about checkpatch (namely "don't go globally fixing everything it says, but fix it in new or changed code").
I certainly don't think joining those lines would *hurt*.

I disagree. Joining those lines will push the conversion specifiers way off to the right (possibly off the screen), and these specifiers are vital to understanding the rest of the function parameters. In other words, the specifiers should be treated the same as actual *code*.

Quoted 2 lines

Making that change blindly across the entire kernel doesn't seem worth it, but changing it on new code seems like a good idea.

For reasons that I've already outlined, I don't think it's a good idea to change it in new code, either.

Quoted 3 lines

In theory checkpatch could try to heuristically ignore cases where the split in the string occurs immediately before or after a %format, but I don't fancy writing a regex to match valid printf format specifiers. :)

It doesn't hurt to be conservative, erring on the side of not warning (the status quo) rather than warning. Regardless, writing a regex for printf conversion specifiers should be straightforward, because the format is so rigid.

Here's a perl regex which should match all the valid possibilities according to ISO C99 (untested!). Catching the Linux extensions is left as an exercise to the reader.

%[-+ #0]*(hh|ll|[ljztL])?[*[:digit:]]*(\.[*[:digit:]]*)?[diouxXfFeEgGaAcspn%]

Quoted 2 lines

I still think this patch provides a net win, and I don't think the example you mentioned represents a false positive.

Cheers,

Signature · 1 line

Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Josh Triplett

author

2012-02-03 01:31 UTC #4 lore raw

Quoted 34 lines · Nick Bowler

Quoted 25 lines · Josh Triplett
Quoted 24 lines · Nick Bowler
Quoted 5 lines · Josh Triplett
Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.
[...]
scripts/checkpatch.pl sub process { · line 1743
Quoted 4 lines
scripts/checkpatch.pl +4 -0
+# Check for strings broken across lines (breaks greppability).  Make an
+# exception when the previous string ends in a newline (multiple lines in one
+# string constant) or \n\t (common in inline assembly to indent the instruction
+# on the following line).
There are tons of strings in the kernel that this makes checkpatch warn about where it probably shouldn't. For example, this one (from kernel/auditsc.c:1476):
audit_log_format(ab,
"oflag=0x%x mode=%#ho mq_flags=0x%lx mq_maxmsg=%ld " "mq_msgsize=%ld mq_curmsgs=%ld",
WARNING: quoted string split across lines
#1478: FILE: auditsc.c:1478:
+ "mq_msgsize=%ld mq_curmsgs=%ld",
Quoted 7 lines
Quoted 3 lines
Breaking "greppability" of this string is a non-issue, because this sort of string is not really greppable to begin with (and would certainly not be any easier to grep for if it were all on one line).
While I agree that in that particular case (heavy on the %formats and light on the text) you can't easily grep to begin with, the guideline from CodingStyle still applies,

Quoted 6 lines

The guideline from CodingStyle talks about "user-visible strings". I don't think this string counts, because it contains code that is not displayed to the user (hence, the string is not user-visible). That is the reason why it's not greppable in the first place.
There are hundreds, if not thousands of similar strings in the kernel.

OK, with a combination of arcane grep/sed commands and hand checking, I just scanned the *entire* kernel for quoted strings split across lines, and filtered out all user-visible strings, leaving only the split non-user-visible strings.

An excerpt from that pile of ugly, suggesting just how many ways kernel code has to spell "user-visible":

I found that a single change to my checkpatch test (only checking strings passed as function arguments) limits the false positives in the entire kernel to a grand total of 12:

arch/powerpc/platforms/iseries/mf.c: memcpy(src, "\x01\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00"

arch/um/os-Linux/process.c:     if (sscanf(buf, "%*d " COMM_SCANF " %*c %*d %*d %*d %*d %*d %*d %*d "
arch/x86/platform/olpc/olpc_dt.c:       olpc_dt_interpret("\" /battery@0\" find-device"
arch/x86/platform/olpc/olpc_dt.c:               olpc_dt_interpret("\" /pci/display@1\" find-device"
arch/x86/platform/olpc/olpc_dt.c:               olpc_dt_interpret("\" /pci/display@1,1\" find-device"
drivers/block/rbd.c:               "%" __stringify(RBD_MAX_OBJ_NAME_LEN) "s"
drivers/pcmcia/ds.c:    if (add_uevent_var(env, "MODALIAS=pcmcia:m%04Xc%04Xf%02Xfn%02Xpfn%02X"
drivers/tty/tty_audit.c:                audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u "
fs/ecryptfs/crypto.c:static unsigned char *portable_filename_chars = ("-.0123456789ABCD"
kernel/auditsc.c:               audit_log_format(ab, " inode=%lu"
kernel/auditsc.c:                       audit_log_format(ab, "login pid=%d uid=%u "
security/selinux/ss/services.c: audit_log_format(ab, "op=security_compute_av reason=%s "

Not "hundreds, if not thousands", but 12, including 4 calls to audit_log_format. :)

Meanwhile, this checkpatch test correctly flags several thousand instances of user-visible strings that shouldn't get split. That seems like a pretty reasonable ratio to me; what do you think?

I'll send PATCHv2, which limits the check to function parameters, momentarily.

- Josh Triplett

Jesper Juhl

2012-02-02 21:29 UTC #5 lore raw

In reply to Nick Bowler

Quoted 8 lines · Nick Bowler

Quoted 5 lines · Josh Triplett
Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.
[...]
Quoted 1 line
+# Check for strings broken across lines (breaks greppability). Make an

s/greppability/grepability/ ?

Quoted 12 lines

scripts/checkpatch.pl sub process { · line 1743
Quoted 3 lines
scripts/checkpatch.pl +3 -0
+# exception when the previous string ends in a newline (multiple lines in one
+# string constant) or \n\t (common in inline assembly to indent the instruction
+# on the following line).
There are tons of strings in the kernel that this makes checkpatch warn about where it probably shouldn't. For example, this one (from kernel/auditsc.c:1476):
audit_log_format(ab,
"oflag=0x%x mode=%#ho mq_flags=0x%lx mq_maxmsg=%ld " "mq_msgsize=%ld mq_curmsgs=%ld",

Quoted 8 lines

WARNING: quoted string split across lines
#1478: FILE: auditsc.c:1478:
+ "mq_msgsize=%ld mq_curmsgs=%ld",
Breaking "greppability" of this string is a non-issue, because this sort of string is not really greppable to begin with (and would certainly not be any easier to grep for if it were all on one line).

Agreed. checkpatch needs to not warn about strings like this. But generally a good idea to warn about "easily grepable strings broken into multiple lines" if possible, IMHO...

Signature · 3 lines

Jesper Juhl <[email protected]>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Joe Perches

maintainer

2012-02-02 21:34 UTC #6 lore raw

Quoted 2 lines · Jesper Juhl

But generally a good idea to warn about "easily grepable strings broken into multiple lines" if possible, IMHO...

Maybe count the %'s? Ignore if more than 2?

Josh Triplett

author

2012-02-02 22:02 UTC #7 lore raw

Quoted 5 lines · Joe Perches

Quoted 2 lines · Jesper Juhl
But generally a good idea to warn about "easily grepable strings broken into multiple lines" if possible, IMHO...
Maybe count the %'s? Ignore if more than 2?

Numerous greppable strings include several formats, but you can generally look at a message and know which non-parameterized bits to grep for. Even in the case of strings with numerous parameters, it often works better to grep for the whole string and replace the parameters with '.*', and make the string incrementally less specific until I find it. For example, from dmesg on my system:

[    0.000000] last_pfn = 0xdb000 max_arch_pfn = 0x400000000

Grepping for "last_pfn" or even "last_pfn = " produces a pile of results, but grepping for 'last_pfn = .* max_arch_pfn' produces exactly one result:

arch/x86/kernel/e820.c: printk(KERN_INFO "last_pfn = %#lx max_arch_pfn = %#lx\n",

That wouldn't work if the string broke across lines. So, I still think it seems reasonable for checkpatch to encourage keeping such strings un-split.

- Josh Triplett

Joe Perches

maintainer

2012-02-02 20:36 UTC #8 lore raw

In reply to Josh Triplett

Quoted 5 lines · Josh Triplett

Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.

[]

scripts/checkpatch.pl

Quoted 1 line

scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process {

Quoted 1 line

scripts/checkpatch.pl +0 -0
@@ -1737,6 +1737,17 @@ sub process {

[]

scripts/checkpatch.pl sub process { · line 1748

Quoted 9 lines

scripts/checkpatch.pl +9 -0

+# Check for strings broken across lines (breaks greppability).  Make an
+# exception when the previous string ends in a newline (multiple lines in one
+# string constant) or \n\t (common in inline assembly to indent the instruction
+# on the following line).
+		if ($line =~ /^\+\s*"/ &&
+		    $prevline =~ /"\s*$/ &&
+		    $prevrawline !~ /\\n(?:\\t)*"\s*$/) {
+			WARN("SPLIT_STRING",
+			     "quoted string split across lines\n" . $herecurr);

Seems sensible but there are also asm uses like:

arch/x86/include/asm/pvclock.h: "xor %5,%5 ; "

                "add  %4,%%eax ; "

Josh Triplett

author

2012-02-02 21:28 UTC #9 lore raw

Quoted 23 lines · Joe Perches

Quoted 5 lines · Josh Triplett
Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.
[]
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0 -0
[]
scripts/checkpatch.pl sub process {
Quoted 1 line
scripts/checkpatch.pl +0 -0
@@ -1737,6 +1737,17 @@ sub process {
[]
scripts/checkpatch.pl sub process { · line 1748
Quoted 9 lines
scripts/checkpatch.pl +9 -0
+# Check for strings broken across lines (breaks greppability).  Make an
+# exception when the previous string ends in a newline (multiple lines in one
+# string constant) or \n\t (common in inline assembly to indent the instruction
+# on the following line).
+		if ($line =~ /^\+\s*"/ &&
+		    $prevline =~ /"\s*$/ &&
+		    $prevrawline !~ /\\n(?:\\t)*"\s*$/) {
+			WARN("SPLIT_STRING",
+			     "quoted string split across lines\n" . $herecurr);
Seems sensible but there are also asm uses like:

Quoted 2 lines

arch/x86/include/asm/pvclock.h: "xor %5,%5 ; "
                "add  %4,%%eax ; "

I did see those, yes. However, a quick grep through the kernel shows that those occur quite rarely compared to \n or \n\t; the latter looks like the standard style. How about I provide a patch for Documentation/CodingStyle adding a chapter on inline assembly, with that and other style guidelines? :)

- Josh Triplett

Joe Perches

maintainer

2012-02-02 21:32 UTC #10 lore raw

Quoted 7 lines · Josh Triplett

Quoted 6 lines · Joe Perches
Quoted 5 lines · Josh Triplett
Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.

[]

Quoted 8 lines

Quoted 3 lines
Seems sensible but there are also asm uses like:
arch/x86/include/asm/pvclock.h: "xor %5,%5 ; "
                "add  %4,%%eax ; "
I did see those, yes. However, a quick grep through the kernel shows that those occur quite rarely compared to \n or \n\t; the latter looks like the standard style. How about I provide a patch for Documentation/CodingStyle adding a chapter on inline assembly, with that and other style guidelines? :)

Sounds fine to me.

Another option might be to ignore all
strings in any asm block.

Josh Triplett

author

2012-02-02 22:36 UTC #11 lore raw

Quoted 12 lines · Joe Perches

Quoted 7 lines · Josh Triplett
Quoted 6 lines · Joe Perches
Quoted 5 lines · Josh Triplett
Documentation/CodingStyle recommends not splitting quoted strings across lines, because it breaks the ability to grep for the string. checkpatch already makes an exception to the 80-column rule for quoted strings to allow this. Rather than just allowing it, actively warn about quoted strings split across lines.
[]
Quoted 3 lines
Quoted 3 lines
Seems sensible but there are also asm uses like:
arch/x86/include/asm/pvclock.h: "xor %5,%5 ; "
                "add  %4,%%eax ; "

Quoted 7 lines

Quoted 5 lines
I did see those, yes. However, a quick grep through the kernel shows that those occur quite rarely compared to \n or \n\t; the latter looks like the standard style. How about I provide a patch for Documentation/CodingStyle adding a chapter on inline assembly, with that and other style guidelines? :)
Sounds fine to me.

Done, with message-id <20120202223304.GA13069@leaf> and subject "[PATCH] Documentation/CodingStyle: Add guidelines for inline assembly".

- Josh triplett