On Fri 2023-07-07 06:29:11, Breno Leitao wrote:
> Create a new netconsole runtime option that prepends the kernel version in
> the netconsole message. This is useful to map kernel messages to kernel
> version in a simple way, i.e., without checking somewhere which kernel
> version the host that sent the message is using.
>
> If this option is selected, then the "<release>," is prepended before the
> netconsole message. This is an example of a netconsole output, with
> release feature enabled:
>
> 6.4.0-01762-ga1ba2ffe946e;12,426,112883998,-;this is a test
>
> Calvin Owens send a RFC about this problem in 2016[1], but his
> approach was a bit more intrusive, changing the printk subsystem. This
> approach is lighter, and just append the information in the last mile,
> just before netconsole push the message to netpoll.
Thanks a lot for solving this on the netconsole level. The extended
console format is used also for /dev/kmsg. Which is then used by
systemd journal, dmesg, and maybe other userspace tools. Any format
changes might break these tools.
I have glanced over the patch and have two comments.
> @@ -254,6 +267,11 @@ static ssize_t extended_show(struct config_item *item, char *buf)
> return snprintf(buf, PAGE_SIZE, "%d\n", to_target(item)->extended);
> }
>
> +static ssize_t release_show(struct config_item *item, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "%d\n", to_target(item)->release);
I have learned recently that sysfs_emit() was preferred over snprintf() in the
_show() callbacks.
> +}
> +
> static ssize_t dev_name_show(struct config_item *item, char *buf)
> {
> return snprintf(buf, PAGE_SIZE, "%s\n", to_target(item)->np.dev_name);
> @@ -366,6 +389,38 @@ static ssize_t enabled_store(struct config_item *item,
> return err;
> }
>
> +static ssize_t release_store(struct config_item *item, const char *buf,
> + size_t count)
> +{
> + struct netconsole_target *nt = to_target(item);
> + int release;
> + int err;
> +
> + mutex_lock(&dynamic_netconsole_mutex);
> + if (nt->enabled) {
> + pr_err("target (%s) is enabled, disable to update parameters\n",
> + config_item_name(&nt->item));
> + err = -EINVAL;
> + goto out_unlock;
> + }
> +
> + err = kstrtoint(buf, 10, &release);
> + if (err < 0)
> + goto out_unlock;
> + if (release < 0 || release > 1) {
> + err = -EINVAL;
> + goto out_unlock;
> + }
You might consider using:
bool enabled;
err = kstrtobool(buf, &enabled);
if (err)
goto unlock;
It accepts more input values, for example, 1/0, y/n, Y/N, ...
Well, I see that kstrtoint() is used also in enabled_store().
It might be confusing when "/enabled" supports only "1/0"
and "/release" supports more variants.
> +
> + nt->release = release;
> +
> + mutex_unlock(&dynamic_netconsole_mutex);
> + return strnlen(buf, count);
> +out_unlock:
> + mutex_unlock(&dynamic_netconsole_mutex);
> + return err;
> +}
> +
Best Regards,
Petr
