Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 21 Jun 2022 03:51:59 -0400
From:   Kent Overstreet <kent.overstreet@gmail.com>
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org, pmladek@suse.com,
        rostedt@goodmis.org, enozhatsky@chromium.org, willy@infradead.org
Subject: Re: [PATCH v4 05/34] vsprintf: %pf(%p)
Message-ID: <20220621075159.m67qzftqulvphivw@moria.home.lan>
References: <20220620004233.3805-1-kent.overstreet@gmail.com>
 <20220620004233.3805-6-kent.overstreet@gmail.com>
 <f9224687-ce0c-b41b-f158-1b679a70c2d5@rasmusvillemoes.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f9224687-ce0c-b41b-f158-1b679a70c2d5@rasmusvillemoes.dk>
Precedence: bulk

On Tue, Jun 21, 2022 at 09:04:38AM +0200, Rasmus Villemoes wrote:
> On 20/06/2022 02.42, Kent Overstreet wrote:
> > +Note that a pretty-printer may not sleep, if called from printk(). If called
> > +from pr_buf() or sprintf() there are no such restrictions.
> 
> I know what you're trying to say, but if the sprintf() call itself is
> from a non-sleepable context this is obviously not true. So please just
> make the rule "A pretty-printer must not sleep.". That's much simpler
> and less error-prone. Otherwise I guarantee you that somebody is going
> to add a sleeping pretty-printer for their own need, use it in a couple
> of safe places, and then somebody wants to add a printk() in that driver
> and sees "hey, I can get all this state dumped very easily with this
> pretty-printer".

Kernel programmers are used to having to consider the context they're in and
what the functions they're calling might do, a pretty-printer being called
indirectly via sprintf() is absolutely no different. 

> >  struct printf_spec {
> > @@ -2520,7 +2521,16 @@ int format_decode(const char *fmt, struct printf_spec *spec)
> >  		return ++fmt - start;
> >  
> >  	case 'p':
> > -		spec->type = FORMAT_TYPE_PTR;
> > +		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_FN;
> >  		return ++fmt - start;
> 
> NAK. Don't implement something that will never be tested nor used.
> There's not a snowball's chance in hell that we'll ever build the kernel
> without -Wformat.

We're not stopping here. Matthew is taking this to WG14 and I'll be working on
adding this functionality to glibc next, and %() is the syntax we intend to take
to the working group.

But the working group is naturally going to want to see that a working
implementation of it exists.

> Sorry, but this is way too ugly, and the prospect of at some point in
> the future invoking libffi to do something even naster... eww. We do not
> need more functions with completely generic prototypes with no
> typechecking and making it extremely hard to teach one of our static
> analyzers (smatch has some %pX checking) to do that typechecking.
> 
> There are at least two ways you can achieve this passing of a variable
> number of arguments with proper types.
> 
> (1) Each pretty-printer comes with a struct wrapping up its real
> arguments and a macro for creating a compound literal passing those
> arguments.
> 
> struct foo_pp {
>   void (*func)(struct printbuf *pb, void *ctx); /* always first */
>   int x;
>   long y;
> };
> void foo_pp(struct printbuf *pb, void *ctx)
> {
>   struct foo_pp *f = ctx;
>   pr_printf(pb, "%d %ld", f->x, f->y);
> }
> 
> #define FOO_PP(_x, _y) (struct foo_pp){.func = foo_pp, .x = (_x), .y = (_y)}
> 
> printk("bla bla %pf\n", &FOO_PP(aa, bb));

Hellllllllll no.

All that's missing right now is gcc checking that the function signature matches
the args specified within the (). That will come. No way in hell am I going to
implement some half baked hacked up macro crap - I intend to do this right.

> (2) Let the pretty-printer itself extract the varargs it expects. To
> portably pass around a va_list by reference it needs to be wrapped, so
> this would be
> 
> struct wva { va_list ap; };
> 
> void foo_pp(struct printbuf *pb, struct wva *w)
> {
>   int x = va_arg(w->ap, int);
>   long y = va_arg(w->ap, long);
>   pr_printf(pb, "%d %ld", x, y);
> }
> 
> printk("bla bla %pf(%d, %ld)\n", foo_pp, aa, bb)
> 
> with the core printf implementation internally using such a wrapped
> va_list, and after a %pf( relying on the pretty-printer having consumed
> the arguments up until the closing ). It would probably be a good idea
> to give the pretty-printer a pointer to that opening '(' or one-past-it
> as well so it could do a sanity check.

Also no. Varargs is terrible in most situations, and should only be used for
functions that actually want to take variable numbers of arguments - that
doesn't apply to pretty printers.