linux-kernel - Re: [PATCH 3/3] kunit: test: Drop CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250617095936-50d985a4-ea18-49cf-9d16-dfd0dd0b627f@linutronix.de>
Date: Tue, 17 Jun 2025 10:39:45 +0200
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Petr Pavlu <petr.pavlu@...e.com>
Cc: Luis Chamberlain <mcgrof@...nel.org>, 
	Sami Tolvanen <samitolvanen@...gle.com>, Daniel Gomez <da.gomez@...sung.com>, 
	Brendan Higgins <brendan.higgins@...ux.dev>, David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>, 
	linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	kunit-dev@...glegroups.com
Subject: Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery

On Tue, Jun 17, 2025 at 09:44:49AM +0200, Petr Pavlu wrote:
> On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
> > The function stubs exposed by module.h allow the code to compile properly
> > without the ifdeffery. The generated object code stays the same, as the
> > compiler can optimize away all the dead code.
> > As the code is still typechecked developer errors can be detected faster.
> > 
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> 
> I'm worried that patches #2 and #3 make the code harder to understand
> because they hide what is compiled and when.
> 
> Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly
> surrounds functionality that should be conditional. This makes it clear
> what is used and when.

#ifdef is discouraged in C files and IS_ENABLED(CONFIG_MODULES) does not work
(here) without patch #2.

> The patches obscure whether, for instance, kunit_module_notify() in
> lib/kunit/test.c is actually used and present in the resulting binary
> behind several steps. Understanding its usage requires tracing the code
> from kunit_module_notify() to the definition of kunit_mod_nb, then to
> the register_module_notifier() call, and finally depends on an ifdef in
> another file, include/linux/module.h.

I agree that it is not completely clear what will end up in the binary.
On the other hand we can program the happy path and the compiler will take care
of all the corner cases.
We could add an "if (IS_ENABLED(CONFIG_MODULES))" which does not really change
anything but would be clearer to read.

> Is this really better? Are there places where this pattern is already
> used? Does it actually help in practice, considering that CONFIG_MODULES
> is enabled in most cases?

This came up for me when refactoring some KUnit internal code.
I used "kunit.py run" (which uses CONFIG_MODULES=n) to test my changes.
But some callers of changed functions were not updated and this wasn't reported.

The stub functions are a standard pattern and already implemented by module.h.
I have not found a header which hides structure definitions.

Documentation/process/coding-style.rst:

	21) Conditional Compilation
	---------------------------

	Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
	files; doing so makes code harder to read and logic harder to follow.  Instead,
	use such conditionals in a header file defining functions for use in those .c
	files, providing no-op stub versions in the #else case, and then call those
	functions unconditionally from .c files.  The compiler will avoid generating
	any code for the stub calls, producing identical results, but the logic will
	remain easy to follow.

I should add the documentation reference to patch #2.