Re: [PATCH] WARN(): add a \n to the message printk

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Mon, 15 Jun 2009, Linus Torvalds wrote:
> > 
> > On Mon, 15 Jun 2009, Arjan van de Ven wrote:
> > >  
> > > -	if (args)
> > > +	if (args) {
> > >  		vprintk(args->fmt, args->args);
> > > +		printk("\n");
> > > +	}
> > 
> > I really don't like this. What if the format already does have a '\n'? And 
> > what if some other CPU is printing at the same time?
> > 
> > I'd almost be open to adding a "flags" field to vprintk, and allow setting 
> > things like "finish line with \n" there. Or perhaps even better, have a 
> > "vprintk_line()" function that does it with no dynamic flags. Maybe make 
> > it static, and move all these panic helper funtions into kernel/printk.c 
> > (since this really is a special case).	
> > 
> > I dunno. I'm just throwing out suggestions. I just don't think the above 
> > patch is very nice.
> 
> Oh, I actually think I have a preference.
> 
> I think we should _always_ cause a line break at the beginning of a new 
> line, unless the new printk() starts with a KERN_CONT thing.
> 
> Right now KERN_CONT is "", but we could easily make it an explicit 
> "loglevel" thing. Like this.
> 
> NOTE! This is, of course, totally untested. And we're bound to have 
> continuation printk's that don't have the KERN_CONT at front, and need 
> them added, but I think this is generally a saner model than what we have 
> now, or your suggested explicit addition of '\n'.
> 
> Basically, it tries to guarantee that new messages always get a newline, 
> unless they _explicitly_ say that they don't want one. Doesn't that make 
> sense?
> 
> 		Linus
> 
> ---
>  include/linux/kernel.h |    2 +-
>  kernel/printk.c        |   16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 883cd44..066bb1e 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -102,7 +102,7 @@ extern const char linux_proc_banner[];
>   * line that had no enclosing \n). Only to be used by core/arch code
>   * during early bootup (a continued line is not SMP-safe otherwise).
>   */
> -#define	KERN_CONT	""
> +#define	KERN_CONT	"<c>"
>  
>  extern int console_printk[];
>  
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 5052b54..6f416fd 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -691,7 +691,21 @@ asmlinkage int vprintk(const char *fmt, va_list args)
>  	 * Copy the output into log_buf.  If the caller didn't provide
>  	 * appropriate log level tags, we insert them here
>  	 */
> -	for (p = printk_buf; *p; p++) {
> +	p = printk_buf;
> +
> +	/* Are we continuing a previous printk? */
> +	if (!new_text_line) {
> +		if (!memcmp(p, KERN_CONT, 3)) {
> +			/* We intended to do that continued printk! */
> +			p += 3;
> +		} else {
> +			/* Force a line break */
> +			emit_log_char('\n');
> +			new_text_line = 1;
> +		}
> +	}
> +

Nice idea ...

Puts some pressure on current intentionally 'naked' printks (there's 
still a few of them) - but that's OK, it's not like KERN_CONT (or 
pr_cont()) is that hard to add.

( Plus many of our boot printks (where most of the 'naked' printks 
  are currently occuring) are development leftovers and should 
  really be removed, so it's good to shake them up a bit. )

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/