Re: [PATCH v6 10/11] printk: Deprecate the kernel.printk sysctl interface

[Petr Mladek <pmladek@suse.com>]

On Mon 2024-10-28 16:45:59, Chris Down wrote:
> The kernel.printk sysctl interface is deprecated in favour of more
> granular and clearer sysctl interfaces for controlling loglevels, namely
> kernel.console_loglevel and kernel.default_message_loglevel.
> 
> kernel.printk has a number of fairly significant usability problems:
> 
> - It takes four values in a specific order, which is not intuitive.
>   Speaking from personal experience, even people working on printk
>   sometimes need to look up the order of these values, which doesn't
>   suggest much in the way of perspicuity.
> - There is no validation on the input values, so users can set them to
>   invalid or nonsensical values without receiving any errors or
>   warnings. This can lead to unpredictable behaviour.
> - One of the four values, default_console_loglevel, is not used in the
>   kernel (see below), making it redundant and potentially confusing.
> - Overall, the interface is complex, hard to understand, and combines
>   multiple controls into a single sysctl, which is not ideal. It should
>   be separated into distinct controls for clarity.
> 
> Setting kernel.printk now calls printk_sysctl_deprecated, which emits a
> pr_warn. The warning informs users about the deprecation and suggests
> using the new sysctl interfaces instead.
> 
> By deprecating kernel.printk, we aim to:
> 
> - Encourage users to adopt the new, dedicated sysctl interfaces
>   (kernel.console_loglevel and kernel.default_message_loglevel), which
>   are more straightforward and easier to understand.
> - Improve input validation and error handling, ensuring that users
>   receive appropriate feedback when setting invalid values.
> - Simplify the configuration of loglevels by exposing only the controls
>   that are necessary and relevant.
> 
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -7,6 +7,7 @@
>  #include <linux/printk.h>
>  #include <linux/capability.h>
>  #include <linux/ratelimit.h>
> +#include <linux/console.h>
>  #include "internal.h"
>  
>  static const int ten_thousand = 10000;
> @@ -46,13 +47,24 @@ static int printk_console_loglevel(const struct ctl_table *table, int write,
>  	return 0;
>  }
>  
> +static int printk_sysctl_deprecated(const struct ctl_table *table, int write,
> +				    void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	int res = proc_dointvec(table, write, buffer, lenp, ppos);
> +
> +	if (write)
> +		pr_warn_once("printk: The kernel.printk sysctl is deprecated. Consider using kernel.console_loglevel or kernel.default_message_loglevel instead.\n");
> +
> +	return res;
> +}
> +
>  static struct ctl_table printk_sysctls[] = {
>  	{
>  		.procname	= "printk",
>  		.data		= &console_loglevel,
>  		.maxlen		= 4*sizeof(int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> +		.proc_handler	= printk_sysctl_deprecated,

I would prefer to follow the existing naming scheme for
custom modifications of proc_dointvec() and call it
"proc_dointvec_printk_deprecated".

>  	},
>  	{
>  		.procname	= "printk_ratelimit",

With this cosmetic change:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr