Message-ID: <CAHk-=wiGMNvKaVuSDD7y2JeK+NsNyXtqZEusOLmEw9uE+0ZySg@mail.gmail.com>
Date:   Mon, 27 Jun 2022 21:46:46 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Kent Overstreet <kent.overstreet@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Petr Mladek <pmladek@...e.com>
Subject: Re: [PATCH v5whatever, now with typechecking] vsprintf: %pf(%p)

On Mon, Jun 27, 2022 at 8:24 PM Kent Overstreet
<kent.overstreet@...il.com> 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

Powered by blists - more mailing lists