On Fri, 5 Feb 2021 17:42:55 +0100
Petr Mladek <pmladek@xxxxxxxx> wrote:
> Hi,
>
> I would like to hear opinion from a bigger audience. It is an
> userspace interface that we might need to maintain forewer.
> Adding few more people in to CC:
>
> Steven Rostedt <rostedt@xxxxxxxxxxx>: printk co-maintainer
Thanks for Cc'ing me.
> Alexey Dobriyan <adobriyan@xxxxxxxxx>: fs/proc maintainer
> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>: sysfs maintainer
> Jason Baron <jbaron@xxxxxxxxxx>: dynamic_debug maintainer
> Kees Cook <keescook@xxxxxxxxxxxx>: security POV
> linux-api@xxxxxxxxxxxxxxx: Linux API mailing list
>
> Of course, we should also ask if this is the right approach
> for the think that you want to achieve.
>
> The motivation for this patch is that the strings printed by kernels
> are not reliable and you want a simple way to compare differences
> bethween versions. Do I get it right?
>
> See more comments below.
>
>
> Also this is yet another style how the format is displayed. We already have
>
> + console/syslog: formated by record_print_text()
> + /dev/kmsg: formatted by info_print_ext_header(), msg_print_ext_body().
> + /sys/kernel/debug/dynamic_debug/control
> + /sys/kernel/debug/tracing/printk_formats
>
> We should get some inspiration from the existing interfaces.
Interesting, because when I was looking at the original patch (looked at
the lore link before reading your reply), I thought to myself "this looks
exactly like what I did for trace_printk formats", which the above file is
where it is shown. I'm curious if this work was inspired by that?
> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index 34b7e0d2346c..0ca6e28e05d6 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -309,6 +309,17 @@
> > #define ACPI_PROBE_TABLE(name)
> > #endif
> >
> > +#ifdef CONFIG_PRINTK_ENUMERATION
> > +#define PRINTK_FMTS \
> > + .printk_fmts : AT(ADDR(.printk_fmts) - LOAD_OFFSET) { \
> > + __start_printk_fmts = .; \
> > + *(.printk_fmts) \
> > + __stop_printk_fmts = .; \
> > + }
> > +#else
> > +#define PRINTK_FMTS
> > +#endif
>
> It should be defined after #define TRACEDATA to follow the existing
> style.
>
> But honestly I am not much familiar with the sections definitions.
> I am curious why TRACE_PRINTKS() and __dyndbg are defined
> a bit different way.
>
I'm not sure what difference you mean.
> > +static int proc_pf_show(struct seq_file *s, void *v)
> > +{
> > + const struct printk_fmt_sec *ps = NULL;
> > + const char **fptr = NULL;
> > +
> > + mutex_lock(&printk_fmts_mutex);
> > +
> > + list_for_each_entry(ps, &printk_fmts_list, list) {
> > + const char *mod_name = ps_get_module_name(ps);
> > +
> > + for (fptr = ps->start; fptr < ps->end; fptr++) {
> > + seq_puts(s, mod_name);
> > + seq_putc(s, ',');
> > + seq_puts(s, *fptr);
> > + seq_putc(s, '\0');
> > + }
>
> You probably should get inspiration from t_show() in trace_printk.c.
> It handles newlines, ...
>
> Or by ddebug_proc_show(). It uses seq_escape().
>
> Anyway, there is something wrong at the moment. The output looks fine
> with cat. But "less" says that it is a binary format and the output
> is a bit messy:
Hmm, that's usually the case when lseek gets messed up. Not sure how that
happened.
>
> $> less /proc/printk_formats
> "/proc/printk_formats" may be a binary file. See it anyway?
> vmlinux,^A3Warning: unable to open an initial console.
> ^@vmlinux,^A3Failed to execute %s (error %d)
> ^@vmlinux,^A6Kernel memory protection disabled.
> ^@vmlinux,^A3Starting init: %s exists but couldn't execute it (error %d)
>
>
> That is for now. I still have to think about it. And I am also curious
> about what others thing about this idea.
>
I'm not against the idea. I don't think it belongs in /proc. Perhaps
debugfs is a better place to put it.
-- Steve
