[PATCH] init: Fix printk format strings

[PATCH] init: Fix printk format strings

[Vegard Nossum]

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

Re: [PATCH] init: Fix printk format strings

[Randy Dunlap]

On Thu, 11 Oct 2007 08:17:02 +0200 Vegard Nossum wrote:

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

Each patch needs justification (unless it is blatantly obvious).


> 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/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");
>  	}
>  }

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

---
~Randy

Re: [PATCH] init: Fix printk format strings

[Vegard Nossum]

On 10/11/07, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> On Thu, 11 Oct 2007 08:17:02 +0200 Vegard Nossum wrote:
>
> > This makes sure printk format strings are string literals containing no
> > more than a single line.
>
> Each patch needs justification (unless it is blatantly obvious).
>
>
> > 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/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");
> >       }
> >  }
>
> Why this one?
> and if it must change, use:
>                 if (error)
> but I see little need for the change.

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

Re: [PATCH] init: Fix printk format strings

[Randy Dunlap]

On Thu, 11 Oct 2007 17:55:16 +0200 Vegard Nossum wrote:

> On 10/11/07, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> > On Thu, 11 Oct 2007 08:17:02 +0200 Vegard Nossum wrote:
> >
> > > This makes sure printk format strings are string literals containing no
> > > more than a single line.
> >
> > Each patch needs justification (unless it is blatantly obvious).
> >
> >
> > > 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/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");
> > >       }
> > >  }
> >
> > Why this one?
> > and if it must change, use:
> >                 if (error)
> > but I see little need for the change.
> 
> 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.

so would this be OK?

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


---
~Randy

Re: [PATCH] init: Fix printk format strings

[Vegard Nossum]

On 10/11/07, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> On Thu, 11 Oct 2007 17:55:16 +0200 Vegard Nossum wrote:
>
> > On 10/11/07, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> > > On Thu, 11 Oct 2007 08:17:02 +0200 Vegard Nossum wrote:
> > >
> > > > This makes sure printk format strings are string literals containing no
> > > > more than a single line.
> > >
> > > Each patch needs justification (unless it is blatantly obvious).
> > >
> > >
> > > > 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/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");
> > > >       }
> > > >  }
> > >
> > > Why this one?
> > > and if it must change, use:
> > >                 if (error)
> > > but I see little need for the change.
> >
> > 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.
>
> so would this be OK?
>
>                 printk("%s\n", error ? "failed" : "okay");

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

Re: [PATCH] init: Fix printk format strings

[Randy Dunlap]

On Thu, 11 Oct 2007 18:09:47 +0200 Vegard Nossum wrote:

> On 10/11/07, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> > On Thu, 11 Oct 2007 17:55:16 +0200 Vegard Nossum wrote:
> >
> > > On 10/11/07, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> > > > On Thu, 11 Oct 2007 08:17:02 +0200 Vegard Nossum wrote:
> > > >
> > > > > This makes sure printk format strings are string literals containing no
> > > > > more than a single line.
> > > >
> > > > Each patch needs justification (unless it is blatantly obvious).
> > > >
> > > >
> > > > > 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/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");
> > > > >       }
> > > > >  }
> > > >
> > > > Why this one?
> > > > and if it must change, use:
> > > >                 if (error)
> > > > but I see little need for the change.
> > >
> > > 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.
> >
> > so would this be OK?
> >
> >                 printk("%s\n", error ? "failed" : "okay");
> 
> 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... :-))

Oh well.

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


---
~Randy

Re: [PATCH] init: Fix printk format strings

[Johannes Weiner]

Hi,

On Thu, Oct 11, 2007 at 08:17:02AM +0200, Vegard Nossum wrote:
> This makes sure printk format strings are string literals containing no
> more than a single line.

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

Re: [PATCH] init: Fix printk format strings

[Vegard Nossum]

On 10/11/07, Johannes Weiner <hannes-kernel@saeurebad.de> wrote:
> On Thu, Oct 11, 2007 at 08:17:02AM +0200, Vegard Nossum wrote:
> > This makes sure printk format strings are string literals containing no
> > more than a single line.
>
> 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.

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