treewide: Fix format strings that misuse continuations

String constants that are continued on subsequent lines with \
are not good.

The characters between seq_printf elements are tabs. That was probably not intentional, but isn't being changed. It's behind an #ifdef, so it could probably become a single space.

Signed-off-by Joe Perches <[email protected]>

---
 mm/slab.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/slab.c +0 -0

index 7451bda..9964619 100644

mm/slab.c static int s_show(struct seq_file *m, void *p) · line 4232

Quoted 12 lines · Joe Perches

mm/slab.c +2 -2

index 7451bda..9964619 100644
@@ -4228,8 +4228,8 @@ static int s_show(struct seq_file *m, void *p)
 		unsigned long node_frees = cachep->node_frees;
 		unsigned long overflows = cachep->node_overflow;
 
-		seq_printf(m, " : globalstat %7lu %6lu %5lu %4lu \
-				%4lu %4lu %4lu %4lu %4lu", allocs, high, grown,
+		seq_printf(m, " : globalstat %7lu %6lu %5lu %4lu 				%4lu %4lu %4lu %4lu %4lu",
+				allocs, high, grown,

Yuck. The right way to do this is by mergeable adjacent strings, eg:

printk("part 1..."

       " part 2...", ...);

String constants that are continued on subsequent lines with \
are not good.

The characters between seq_printf elements are tabs. That was probably not intentional, but isn't being changed. It's behind an #ifdef, so it could probably become a single space.

Signed-off-by Joe Perches <[email protected]>

---
 mm/slab.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/slab.c +0 -0

index 7451bda..9964619 100644

If that spacing part is really needed (is it?), wouldn't it be more readable as:

Review

Quoted 4 lines

(quoted diff) +4 -0

+		seq_printf(m, " : globalstat %7lu %6lu %5lu %4lu"
+				" 				"
+				"%4lu %4lu %4lu %4lu %4lu",  
+				allocs, high, grown,

String constants that are continued on subsequent lines with \
are not good.
Fixed a "is tryied" / tried typo

Signed-off-by Joe Perches <[email protected]>

---
 drivers/staging/dream/qdsp5/audio_mp3.c            |    3 +-- .../rtl8187se/ieee80211/ieee80211_softmac_wx.c     |    3 +-- drivers/staging/rtl8187se/r8180_core.c             |    3 +-- drivers/staging/sep/sep_driver.c                   |    3 +--

 4 files changed, 4 insertions(+), 8 deletions(-)

drivers/staging/dream/qdsp5/audio_mp3.c +1 -2

index b95574f..7ed6e26 100644
@@ -650,8 +650,7 @@ static long audio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
                                                       &audio->read_phys,
                                                       GFP_KERNEL);
                                if (!audio->read_data) {
-                                       pr_err("audio_mp3: malloc pcm \
-                                       buf failed\n");
+                                       pr_err("audio_mp3: malloc pcm buf failed\n");
                                        rc = -1;
                                } else {
                                        uint8_t index;

drivers/staging/rtl8187se/ieee80211/ieee80211_softmac_wx.c +0 -0

index f1d6cb4..ad42bcd 100644
@@ -482,8 +482,7 @@ int ieee80211_wx_set_power(struct ieee80211_device *ieee,

               (!ieee->enter_sleep_state) ||
               (!ieee->ps_is_queue_empty)){

-               printk("ERROR. PS mode is tryied to be use but\
-driver missed a callback\n\n");
+               printk("ERROR. PS mode tried to be use but driver missed a callback\n\n");

               return -1;
       }

drivers/staging/rtl8187se/r8180_core.c +0 -0

index e0f13ef..278992a 100644

drivers/staging/rtl8187se/r8180_core.c +0 -0

        printk(KERN_INFO "Copyright (c) 2004-2005, Andrea Merello\n");
        DMESG("Initializing module");
        DMESG("Wireless extensions version %d", WIRELESS_EXT);

drivers/staging/sep/sep_driver.c +0 -0

index e7bc9ec..8793878 100644
@@ -380,8 +380,7 @@ static int sep_mmap(struct file *filp, struct vm_area_struct *vma)

          shared area */
       if ((vma->vm_end - vma->vm_start) > SEP_DRIVER_MMMAP_AREA_SIZE) { edbg("SEP Driver mmap requested size is more than allowed\n");
-               printk(KERN_WARNING "SEP Driver mmap requested size is more \
-                       than allowed\n");
+               printk(KERN_WARNING "SEP Driver mmap requested size is more than allowed\n"); printk(KERN_WARNING "SEP Driver vma->vm_end is %08lx\n", vma->vm_end); printk(KERN_WARNING "SEP Driver vma->vm_end is %08lx\n", vma->vm_start); return -EAGAIN;
--
1.6.6.rc0.57.gad7a

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Quoted 7 lines · Frans Pop

If that spacing part is really needed (is it?), wouldn't it be more

readable as:

Review

Quoted 4 lines

(quoted diff) +4 -0

+		seq_printf(m, " : globalstat %7lu %6lu %5lu %4lu"
+				" 				"
+				"%4lu %4lu %4lu %4lu %4lu",
+				allocs, high, grown,

If it's required (most likely not, but it's a seq_printf and some people think those should never be modified because it's a public interface), it should probably be explicit:

" : globalstat %7lu %6lu %5lu %4lu \t\t\t\t%4lu %4lu %4lu %4lu %4lu"

String constants that are continued on subsequent lines with \
are not good.

Signed-off-by Joe Perches <[email protected]>

---
 arch/powerpc/kernel/nvram_64.c               |    6 +++---
 arch/powerpc/platforms/pseries/hotplug-cpu.c |    8 ++++----
 arch/powerpc/platforms/pseries/smp.c         |    4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

arch/powerpc/kernel/nvram_64.c +2 -2

index ad461e7..9cf197f 100644
@@ -338,8 +338,8 @@ static int __init nvram_create_os_partition(void)
 
 	rc = nvram_write_header(new_part);
 	if (rc <= 0) {
-		printk(KERN_ERR "nvram_create_os_partition: nvram_write_header \
-				failed (%d)\n", rc);
+		printk(KERN_ERR "nvram_create_os_partition: nvram_write_header "
+				"failed (%d)\n", rc);
 		return rc;
 	}
 

arch/powerpc/platforms/pseries/hotplug-cpu.c +4 -4

index 6ea4698..a70de10 100644
@@ -397,12 +397,12 @@ static int parse_cede_parameters(void)
 				CEDE_LATENCY_PARAM_MAX_LENGTH);
 
 	if (call_status != 0)
-		printk(KERN_INFO "CEDE_LATENCY: \
-			%s %s Error calling get-system-parameter(0x%x)\n",
+		printk(KERN_INFO "CEDE_LATENCY: "
+			"%s %s Error calling get-system-parameter(0x%x)\n",
 			__FILE__, __func__, call_status);
 	else
-		printk(KERN_INFO "CEDE_LATENCY: \
-			get-system-parameter successful.\n");
+		printk(KERN_INFO "CEDE_LATENCY: "
+			"get-system-parameter successful.\n");
 
 	return call_status;
 }

arch/powerpc/platforms/pseries/smp.c +2 -2

index b488663..4e7f89a 100644
@@ -144,8 +144,8 @@ static void __devinit smp_pSeries_kick_cpu(int nr)
 		hcpuid = get_hard_smp_processor_id(nr);
 		rc = plpar_hcall_norets(H_PROD, hcpuid);
 		if (rc != H_SUCCESS)
-			printk(KERN_ERR "Error: Prod to wake up processor %d\
-						Ret= %ld\n", nr, rc);
+			printk(KERN_ERR "Error: Prod to wake up processor %d "
+						"Ret= %ld\n", nr, rc);
 	}
 }
 

String constants that are continued on subsequent lines with \
are not good.

Signed-off-by Joe Perches <[email protected]>

Quoted 3 lines · Joe Perches

String constants that are continued on subsequent lines with \
are not good.

Quoted 1 line

Signed-off-by Joe Perches <[email protected]>

Gah, I thought I'd caught most of these when reviewing. If you're using a script to pick this stuff up it'd be worth checking for extraneous continuations in the middle of code - outside of macros there's little call for it.

Applied, thanks.

String constants that are continued on subsequent lines with \
are not good.

Fix rebulid/rebuild typos

Signed-off-by Joe Perches <[email protected]>

---
 drivers/scsi/arcmsr/arcmsr_hba.c |   49 +++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 26 deletions(-)

drivers/scsi/arcmsr/arcmsr_hba.c +0 -0

index 47d5d19..a0378d5 100644

Quoted 2 lines · Joe Perches

String constants that are continued on subsequent lines with \
are not good.

Why? It's perfectly valid ansi C.

but I also dislike your solution; I'd have split the string into two separate ones and relied on compiler concatenation.

However, the point is that all three are perfectly legal C. Choosing one form over another is something best left to the maintainers rather than imposing one style by fiat.

Consider this change veto'd unless you can get an explicit ack from the current maintainer for changing their style.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb) · line 589

Quoted 2 lines

drivers/scsi/arcmsr/arcmsr_hba.c +0 -2

-                     printk(KERN_NOTICE "arcmsr%d: wait 'flush adapter cache' \
-                     timeout, retry count down = %d \n", acb->host->host_no, retry_count);

So I might personally dislike this style

Quoted 3 lines · Joe Perches

String constants that are continued on subsequent lines with \
are not good.

Signed-off-by Joe Perches <[email protected]>

You want me to take that in the powerpc tree ?

A minor glitch below tho...

arch/powerpc/kernel/nvram_64.c static int __init nvram_create_os_partition(void) · line 354

Quoted 8 lines

arch/powerpc/kernel/nvram_64.c +1 -1

@@ -349,7 +349,7 @@ static int __init nvram_create_os_partition(void)
 	rc = ppc_md.nvram_write((char *)&seq_init, sizeof(seq_init), &tmp_index);
 	if (rc <= 0) {
 		printk(KERN_ERR "nvram_create_os_partition: nvram_write "
-				"failed (%d)\n", rc);
+		       "failed (%d)\n", rc);
 		return rc;
 	}

The above is objectionable :-)

