Re: [PATCH v1 1/2] kunit: add macro to allow conditionally exposing static symbols to tests

[David Gow]

On Thu, Nov 3, 2022 at 2:02 AM Rae Moar <rmoar@xxxxxxxxxx> wrote:
>
> Create two macros:
>
> VISIBLE_IF_KUNIT - A macro that sets symbols to be static if CONFIG_KUNIT
> is not enabled. Otherwise if CONFIG_KUNIT is enabled there is no change
> to the symbol definition.
>
> EXPORT_SYMBOL_IF_KUNIT(symbol) - Exports symbol into
> EXPORTED_FOR_KUNIT_TESTING namespace only if CONFIG_KUNIT is enabled. Must
> use MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING) in test file in order to
> use symbols.
>
> Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx>
> ---

This looks good to me overall.

Two thoughts (though they're not _problems_ with this patch, so
shouldn't necessarily hold it up).

How many KUnit headers do we want for these sorts of things? We've
currently got 'test.h' (for actual tests) and 'test-bug.h' for the
kunit_fail_current_test() style hooks. And there'll be 'static-stub.h'
coming.
Maybe it'd make sense to merge some of these, if it turns out the same
code always needs to access them? On the other hand, I actually quite
like having them separate like this. (Though the split of assertions
into assert.h and perhaps renaming test-bug.h might be worth doing
down the line.) No immediate action item, but something perhaps worth
thinking about.

Secondly, do we want to support individual subsystems and/or tests to
have their own symbol namespaces. Again, not a problem for the moment,
but we may need to deal with this if we need to export multiple copies
of very-generically-named functions. Any thoughts (e.g., an
EXPORT_SYMBOL_IF_KUNIT_NS macro which allows the user to specify the
namespace?)

Regardless, this is
Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David

>  include/kunit/visibility.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 include/kunit/visibility.h
>
> diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h
> new file mode 100644
> index 000000000000..eb22c9e6b4eb
> --- /dev/null
> +++ b/include/kunit/visibility.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * KUnit API to allow symbols to be conditionally visible during KUnit
> + * testing
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@xxxxxxxxxx>

Please update the date here, and use your name/email for the Author section.


> + */
> +
> +#ifndef _KUNIT_VISIBILITY_H
> +#define _KUNIT_VISIBILITY_H
> +
> +/**
> + * VISIBLE_IF_KUNIT - A macro that sets symbols to be static if CONFIG_KUNIT
> + * is not enabled. Otherwise if CONFIG_KUNIT is enabled there is no change
> + * to the symbol definition.
> + *
> + * EXPORT_SYMBOL_IF_KUNIT(symbol) - Exports symbol into
> + * EXPORTED_FOR_KUNIT_TESTING namespace only if CONFIG_KUNIT is
> + * enabled. Must use MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING)
> + * in test file in order to use symbols.
> + */
> +#if IS_ENABLED(CONFIG_KUNIT)
> +    #define VISIBLE_IF_KUNIT
> +    #define EXPORT_SYMBOL_IF_KUNIT(symbol) EXPORT_SYMBOL_NS(symbol, \
> +           EXPORTED_FOR_KUNIT_TESTING)
> +#else
> +    #define VISIBLE_IF_KUNIT static
> +    #define EXPORT_SYMBOL_IF_KUNIT(symbol)
> +#endif
> +
> +#endif /* _KUNIT_VISIBILITY_H */
> --
> 2.38.1.273.g43a17bfeac-goog
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature