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

From: Chris Down
Date: Fri Jun 04 2021 - 07:51:11 EST

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,

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 :-)