String constants that are continued on subsequent lines with \
are not good.
Fix rebulid/rebuild typos

Signed-off-by Joe Perches <[email protected]>

Would it be an idea to also get rid of all the trailing spaces in printks?

If you'd prefer a separate patch, I can do it.

I think it's useful, though like all of these cleanups, a low priority. I suggest you submit a few through to various maintainers and see what happens.

(with false positives for asm and such)

$ grep -rP --include=*.[ch] "\s+\\\n" * | wc -l
7339

Quoted 4 lines · Mark Brown

Quoted 3 lines · Joe Perches

String constants that are continued on subsequent lines with \
are not good.

Quoted 8 lines

Quoted 1 line

Signed-off-by Joe Perches <[email protected]>

Gah, I thought I'd caught most of these when reviewing.  If you're using a script to pick this stuff up it'd be worth checking for extraneous continuations in the middle of code - outside of macros there's little call for it.

Applied, thanks.

There are a few false positives and probably a few missing using

       grep -rP --include=*.[ch] '".*\\$' * | \
       awk '{ if ((gsub("\"", "\"") % 2) == 1) print $0; }'

Most of the uses are __asm__ __volatile__ which could be considered unsightly but don't impact logging messages.

The rest could/should be fixed.

Quoted 9 lines · Joe Perches

