Re: [PATCH v6 3/4] printk: Userspace format indexing support

[Chris Down]

Petr Mladek writes:
> > Petr Mladek writes:
> > > Note that we do not store the value defined by pr_fmt() which
> > > is yet another automatically added prefix.
> >
> > Hmm, we do actually store pr_fmt, since it's known at compile time (or we
> > wouldn't store it at all if it was dynamic due to the __builtin_constant_p
> > check). Or are you seeing something different?
>
> I see. Well, the prefix defined by pr_fmt() becomes part of entry.fmt.
> It is _not_ stored in entry.pre_fmt which confused me.
>
> As I mentioned in the previous mail. I think about using "subsys_fmt"
> or "subsys_prefix" instead of "pre_fmt".
>
> Anyway, we should improve the comment. For example, something like:
>
> 	/*
> 	 * The format string used by various subsystem specific printk()
> 	 * wrappers to prefix the message.
> 	 *
> 	 * Note that the static prefix defined by pr_fmt() macro is handled
> 	 * another way. It is stored directly in the message format (@fmt).
> 	 */
> 	const char *subsys_fmt;

Sounds totally reasonable, will do.

> > > Is is possible to update __printk_index_emit() to do not create
> > > entries with fmt = NULL ?
> > >
> > > We should either remove the above check or add a comment
> > > explaining why it is necessary.
> >
> > The trick is unfortunately necessary for the reason described in the comment
> > above the double check:
> >
> >     /*
> >      * The compiler may not be able to eliminate the
> >      * non-constant variants of _fmt and _level, so we need
> >      * to make sure that it doesn't see any hypothetical
> >      * assignment for non-constants even though this is
> >      * already inside the __builtin_constant_p guard.
> >      */
> >
> > Happy to add another comment if it helps.
>
> I re-read the discussion about this,
> namely https://lore.kernel.org/r/3Kf896Zt9O+/Yh@xxxxxxxxxxxxxx
>
> Frankly, the comment above ftrace_vprintk() is easier to understand
> for me. I mean:
>
> /*
> * The double __builtin_constant_p is because gcc will give us an error
> * if we try to allocate the static variable to fmt if it is not a
> * constant. Even with the outer if statement.
> */

Oh yes, that does sound more clear. I'll update it :-)

Thanks!