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.
The overall logic looks good to me. I finally have good feeling that
the behavior is "easy" to understand.
+ */
+#define CON_LOGLEVEL (128) /* Level set locally for this console */
I would write:
#define CON_LOGLEVEL (128) /* Local (per-console) loglevel is set. */
Alternatively we could avoid the flag completely. The per-console
loglevel is set when con->level > 0. A valid value must never
be below CONSOLE_MIN_LOGLEVEL which is 1. And it is perfectly fine
to say that 0 or -1 is not a valid loglevel. The same effect could
be achieved by disabling the console completely.
I do not have strong opinion. The flag has obvious meaning and might
make the code better readable. On the other hand, it adds an extra
code and complexity.
I slightly prefer to do it without the flag.
Anyway, if we add the new flag, we should also show it in
/proc/consoles, see fs/proc/consoles.c.
+static int console_effective_loglevel(const struct console *con,
+ enum loglevel_source *source)
+{
+ enum loglevel_source lsource;
+ int level;
+
+ if (ignore_loglevel) {
+ lsource = LLS_IGNORE_LOGLEVEL;
+ level = CONSOLE_LOGLEVEL_MOTORMOUTH;
+ goto out;
+ }
+
+ if (!ignore_per_console_loglevel &&
+ (con && (con->flags & CON_LOGLEVEL))) {
+ lsource = LLS_LOCAL;
+ level = con->level;
+ goto out;
+ }
+
+ lsource = LLS_GLOBAL;
+ level = console_loglevel;
+
+out:
+ *source = lsource;
+ return level;
+}
It might be a matter of taste. But I would probably do it the
following way (note that these would not be used in
boot_delay_msec()):
static int console_effective_loglevel(const struct console *con)
{
enum loglevel_source source;
int level;
if (WARN_ON_ONCE(!con))
return;
source = console_effective_loglevel_source(con);
switch (source) {
case LLS_IGNORE_LOGLEVEL:
level = CONSOLE_LOGLEVEL_MOTORMOUTH;
break;
case LSS_LOCAL:
level = con->level;
break;
case LSS_GLOBAL:
level = console_loglevel;
break;
default:
pr_warn("Unhandled console loglevel source: %d, source);
level = default_console_loglevel;
break;
}
return level;
}
static const char *console_effective_loglevel_source_str(const struct *con)
{
enum loglevel_source source;
const char *str;
if (WARN_ON_ONCE(!con))
return;
source = console_effective_loglevel_source(con);
switch (source) {
case LLS_IGNORE_LOGLEVEL:
str = "ignore_loglevel";
break;
case LSS_LOCAL:
str = "local"
break;
case LSS_GLOBAL:
str = "global";
break;
default:
pr_warn("Unhandled console loglevel source: %d, source);
str = "unknown";
break;
}
return str;
}
static enum loglevel_source
console_effective_loglevel_source(const struct console *con)
{
if (WARN_ON_ONCE(!con))
return;
if (ignore_loglevel)
return LLS_IGNORE_LOGLEVEL;
if (con->flags & CON_LOGLEVEL && !ignore_per_console_loglevel))
return LLS_LOCAL;
return LLS_GLOBAL;
}
It looks like a bit cleaner and better separated (layered) logic.
There is no need to define "enum loglevel_source" variable when
the caller is interested only into the loglevel value.
The advantage of console_effective_loglevel_source_str() is that it
always returns a valid string. It prevents a potential out-of-bound
access to loglevel_source_names[].
@@ -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;
+ 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");
+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.
+
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.