There are a few false positives and probably a few missing using

       grep -rP --include=*.[ch] '".*\\$' * | \
       awk '{ if ((gsub("\"", "\"") % 2) == 1) print $0; }'

Most of the uses are __asm__ __volatile__ which could be considered unsightly but don't impact logging messages.

The rest could/should be fixed.

the Blackfin alsa fixes all look good to me, thanks

it'd be good to also check for just regular use of continuations in code other than macro definitions. These are just a style nit but if there's a script that filters out false positives from the macros that'd be handy...

Running "grep ' \\$' sound/soc/blackfin/*.[ch]" suggests that there's still some of the continuations I mentioned above in there (plus a lot of false positives from macros).

Quoted 8 lines · Mark Brown

it'd be good to also check for just regular use of continuations in code other than macro definitions.  These are just a style nit but if there's a script that filters out false positives from the macros that'd be handy...

Running "grep ' \\$' sound/soc/blackfin/*.[ch]" suggests that there's still some of the continuations I mentioned above in there (plus a lot of false positives from macros).

scripts/checkpatch.pl +11 -0

index 3257d3d..a174501 100755
@@ -1234,6 +1234,7 @@ sub process {
 
        $realcnt = 0;
        $linenr = 0;
+       my $in_define = 0;
        foreach my $line (@lines) {
                $linenr++;
 

@@ -1388,6 +1389,16 @@ sub process {
                        WARN("adding a line without newline at end of file\n" . $herecurr);
                }
 
+               if ($rawline =~ /\\$/) {
+                   if ($rawline =~ /\s*\#\s*define\s+/) {
+                       $in_define = 1;
+                   } elsif (!$in_define) {
+                       WARN("Continuations outside of macros should be avoided\n" . $herecurr);
+                   }
+               } else {
+                   $in_define = 0;
+               }
+
  # Blackfin: use hi/lo macros
                if ($realfile =~ m@arch/blackfin/.*\.S$@) {
                        if ($line =~ /\.[lL][[:space:]]*=.*&[[:space:]]*0x[fF][fF][fF][fF]/) {

find ./ -name "*.[ch]" | xargs ./scripts/checkpatch.pl -f | grep -a3 Continuations

Quoted 40 lines · Joe Perches

Quoted 8 lines · Mark Brown

it'd be good to also check for just regular use of continuations in code other than macro definitions.  These are just a style nit but if there's a script that filters out false positives from the macros that'd be handy...

Running "grep ' \\$' sound/soc/blackfin/*.[ch]" suggests that there's still some of the continuations I mentioned above in there (plus a lot of false positives from macros).

scripts/checkpatch.pl +11 -0

index 3257d3d..a174501 100755
@@ -1234,6 +1234,7 @@ sub process {
 
        $realcnt = 0;
        $linenr = 0;
+       my $in_define = 0;
        foreach my $line (@lines) {
                $linenr++;
 

@@ -1388,6 +1389,16 @@ sub process {
                        WARN("adding a line without newline at end of file\n" . $herecurr);
                }
 
+               if ($rawline =~ /\\$/) {
+                   if ($rawline =~ /\s*\#\s*define\s+/) {
+                       $in_define = 1;
+                   } elsif (!$in_define) {
+                       WARN("Continuations outside of macros should be avoided\n" . $herecurr);
+                   }
+               } else {
+                   $in_define = 0;
+               }
+
  # Blackfin: use hi/lo macros
                if ($realfile =~ m@arch/blackfin/.*\.S$@) {
                        if ($line =~ /\.[lL][[:space:]]*=.*&[[:space:]]*0x[fF][fF][fF][fF]/) {

Quoted 1 line

find ./ -name "*.[ch]" | xargs ./scripts/checkpatch.pl -f | grep -a3 Continuations

Checkpatch is already kind of loud, so I'm not sure I like the idea - you even say yourself that it's just a style nit.
But just in-case there are a lot of people who do like this, then you have to do some work to get rid of false positives. Try something like the following.

1. apply your patch.
2. run something like
find ./ -name "*.[ch]" | xargs ./scripts/checkpatch.pl -f | grep -a3 Continuations

and then go through the results looking for false positives.

One that I see right away, is with #if
For example,

#39: FILE: arch/arm/mach-lh7a40x/lcd-panel.h:39:
+#if defined (MACH_LPD79520)\

WARNING: Continuations outside of macros should be avoided #40: FILE: arch/arm/mach-lh7a40x/lcd-panel.h:40:
+ || defined (MACH_LPD79524)\

Quoted 4 lines · James Bottomley

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb) · line 589

Quoted 2 lines

drivers/scsi/arcmsr/arcmsr_hba.c +0 -2

-                     printk(KERN_NOTICE "arcmsr%d: wait 'flush adapter cache' \
-                     timeout, retry count down = %d \n", acb->host->host_no, retry_count);

So I might personally dislike this style

The problem here is not style, but that the whitespace of the indentation on the second line becomes part of the output!
That makes the code defective and is why Joe posted the patch series.

Quoted 1 line

Why? It's perfectly valid ansi C.

Quoted 2 lines

Consider this change veto'd unless you can get an explicit ack from the current maintainer for changing their style.

Quoted 9 lines · Joe Perches

Quoted 8 lines · Mark Brown

it'd be good to also check for just regular use of continuations in code other than macro definitions. These are just a style nit but if there's a script that filters out false positives from the macros that'd be handy...

Running "grep ' \\$' sound/soc/blackfin/*.[ch]" suggests that there's still some of the continuations I mentioned above in there (plus a lot of false positives from macros).

Checkpatch is already kind of loud, so I'm not sure I like the idea - you even say yourself that it's just a style nit.
But just in-case there are a lot of people who do like this, then you have to do some work to get rid of false positives. Try something like the following.

1. apply your patch.
2. run something like
find ./ -name "*.[ch]" | xargs ./scripts/checkpatch.pl -f | grep -a3 Continuations

and then go through the results looking for false positives.

There's probably something more appropriate that Andy Whitcroft could work out that actually apply patches and verifies the code using something like the ctx_statement_block functions but that code is overly mysterious to me.

Does anyone know if Andy Whitcroft is still looking after checkpatch?

Does anyone know if Andy Whitcroft is still looking after checkpatch?

String constants that are continued on subsequent lines with \
are not good.

Signed-off-by Joe Perches <[email protected]>

String constants that are continued on subsequent lines with \
are not good.

Signed-off-by Joe Perches <[email protected]>

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 3 lines · Joe Perches

String constants that are continued on subsequent lines with \
are not good.

Signed-off-by Joe Perches <[email protected]>

You want me to take that in the powerpc tree ?

Yes please.

Quoted 11 lines

A minor glitch below tho...

arch/powerpc/kernel/nvram_64.c static int __init nvram_create_os_partition(void) · line 354

Quoted 8 lines

arch/powerpc/kernel/nvram_64.c +1 -1

@@ -349,7 +349,7 @@ static int __init nvram_create_os_partition(void)
 	rc = ppc_md.nvram_write((char *)&seq_init, sizeof(seq_init), &tmp_index);
 	if (rc <= 0) {
 		printk(KERN_ERR "nvram_create_os_partition: nvram_write "
-				"failed (%d)\n", rc);
+		       "failed (%d)\n", rc);
 		return rc;
 	}

The above is objectionable :-)

Can you drop that section or do you need another patch?