Re: [PATCH] init: Fix printk format strings

Previous thread: [GIT PULL] Blackfin updates for 2.6.24 by Bryan Wu on Wednesday, October 10, 2007 - 8:31 pm. (1 message)

Next thread: [PATCH] Fix printk format strings by Vegard Nossum on Wednesday, October 10, 2007 - 11:23 pm. (1 message)
From: Vegard Nossum
Date: Wednesday, October 10, 2007 - 11:17 pm

This makes sure printk format strings are string literals containing no
more than a single line.


Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 init/calibrate.c        |    4 +++-
 init/do_mounts_initrd.c |    5 ++++-
 init/main.c             |    2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/init/calibrate.c b/init/calibrate.c
index 40ff3b4..2a35718 100644
--- a/init/calibrate.c
+++ b/init/calibrate.c
@@ -98,7 +98,9 @@ static unsigned long __devinit calibrate_delay_direct(void)
 		return (good_tsc_sum/good_tsc_count);
 
 	printk(KERN_WARNING "calibrate_delay_direct() failed to get a good "
-	       "estimate for loops_per_jiffy.\nProbably due to long platform interrupts. Consider using \"lpj=\" boot option.\n");
+	       "estimate for loops_per_jiffy.\n");
+	printk(KERN_WARNING "Probably due to long platform interrupts. "
+		"Consider using \"lpj=\" boot option.\n");
 	return 0;
 }
 #else
diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
index fd4fc12..ad6174c 100644
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -98,7 +98,10 @@ static void __init handle_initrd(void)
 			error = sys_ioctl(fd, BLKFLSBUF, 0);
 			sys_close(fd);
 		}
-		printk(!error ? "okay\n" : "failed\n");
+		if(error)
+			printk("failed\n");
+		else
+			printk("okay\n");
 	}
 }
 
diff --git a/init/main.c b/init/main.c
index 9def935..5491bba 100644
--- a/init/main.c
+++ b/init/main.c
@@ -537,7 +537,7 @@ asmlinkage void __init start_kernel(void)
 	boot_cpu_init();
 	page_address_init();
 	printk(KERN_NOTICE);
-	printk(linux_banner);
+	printk("%s", linux_banner);
 	setup_arch(&command_line);
 	setup_command_line(command_line);
 	unwind_setup();
-- 
1.5.2.4



-

From: Randy Dunlap
Date: Thursday, October 11, 2007 - 8:40 am

Why this one?
and if it must change, use:
		if (error)
but I see little need for the change.

---
~Randy
-

From: Vegard Nossum
Date: Thursday, October 11, 2007 - 8:55 am

Oops about the space. And it's to make the format string a constant
literal. If it isn't, you are going to run into trouble with options
to remove printks at compile-time based on log-level token. I realize
it's not really an issue at this point in time, but I assume that it
will eventually make its way into the kernel in some way or another.

Vegard
-

From: Randy Dunlap
Date: Thursday, October 11, 2007 - 9:01 am

so would this be OK?

		printk("%s\n", error ? "failed" : "okay");


---
~Randy
-

From: Vegard Nossum
Date: Thursday, October 11, 2007 - 9:09 am

It helps the filtering, but it doesn't help localisation very much, I
think. Since for that, you'd only translate the format string, which
in this case isn't much to translate. (I do realize that this is a
very, very tiny part of the kernel and probably hundreds of other
call-sites have much worse problems -- but every little helps, and now
that you're at it... :-))

Vegard
-

From: Randy Dunlap
Date: Thursday, October 11, 2007 - 9:20 am

Oh well.

I find the proposed changes too restrictive, practically changing
a stream-oriented output into a variable-length block-oriented output.


---
~Randy
-

From: Johannes Weiner
Date: Thursday, October 11, 2007 - 10:21 am

Hi,


Perhaps you should write _why_ one-line printk()s are even needed, with
profound reasons instead of constantly talking about mysterious later changes
that you will propose any time soon.

	Hannes
-

From: Vegard Nossum
Date: Thursday, October 11, 2007 - 12:15 pm

In short, the reason for disallowing multiple lines per call is that
printk() will be less complex, while not really changing the
complexity of the callers (a change in the code, yes, but the changes
are trivial, and, actually, not that many).

I think I see now, though, that my changes will not gain support if I
do not lift this restriction.

You can find my current "mysterious later changes" as patches, with
descriptions, at this address: http://folk.uio.no/vegardno/xprintf/

Thanks for the tip, though. I am quite new to the kernel business, so
bear with me :)

Kind regards,
Vegard Nossum
-

Previous thread: [GIT PULL] Blackfin updates for 2.6.24 by Bryan Wu on Wednesday, October 10, 2007 - 8:31 pm. (1 message)

Next thread: [PATCH] Fix printk format strings by Vegard Nossum on Wednesday, October 10, 2007 - 11:23 pm. (1 message)