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

[Thomas Weißschuh]

On 2022-11-30 15:23+0100, Petr Mladek wrote:
> On Fri 2022-11-25 20:09:46, 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.
>> 
>> Suggested-by: Petr Mladek <pmladek@xxxxxxxx>
>> Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
>> ---
>>  include/linux/printk.h | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>> 
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 8c81806c2e99..8f564c38f121 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -537,6 +537,8 @@ struct pi_entry {
>>   * This macro expands to a printk with KERN_CONT loglevel. It should only be
>>   * used when continuing a log message with no newline ('\n') enclosed. Otherwise
>>   * it defaults back to KERN_DEFAULT loglevel.
>> + *
>> + * Use the dedicated pr_<level>_cont() macros instead.
>>   */
>>  #define pr_cont(fmt, ...) \
>>  	printk(KERN_CONT fmt, ##__VA_ARGS__)
>> @@ -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__)
>> +#define pr_alert_cont(fmt, ...)					\
>> +	printk(KERN_ALERT KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_crit_cont(fmt, ...)					\
>> +	printk(KERN_CRIT KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_err_cont(fmt, ...)					\
>> +	printk(KERN_ERR KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_warn_cont(fmt, ...)					\
>> +	printk(KERN_WARN KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_notice_cont(fmt, ...)					\
>> +	printk(KERN_NOTICE KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_info_cont(fmt, ...)					\
>> +	printk(KERN_INFO KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
>> +/* no pr_debug_ratelimited, it doesn't make sense with CONFIG_DYNAMIC_DEBUG. */
> 
> I guess that you wanted to write "pr_debug_cont".

Indeed.

> Also I am not sure what you mean with "doesn't make sense". IMHO, it
> might  make sense. But it would be hard to use and error prone
> with CONFIG_DYNAMIC_DEBUG.
> 
> And more importantly, it probably would not work properly. If I get
> it corretly the dynamic debug messages are printed by the wrapper:
> 
> void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
> {
> [...]
> 	vaf.fmt = fmt;
> 	vaf.va = &args;
> 
> 	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
> [...]
> 
> This clearly does not support KERN_CONT in "fmt".

Good point.

My doubt was more that it would force users to know which message
continuations belong together and always enable all of them together with
dynamic debug.
Which would be very errorprone and annoying to use.

But if it doesn't work at all that's an even stronger point.

> I suggest to either remove the comment completely. Or write something
> like:
> 
> /* no pr_debug_cont(), can't be supported easily with CONFIG_DYNAMIC_DEBUG */

What about:

/* no pr_debug_cont(), it's errorprone to use
 * and can't be supported easily with CONFIG_DYNAMIC_DEBUG */

Thomas