Re: [PATCH] printk: Userspace format enumeration support

On Fri, 5 Feb 2021 17:42:55 +0100
Petr Mladek [off-list ref] 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 [off-list ref]: printk co-maintainer

Thanks for Cc'ing me.

Alexey Dobriyan [off-list ref]: fs/proc maintainer
Greg Kroah-Hartman [off-list ref]: sysfs maintainer
Jason Baron [off-list ref]: dynamic_debug maintainer
Kees Cook [off-list ref]: security POV
linux-api@vger.kernel.org: 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?

quoted

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.

quoted

+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