Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 4 Jun 2021 12:50:05 +0100
From:   Chris Down <chris@chrisdown.name>
To:     Petr Mladek <pmladek@suse.com>
Cc:     linux-kernel@vger.kernel.org, Jessica Yu <jeyu@kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        John Ogness <john.ogness@linutronix.de>,
        Steven Rostedt <rostedt@goodmis.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Kees Cook <keescook@chromium.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>, kernel-team@fb.com
Subject: Re: [PATCH v6 3/4] printk: Userspace format indexing support
Message-ID: <YLoTbQjQzrPcLcHd@chrisdown.name>
References: <cover.1621338324.git.chris@chrisdown.name>
 <05d25c65d3f5149c1e8537f74041a7a46bd489d6.1621338324.git.chris@chrisdown.name>
 <YK0ShqdTSVTYXGPF@alley>
 <YLZPJpTC6YpJwAJk@chrisdown.name>
 <YLn+IEf3iEZpEeQZ@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <YLn+IEf3iEZpEeQZ@alley>
User-Agent: Mutt/2.0.7 (481f3800) (2021-05-04)
Precedence: bulk

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!