Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

[Petr Mladek]

On Mon 2022-09-05 15:07:44, Chris Down wrote:
> Hi Petr,
> 
> Thanks a lot for getting back! :-)
> 
> Any comments not explicitly addressed are acked.
> 
> Petr Mladek writes:
> > On Wed 2022-07-20 18:48:16, Chris Down wrote:
> > > In terms of technical implementation, this patch embeds a device pointer
> > > in the console struct, and registers each console using it so we can
> > > expose attributes in sysfs.
> > > 
> > > For information on the precedence and application of the new controls,
> > > see Documentation/ABI/testing/sysfs-class-console and
> > > Documentation/admin-guide/per-console-loglevel.rst.

> > > @@ -1701,12 +1806,14 @@ int do_syslog(int type, char __user *buf, int len, int source)
> > >  		break;
> > >  	/* Disable logging to console */
> > >  	case SYSLOG_ACTION_CONSOLE_OFF:
> > > +		warn_on_local_loglevel();
> > >  		if (saved_console_loglevel == LOGLEVEL_DEFAULT)
> > >  			saved_console_loglevel = console_loglevel;
> > >  		console_loglevel = minimum_console_loglevel;
> > >  		break;
> > 
> > We actually could disable logging on all consoles by setting
> > ignore_per_console_loglevel. Something like:
> > 
> > 	case SYSLOG_ACTION_CONSOLE_OFF:
> > 		if (saved_console_loglevel == LOGLEVEL_DEFAULT) {
> > 			saved_console_loglevel = console_loglevel;
> > 			saved_ignore_per_console_loglevel = ignore_per_console_loglevel;
> > 		}
> > 		console_loglevel = minimum_console_loglevel;
> > 		ignore_per_console_loglevel = true;
> > 		break;
> 
> Oh, that's very true. Thanks!
> 
> > > +		warn_on_local_loglevel();
> > 
> > I would keep it simple:
> > 
> > 		if (!ignore_per_console_loglevel)
> > 			pr_warn_once("SYSLOG_ACTION_CONSOLE_LEVEL is ignored by consoles with explicitely set per-console loglevel, see Documentation/admin-guide/per-console-loglevel\n");
> 
> My concern with this is that this will then warn on basically any first
> syslog() use, even for people who don't care about the per-console loglevel
> semantics. They will now get the warning, since by default
> ignore_per_console_loglevel isn't true -- however no per-console loglevel is
> set either, so it's not really relevant.
> 
> That's why I implemented it as warn_on_local_loglevel() checking for
> CON_LOGLEVEL, because otherwise it seems noisy for those that are not using
> the feature.

IMHO, the question is if any commonly used tool is using syslog
SYSLOG_ACTION_CONSOLE_ON/OFF these days.

It is supported by dmesg but I am not sure if anyone is really
using it. And I am not sure if anyone uses this during boot, suspend,
or so.

I think that I really should open the discussion whether to obsolete syslog
syscall in general. I am sure that it won't me possible to remove
it anytime soon, maybe it would need to stay forever. Anyway, it has
many problems because they modify global variables. And even reading
does not work well when there are more readers.

I am going to send it. Well, I would need to some time to think
about it.

In the meantime, you could either do it the conservative way or
always show it for these operations. It would be simple to fix
when anyone complains.

> > > +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> > > +			      const char *buf, size_t size)
> > > +{
> > > +	struct console *con = classdev_to_console(dev);
> > > +	ssize_t ret;
> > > +	int tmp;
> > > +
> > > +	if (!strcmp(buf, "unset") || !strcmp(buf, "unset\n")) {
> > > +		con->flags &= ~CON_LOGLEVEL;
> > > +		return size;
> > > +	}
> > > +
> > > +	ret = kstrtoint(buf, 10, &tmp);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (tmp < LOGLEVEL_EMERG || tmp > LOGLEVEL_DEBUG + 1)
> > > +		return -ERANGE;
> > > +
> > > +	if (tmp < minimum_console_loglevel)
> > > +		return -EINVAL;
> > 
> > This looks superfluous. Please, use minimum_console_loglevel
> > instead of LOGLEVEL_EMERG in the above range check.
> 
> That's fair. In which case we probably end up with one error for all cases:
> do you prefer we should return -EINVAL or -ERANGE?

I prefer -ERANGE.

> > > +
> > >  static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
> > >  				void *buffer, size_t *lenp, loff_t *ppos)
> > >  {
> > > @@ -76,6 +122,22 @@ static struct ctl_table printk_sysctls[] = {
> > >  		.extra1		= SYSCTL_ZERO,
> > >  		.extra2		= SYSCTL_TWO,
> > >  	},
> > > +	{
> > > +		.procname	= "console_loglevel",
> > > +		.data		= &console_loglevel,
> > > +		.maxlen		= sizeof(int),
> > > +		.mode		= 0644,
> > > +		.proc_handler	= printk_console_loglevel,
> > > +	},
> > > +	{
> > > +		.procname	= "default_message_loglevel",
> > > +		.data		= &default_message_loglevel,
> > > +		.maxlen		= sizeof(int),
> > > +		.mode		= 0644,
> > > +		.proc_handler	= proc_dointvec_minmax,
> > > +		.extra1		= &min_loglevel,
> > > +		.extra2		= &max_loglevel,
> > > +	},
> > 
> > Is there any chance to add this into /sys/class/console instead?
> > I mean:
> > 
> > 	/sys/class/console/loglevel
> > 	/sys/class/console/default_message_loglevel
> > 
> > It would be nice to have the things are on the same place.
> > Especially it would be nice to have the global loglevel there.
> 
> I think this one is a little complicated: on the one hand, yes, it does seem
> more ergonomic to keep everything together in /sys/class/console. On the
> other hand, this means that users can no longer use the sysctl
> infrastructure, which makes things more unwieldy than with kernel.printk.
> 
> Not really a problem with sysfs as much as a problem with userspace
> ergonomics: sysctls have a really easy way to set them at boot, but sysfs
> stuff, not so. You can hack it with systemd-tmpfiles, a boot unit, or
> similar, but the infrastructure is a lot less specialised and requires more
> work. I am worried that people may complain that that's unhelpful,
> especially since we're deprecating kernel.printk.

Good point. Let's keep the global values in /proc so that they might
be modified by sysctl.

Best Regards,
Petr