Re: [PATCH] drm/client: Convert to VISIBLE_IF_KUNIT

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2806 bytes --]

Hi

Am 02.02.23 um 13:35 schrieb Maxime Ripard:
> Hi,
> 
> On Thu, Feb 02, 2023 at 01:22:01PM +0100, Thomas Zimmermann wrote:
>> Am 02.02.23 um 12:03 schrieb Maxime Ripard:
>>> Commit 8fc0380f6ba7 ("drm/client: Add some tests for
>>> drm_connector_pick_cmdline_mode()") was meant to introduce unit tests
>>> for the static drm_connector_pick_cmdline_mode() function.
>>>
>>> In such a case, the kunit documentation recommended to import the tests
>>> source file directly from the source file with the static function to
>>> test.
>>>
>>> While it was working, it's generally frowned upon. Fortunately, commit
>>> 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static
>>> symbols to tests") introduced macros to easily deal with that case. We
>>> can thus remove our include and use those macros instead.
>>
>> I like that this include statements is going away.
> 
> Yeah, when I saw that it was now available, I remembered you really
> didn't like it :)
> 
>> But changing symbol visibility for tests is likewise awkward.
>>
>> Maybe i'm askin gtoo miuch for this simple patch, but can't we have a helper
>> macro that generates an exported wrapper for Kunit tests? Something like
>> this:
>>
>> EXPORT_KUNIT_WRAPPER(struct drm_display_mode *\
>> 			drm_connector_pick_cmdline_mode,
>> 			struct drm_connector *connector);
>>
>> which then generates something like this:
>>
>> struct drm_display_mode * drm_connector_pick_cmdline_mode_kunit(
>> 	struct drm_connector *connector)
>> {
>> 	return drm_connector_pick_cmdline_mode(connector);
>> }
>>
>> I know that the macro for generating this code is more complex than
>> illustrated here. But this solution separates Kunit and functions cleanly.
>> The static functions that are exported for Kunit testing still need to be
>> declared in a header file. That could also be done via such a macro.
> 
> I mean, I guess we could do that, but what's the point? I don't really
> get what that wrapper brings to the table.

The big benefit of the kunit wrapper is that we don't change the 
visibility or implementation of the tested code. The currently existing 
macros invite linker errors because symbol visibility now depends on 
whether Kunit it enabled. It's also not clear to me how Kunit knows the 
symbol. Is there a function declaration in the Kunit test's source code? 
If so, it might diverge from the implementation; with consequences.

Best regards
Thomas

> 
> Also, this deviates from the existing practice we had for selftests and
> EXPORT_SYMBOL_FOR_TESTS_ONLY
> 
> Maxime

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]