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

[Chris Down <chris@chrisdown.name>]

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@chrisdown.name
>
>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!