Re: [PATCH v2 1/3] printk: introduce new macros pr_<level>_cont()

[Petr Mladek <pmladek@suse.com>]

On Wed 2022-11-30 15:50:55, Thomas Weißschuh wrote:
> On 2022-11-30 14:59+0100, Petr Mladek wrote:
> > On Fri 2022-11-25 21:33:40, Thomas Weißschuh wrote:
> >> On 2022-11-25 12:18-0800, Joe Perches wrote:
> >>> On Fri, 2022-11-25 at 20:09 +0100, Thomas Weißschuh wrote:
> >>>> These macros emit continuation messages with explicit levels.
> >>>> In case the continuation is logged separately from the original message
> >>>> it will retain its level instead of falling back to KERN_DEFAULT.
> >>>> 
> >>>> This remedies the issue that logs filtered by level contain stray
> >>>> continuation messages without context.
> >>>> 
> >>>> --- a/include/linux/printk.h
> >>>> +++ b/include/linux/printk.h
> >>>> @@ -701,6 +703,27 @@ do {									\
> >>>>  	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> >>>>  #endif
> >>>>  
> >>>> +/*
> >>>> + * Print a continuation message with level. In case the continuation is split
> >>>> + * from the main message it preserves the level.
> >>>> + */
> >>>> +
> >>>> +#define pr_emerg_cont(fmt, ...)					\
> >>>> +	printk(KERN_EMERG KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
> >>> 
> >>> Aren't this rather backwards?
> >>> KERN_CONT KERN_<LEVEL> seems to make more sense to me.
> >> 
> >> If nobody else disagrees I'll do this for v3.
> > 
> > I slightly prefer the way how it is now. IMHO, it makes it easier
> > to check the related levels in /sys/kernel/debug/printk/index/vmlinux [*]:
> > 
> > <6> kernel/power/process.c:227 thaw_kernel_threads "Restarting kernel threads ... "
> > <6,c> kernel/power/process.c:218 thaw_processes "done.\n"
> > <6> kernel/power/process.c:197 thaw_processes "Restarting tasks ... "
> > <6,c> kernel/power/process.c:176 freeze_kernel_threads "\n"
> > <6,c> kernel/power/process.c:174 freeze_kernel_threads "done."
> > <6> kernel/power/process.c:169 freeze_kernel_threads "Freezing remaining freezable tasks ... "
> > <6,c> kernel/power/process.c:140 freeze_processes "\n"
> > <6,c> kernel/power/process.c:138 freeze_processes "done."
> > <6> kernel/power/process.c:133 freeze_processes "Freezing user space processes ... "
> > <6,c> kernel/power/process.c:105 try_to_freeze_tasks "(elapsed %d.%03d seconds) "
> 
> I did not test it (will do so later) but it seems to me that the code in
> kernel/printk/index.c should do this correctly in either case. At least it
> tries to:
> 
> if (flags & LOG_CONT) {
> 	/*
> 	 * LOGLEVEL_DEFAULT here means "use the same level as the
> 	 * message we're continuing from", not the default message
> 	 * loglevel, so don't display it as such.
> 	 */
> 	if (level == LOGLEVEL_DEFAULT)
> 		seq_puts(s, "<c>");
> 		else
> 		seq_printf(s, "<%d,c>", level);
> 	} else
> 		seq_printf(s, "<%d>", level);
> 	}

Great. It makes the index consistent. I should have checked the code ;-)

I do not mind then about the ordering in the macro definitions.
It really seems to be only an implementation detail.

Best Regards,
Petr