Re: [PATCH v1] module: Fix prefix for module.sig_enforce module param

[Randy Dunlap <rdunlap@infradead.org>]

Hi--

On 6/2/22 20:48, Saravana Kannan wrote:
> On Thu, Jun 2, 2022 at 3:59 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>
>> On Thu, Jun 02, 2022 at 02:47:04PM -0700, Saravana Kannan wrote:
>>> On Thu, Jun 2, 2022 at 12:41 PM Linus Torvalds
>>> <torvalds@linux-foundation.org> wrote:
>>>>
>>>> On Thu, Jun 2, 2022 at 12:16 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>>>
>>>>> Linus want to take this in or should I just queue these up?
>>>>
>>>> I'll take it, and remove the unnecessary #ifdef/#endif. The #undef
>>>> might as well be unconditional - simpler and doesn't hurt.
>>>
>>> Sounds good. I just copy-pasted how it was done elsewhere. Luis was
>>> mentioning adding a wrapper to go this cleanly and I needed it in
>>> another instance too. So I'll look into doing that in a future patch.
>>
>> Virtual hug, or something hippie like that.
> 
> Thanks :)
> 
> I gave this a shot.
> 
> + #define set_module_param_prefix(prefix) \
> + #undef MODULE_PARAM_PREFIX              \
> + #define MODULE_PARAM_PREFIX prefix
> 
> I even wrote up a nice chunk of "function doc" before I tried
> compiling it. And once I compiled it, I was smacked in the head by the
> compiler for trying to put a #define inside a #define (Duh, the
> preprocessing in a single pass).
> 
> Before I tried this, I looked up the current uses of the "hacky" snippet:
> 
> $ git grep -l "define.*MODULE_PARAM_PREFIX" -- :^include/
> block/disk-events.c
> drivers/misc/cxl/fault.c
> drivers/mmc/core/block.c
> drivers/pci/pcie/aspm.c
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> drivers/tty/serial/8250/8250_core.c
> drivers/xen/balloon.c
> drivers/xen/events/events_base.c
> kernel/debug/kdb/kdb_main.c
> kernel/kcsan/core.c
> kernel/rcu/tree.c
> kernel/rcu/update.c
> mm/damon/reclaim.c
> mm/kfence/core.c
> 
> 
> In a lot of those files, there are a lot of module params that follow
> this snippet. Going on a tangent, some of the uses of #define
> MODULE_PARAM_PREFIX don't seem to have any obvious use or param
> macros.
> 
> So adding a module_param_prefixed() doesn't make sense to me either,
> because I'll have to repeat the same prefix in every usage of
> module_param_prefixed() AND I'll have to create a _prefixed() variant
> for other param macros too.
> 
> So, something like:
> module_param_prefixed("module.", sig_enforce, bool, 644);
> module_param_prefixed("module.", another_param1, bool, 644);
> module_param_prefixed("module.", another_param2, bool, 644);
> 
> Or replace "module." with a MY_PREFIX so that it's easy to ensure the
> string is the same across each use:
> #define MY_PREFIX "module."
> module_param_prefixed(MY_PREFIX, sig_enforce, bool, 644);
> module_param_prefixed(MY_PREFIX, another_param1, bool, 644);
> module_param_prefixed(MY_PREFIX, another_param2, bool, 644);
> 
> But at that point, all I'm avoiding is one #undef MODULE_PARAM_PREFIX
> and a whole lot of code churn.
> 
> One other option is to do something like:
> #ifndef MOD_PREFIX
> #define MODULE_PARAM_PREFIX KBUILD_MODNAME "."
> #else
> #define MODULE_PARAM_PREFIX MOD_PREFIX "."
> #endif
> 
> But that will have the limitation that MOD_PREFIX has to be defined
> before any #includes is in a file (similar to pr_fmt())  and doesn't
> allow you to redefine the prefix half way through a file -- which is
> also a thing that happens in drivers/tty/serial/8250/8250_core.c.
> 
> So, long story short, none of these options sound especially appealing
> that it's worth all the code churn across multiple maintainer trees.
> Let me know if you have other ideas that might work or you think one
> of the options I dismissed is actually worth doing.

I agree with your assessment. Nothing new is needed.

-- 
~Randy