Linus, here's an updated version of the patch implementing %pf(%p) that
implements your typechecking macro and cookie idea we discussed. I haven't done
any additional checking like the whitelist I was talking about - this feels
sufficient to me for now, but I'm happy to revisit it if it comes up.
It's been tested lightly.
I would _really_ like to be able to pass integer arguments as well - if we want
to go ahead with converting existing %p extensions to this it would be really
helpful, and I was wondering if you had any thoughts on that.
I'm not sure if we can depend on integers and pointers being passed the same way
at the ABI level - I got some screaming when I floated the idea, but after
looking at actual ABI docs I couldn't find any archs that were _that_ crazy. The
right way to do this would seem to be with libffi - it supports all Linux
architectures and is designed exactly for this, and compiles down to practically
nothing (under a kilobyte) with the features we don't need turned off. If
pulling that into the kernel is something you'd be ok with, I can look more at
it.
It'll be a bit before I'm ready to mail out the next version of the full
printbuf patch series. I decided to go ahead with converting all our existing
pretty-printers in lib/vsprintf.c to printbuf style:
- it makes the code _vastly_ easier to read and understand by separating out
format string parsing from the pretty-printers, and giving actual named
arguments to the pretty-printers
- if we _do_ decide to go ahead with the treewide conversion (and I think we
should, since it'll give us typechecking that we don't have currently), we'll
want to do them all at once.
-- >8 -
This implements a new %p format string extensions for passing a pretty
printer and its arguments to printk, which will then be inserted into
the formatted output.
A pretty-printer is a function that takes as its first argument a
pointer to a struct printbuf, and then zero or more additional pointer
arguments - these being the objects to format and print.
The arguments to the pretty-printer function are denoted in the format
string by %p, i.e
%pf() foo0_to_text(struct printbuf *out)
%pf(%p) foo1_to_text(struct printbuf *out, struct foo *)
%pf(%p,%p) foo2_to_text(struct printbuf *out, struct foo *)
We'd also like to eventually support non pointer arguments - in
particular, integers - but this will probably require libffi.
Typechecking is accomplished with the CALL_PP macro, which verifies that
the arguments passed to sprintf match the types of the pp-function
arguments, and passes a struct with a cookie to sprintf so that sprintf
can verify that the CALL_PP() macro was used.
Full example:
static void foo_to_text(struct printbuf *out, struct foo *foo)
{
prt_printf(out, "bar=%u baz=%u", foo->bar, foo->baz);
}
printf("%pf(%p)", CALL_PP(foo_to_text, foo));
The goal is to replace most of our %p format extensions with this
interface, and to move pretty-printers out of the core vsprintf.c code -
this will get us better organization and better discoverability (you'll
be able to cscope to pretty printer calls!), as well as eliminate a lot
of dispatch code in vsprintf.c.
Signed-off-by: Kent Overstreet <[email protected]>
Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Petr Mladek <[email protected]>
---
Documentation/core-api/printk-formats.rst | 22 +++++
include/linux/printbuf.h | 26 ++++++
lib/test_printf.c | 27 ++++++
lib/vsprintf.c | 100 +++++++++++++++++++++-
4 files changed, 172 insertions(+), 3 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 5e89497ba3..4f4a35b3aa 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -625,6 +625,28 @@ Examples::
%p4cc Y10 little-endian (0x20303159)
%p4cc NV12 big-endian (0xb231564e)
+Calling a pretty printer function
+---------------------------------
+
+::
+
+ %pf(%p) pretty printer function taking one argument
+ %pf(%p,%p) pretty printer function taking two arguments
+
+For calling generic pretty printers. A pretty printer is a function that takes
+as its first argument a pointer to a printbuf, and then zero or more additional
+pointer arguments. For example:
+
+ void foo_to_text(struct printbuf *out, struct foo *foo)
+ {
+ pr_buf(out, "bar=%u baz=%u", foo->bar, foo->baz);
+ }
+
+ printf("%pf(%p)", CALL_PP(foo_to_text, foo));
+
+Note that a pretty-printer may not sleep if called from printk(). If called from
+prt_printf() or sprintf() there are no such restrictions.
+
Thanks
======
diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
index 8186c447ca..189828d48d 100644
--- a/include/linux/printbuf.h
+++ b/include/linux/printbuf.h
@@ -119,4 +119,30 @@ static inline void prt_hex_byte_upper(struct printbuf *out, u8 byte)
.size = _size, \
})
+/*
+ * This is used for the %pf(%p) sprintf format extension, where we pass a pretty
+ * printer and arguments to the pretty-printer to sprintf
+ *
+ * Instead of passing a pretty-printer function to sprintf directly, we pass it
+ * a pointer to a struct call_pp, so that sprintf can check that the magic
+ * number is present, which in turn ensures that the CALL_PP() macro has been
+ * used in order to typecheck the arguments to the pretty printer function
+ *
+ * Example usage:
+ * sprintf("%pf(%p)", CALL_PP(prt_bdev, bdev));
+ */
+struct call_pp {
+ unsigned long magic;
+ void *fn;
+};
+
+#define PP_TYPECHECK(fn, ...) \
+ ({ while (0) fn((struct printbuf *) NULL, ##__VA_ARGS__); })
+
+#define CALL_PP_MAGIC (unsigned long) 0xce0b92d22f6b6be4
+
+#define CALL_PP(fn, ...) \
+ (PP_TYPECHECK(fn, ##__VA_ARGS__), \
+ &((struct call_pp) { CALL_PP_MAGIC, fn })), ##__VA_ARGS__
+
#endif /* _LINUX_PRINTBUF_H */
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 07309c45f3..af0c1c2d2d 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -9,6 +9,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/printk.h>
+#include <linux/printbuf.h>
#include <linux/random.h>
#include <linux/rtc.h>
#include <linux/slab.h>
@@ -783,6 +784,31 @@ test_pointer(void)
fourcc_pointer();
}
+static void printf_test_fn_0(struct printbuf *out)
+{
+ prt_str(out, "0");
+}
+
+static void printf_test_fn_1(struct printbuf *out, void *p)
+{
+ int *i = p;
+
+ prt_printf(out, "%i", *i);
+}
+
+static void __init
+test_fn(void)
+{
+ int i = 1;
+
+ test("0", "%pf()", CALL_PP(printf_test_fn_0));
+ test("1", "%pf(%p)", CALL_PP(printf_test_fn_1, &i));
+ /*
+ * Not tested, so we don't fail the build with -Werror:
+ */
+ //test("1", "%(%p)", printf_test_fn, &i);
+}
+
static void __init selftest(void)
{
alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
@@ -794,6 +820,7 @@ static void __init selftest(void)
test_number();
test_string();
test_pointer();
+ test_fn();
kfree(alloced_buffer);
}
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7b24714674..eb57524c5d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -436,7 +436,8 @@ enum format_type {
FORMAT_TYPE_UINT,
FORMAT_TYPE_INT,
FORMAT_TYPE_SIZE_T,
- FORMAT_TYPE_PTRDIFF
+ FORMAT_TYPE_PTRDIFF,
+ FORMAT_TYPE_FN,
};
struct printf_spec {
@@ -2520,8 +2521,14 @@ int format_decode(const char *fmt, struct printf_spec *spec)
return ++fmt - start;
case 'p':
- spec->type = FORMAT_TYPE_PTR;
- return ++fmt - start;
+ fmt++;
+ if (fmt[0] == 'f' &&
+ fmt[1] == '(') {
+ fmt += 2;
+ spec->type = FORMAT_TYPE_FN;
+ } else
+ spec->type = FORMAT_TYPE_PTR;
+ return fmt - start;
case '%':
spec->type = FORMAT_TYPE_PERCENT_CHAR;
@@ -2602,6 +2609,67 @@ set_precision(struct printf_spec *spec, int prec)
}
}
+static void call_prt_fn(struct printbuf *out, struct call_pp *call_pp, void **fn_args, unsigned nr_args)
+{
+ typedef void (*printf_fn_0)(struct printbuf *);
+ typedef void (*printf_fn_1)(struct printbuf *, void *);
+ typedef void (*printf_fn_2)(struct printbuf *, void *, void *);
+ typedef void (*printf_fn_3)(struct printbuf *, void *, void *, void *);
+ typedef void (*printf_fn_4)(struct printbuf *, void *, void *, void *, void *);
+ typedef void (*printf_fn_5)(struct printbuf *, void *, void *, void *, void *, void *);
+ typedef void (*printf_fn_6)(struct printbuf *, void *, void *, void *, void *, void *, void *);
+ typedef void (*printf_fn_7)(struct printbuf *, void *, void *, void *, void *, void *, void *, void *);
+ typedef void (*printf_fn_8)(struct printbuf *, void *, void *, void *, void *, void *, void *, void *, void *);
+ void *fn;
+ unsigned i;
+
+ if (check_pointer(out, call_pp))
+ return;
+
+ if (call_pp->magic != CALL_PP_MAGIC) {
+ error_string(out, "bad pretty-printer magic");
+ return;
+ }
+
+ fn = call_pp->fn;
+ if (check_pointer(out, fn))
+ return;
+
+ for (i = 0; i < nr_args; i++)
+ if (check_pointer(out, fn_args[i]))
+ return;
+
+ switch (nr_args) {
+ case 0:
+ ((printf_fn_0)fn)(out);
+ break;
+ case 1:
+ ((printf_fn_1)fn)(out, fn_args[0]);
+ break;
+ case 2:
+ ((printf_fn_2)fn)(out, fn_args[0], fn_args[1]);
+ break;
+ case 3:
+ ((printf_fn_3)fn)(out, fn_args[0], fn_args[1], fn_args[2]);
+ break;
+ case 4:
+ ((printf_fn_4)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3]);
+ break;
+ case 5:
+ ((printf_fn_5)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4]);
+ break;
+ case 6:
+ ((printf_fn_6)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5]);
+ break;
+ case 7:
+ ((printf_fn_7)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5], fn_args[6]);
+ break;
+ case 8:
+ ((printf_fn_8)fn)(out, fn_args[0], fn_args[1], fn_args[2], fn_args[3], fn_args[4], fn_args[5], fn_args[6], fn_args[7]);
+ break;
+ }
+}
+
/**
* prt_vprintf - Format a string, outputting to a printbuf
* @out: The printbuf to output to
@@ -2665,6 +2733,32 @@ void prt_vprintf(struct printbuf *out, const char *fmt, va_list args)
fmt++;
break;
+ case FORMAT_TYPE_FN: {
+ unsigned nr_args = 0;
+ void *fn_args[8];
+ void *fn = va_arg(args, void *);
+
+ while (*fmt != ')') {
+ if (nr_args) {
+ if (fmt[0] != ',')
+ goto out;
+ fmt++;
+ }
+
+ if (fmt[0] != '%' || fmt[1] != 'p')
+ goto out;
+ fmt += 2;
+
+ if (WARN_ON_ONCE(nr_args == ARRAY_SIZE(fn_args)))
+ goto out;
+ fn_args[nr_args++] = va_arg(args, void *);
+ }
+
+ call_prt_fn(out, fn, fn_args, nr_args);
+ fmt++; /* past trailing ) */
+ break;
+ }
+
case FORMAT_TYPE_PERCENT_CHAR:
__prt_char(out, '%');
break;
--
2.36.1
On Mon, Jun 27, 2022 at 8:24 PM Kent Overstreet
<[email protected]> wrote:
>
> Linus, here's an updated version of the patch implementing %pf(%p) that
> implements your typechecking macro and cookie idea we discussed
This still has a fundamental problem: you insist on that double '%p' form. Why?
That makes no sense when to a user, there is only *one* value.
A user will ever see two values, because you should probably never use
"CALL_PP()" (which is a horrible name anyway) directly.
You'd presumably either use a per-type "print_dentry()" kind of
function, or it would all be hidden by a macro that does _Generic() to
match up the right pretty-printing function based on the type of the
pointer.
IOW, your "CALL_PP()" macro is something disgusting that normally
should never be visible to any user. The real use might be something
like
printk("%pf\n", pretty_print(dentry));
where all the magic is hidden behind that "pretty_print()" macro.
End result: that "%pf{%p}" format you have tried before is entirely broken.
And that's what I tried to tell you earlier. There should be only one
pointer, and there should be just a "%pf". Having two '%p' things is
just wrong. It makes no sense to the actual use-case that a user would
ever see.
And there is only one pointer argument in the ARGV array too, because
the way you'd actually then implement pretty_print() would most
naturally be something like this:
#include <stdio.h>
#define __PP_MAGIC (0xdeadbeef)
struct pretty_print_struct {
unsigned long magic;
void *fn;
void *p;
};
// type warning *and* link-time failure
extern int not_a_pointer;
extern int pretty_print_int(int *);
extern int pretty_print_long_long(long long *);
#define __PP_FN(ptr) _Generic(*(ptr), \
int: pretty_print_int, \
long long: pretty_print_long_long, \
default: not_a_pointer )
#define __PP_CHECK(fn,ptr) \
__PP_MAGIC+(__typeof__ ((fn)(ptr)))0, fn, ptr
#define pretty_fn(fn,ptr) &(struct pretty_print_struct) \
{ __PP_CHECK(__PP_FN(ptr), ptr) }
#define pretty_print(ptr) pretty_fn(__PP_FN(ptr), ptr)
void test_me(int val)
{
printf("%pf\n", pretty_print(&val));
}
Note how at no point does something like the above want two '%p'
entries in the printf string, and in the concept of that line that the
programmer actually uses:
printf("%pf\n", pretty_print(&val));
there is only ONE value as far as the user is concerned.. Ergo: there
should be only one '%p'. And the compiler only sees one pointer too,
because we hide all the metadata as a pointer to that metadata
structure.
And obviously, the above is silly: the above example is doing
ridiculous types like 'int' and 'long long' that don't need this. For
the kernel, we'd do things like dentry pointers etc.
And maybe not all users would use that "_Generic" thing to pick a
printout function based on type, and you could just have special-case
things, and sure, you can then use
printf("%pf\n", pretty_fn(mny_dentry_print, file->f_dentry));
and it would do the obvious thing, but it would still be just one
pointer as far as "printf" would be concerned.
(I did test all of the above except for the 'dentry' case in a
compiler to see that I got the syntax right, but then I ended up
editing it later in the MUA, so I might have screwed something up
after testing)
Linus
On Tue, Jun 28, 2022 at 10:29:05AM -0700, Linus Torvalds wrote:
> On Tue, Jun 28, 2022 at 10:23 AM Kent Overstreet
> <[email protected]> wrote:
> >
> > - We need to be able to specify the pretty-printer function, and not just have
> > one per type
>
> I even showed you how toi do that, you just deleted it.
>
> Anyway, I'm done with this discussion. I think this is all pointless.
> I think you are making things worse, you are making things more
> complex, and I don't think this is a direction we want to take things
> in.
Linus, I'm ok with this not working out. Really. Trying to do something
difficult and new _often_ ends up in the result being too complicated and ugly
to live.
But that doesn't mean I'm not going to try, and if it's an idea I think has
potential I'll keep trying because it often takes a couple attempts and a couple
rewrites.
Anyways, I've been listening to your feedback (and honestly pretty appreciative
of it, it beats the hell out of what I was getting on the list). I'm sorry if
you feel like I wasn't listening or taking into account _everything_ you had to
say, this is a problem I've been thinking about for quite some time and I've got
a number of constraints I'm trying to fit.
And I apologize, it looks like I did miss something - I went back and reread and
saw you also had pretty_fn() in your example. So sorry for skipping past that,
my bad.
On Mon, Jun 27, 2022 at 09:46:46PM -0700, Linus Torvalds wrote:
> // type warning *and* link-time failure
> extern int not_a_pointer;
>
> extern int pretty_print_int(int *);
> extern int pretty_print_long_long(long long *);
>
> #define __PP_FN(ptr) _Generic(*(ptr), \
> int: pretty_print_int, \
> long long: pretty_print_long_long, \
> default: not_a_pointer )
>
> #define __PP_CHECK(fn,ptr) \
> __PP_MAGIC+(__typeof__ ((fn)(ptr)))0, fn, ptr
>
> #define pretty_fn(fn,ptr) &(struct pretty_print_struct) \
> { __PP_CHECK(__PP_FN(ptr), ptr) }
>
> #define pretty_print(ptr) pretty_fn(__PP_FN(ptr), ptr)
>
> void test_me(int val)
> {
> printf("%pf\n", pretty_print(&val));
> }
Your version is nicer for the simpler cases, but it's not general enough for
what I'm looking at.
- We need to be able to specify the pretty-printer function, and not just have
one per type
From the commit message for my patch converting %p[iI] pretty printers to
printbuf style pretty printers:
%piS -> prt_sockaddr_zeropad
%pIS -> prt_sockaddr
%pISc -> prt_sockaddr_compressed
%pISfc -> prt_sockaddr_flow_compressed
%pISp -> prt_sockaddr_port
%pISpc -> prt_sockaddr_port_compressed
%pISpsc -> prt_sockaddr_port_scope_compressed
%pISpfc -> prt_sockaddr_port_flow_compressed
%pISpfsc -> prt_sockaddr_port_scope_flow_compressed
That's just sockaddrs, and I limited it to creating functions for the printk
formats that are actually currently in use.
What we _really_ want is just to have a separate flags argument to prt_sockaddr,
but in the current code only pointer arguments are supported.
- We need pretty-printer functions that take variable numbers of arguments, and
not just for a flags argument
In my OOM report patch series, I'm doing
printk("%pf()", CALL_PP(slabs_to_text));
printk("%pf()", CALL_PP(shrinkers_to_text));
We want slabs_to_text and shrinkers_to_text to be pretty-printers instead of
just calling printk directly so that procfs/sysfs/whatever the current fashion
is can also dump the same report.
And in bcachefs, I've got a bunch of pretty-printers that need a pointer to the
filesystem object in addition to a pointer to the object they're printing so
that they can decode all kinds of things. Pretty sure this is going to come up
elsewhere (other filesystems, for sure).
Also:
I want the pretty-printer function to be specified explicitly, so that when
reading the code we can cscope to it. When we're doing a function call, even
indirectly, _I want the name of that function in front of me_ and I think that
is one of the major advantages of this approach vs. our current %p extensions.
BUT
Maybe we can get rid of specifying the arguments in the format string, and have
it be just %pf instead of %pf(...).
The struct call_pp needs to handle a variable number of arguments, not just one
as in your example, and I really do want it to handle integer arguments, and not
just pointer arguments.
So here's the $20 million question: Can we, safely, on all archictectures, treat
all arguments as ulongs as long as the actual function arguments are either
pointers or integers not bigger than a ulong? Or are there crazy architectures
out there where passing pointers is different from passing integers in the ABI?
We'd also really like to be able to pass u64s too - prt_u64() is now a standard
pretty-printer, along with prt_u64_human_readable()!
Because yeah fundamentally the reason for the %pf(%p,%llu) thing is that we know
how to pass those types through varargs to sprintf, and libffi is a thing so we
know that the future we'll be able to do constructed function calls including
with u64 arguments, so this seems solvable.
But maybe serializing them before the call in struct call_pp isn't actually any
harder, and actually it gets around the fact that C promotes all ints smaller
than ints to ints in varargs, so that's nice.
I need to think about this some more.
On Tue, Jun 28, 2022 at 10:23 AM Kent Overstreet
<[email protected]> wrote:
>
> - We need to be able to specify the pretty-printer function, and not just have
> one per type
I even showed you how toi do that, you just deleted it.
Anyway, I'm done with this discussion. I think this is all pointless.
I think you are making things worse, you are making things more
complex, and I don't think this is a direction we want to take things
in.
Linus