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]
Date:   Tue, 21 Sep 2021 12:01:52 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Marcelo Schmitt <marcelo.schmitt1@...il.com>
Cc:     brendanhiggins@...gle.com, andy.li@...soc.com,
        andersonreisrosa@...il.com, linux-kernel@...r.kernel.org,
        kunit-dev@...glegroups.com
Subject: Re: [PATCH] kunit: mock: add support for function mocks with no parameters

On Sat, Sep 18, 2021 at 12:44 PM Marcelo Schmitt
<marcelo.schmitt1@...il.com> wrote:
>
> Function mocks defined with DEFINE_FUNCTION_MOCK(...) do not support
> empty parameters list due to strict function prototypes enforcement
> (-Werror=strict-prototypes). Add support for function mocks with no
> parameters by adding checks to declare strict function prototypes when
> an empty param list is provided.
> Further, add an expectation to test that the generated code works.
>
> Co-developed-by: Anderson Reis Rosa <andersonreisrosa@...il.com>
> Signed-off-by: Anderson Reis Rosa <andersonreisrosa@...il.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt1@...il.com>

Meta: kunit/alpha/master isn't really maintained anymore.
I think David and myself having some commits from this year might give
the wrong impression.
But all of my patches in 2021 were to make it easier to get people to
*move away* from kunit/alpha/master ;)

We sort of don't want to encourage anyone new from using it, see
https://kunit.dev/third_party/stable_kernel/index.html
That said, it's useful to have it around as a reference, and this
patch makes it more useful, so:

Reviewed-by: Daniel Latypov <dlatypov@...gle.com>
This looks reasonable to me, minor nit about the added test case below.

I think only Brendan could manually apply this and push it.
It would be a bit easier to go through the process of pushing it to
gerrit at https://kunit-review.googlesource.com/
I'd wait on Brendan to chime in on what he'd like to do (or if he'd
rather not have any new changes).

> ---
> We noted that the proposed changes added a few checkpatch errors.
> However, as checkpatch reported the same errors for other macros at
> params.h, we didn't spend a lot of time trying to find a fix for them.
>
> We hope this may solve Bugzilla Bug 205495
> https://bugzilla.kernel.org/show_bug.cgi?id=205495
>
>  include/test/mock.h    |  2 +-
>  include/test/params.h  | 12 +++++++++++-
>  test/mock-macro-test.c |  6 ++++++
>  3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/include/test/mock.h b/include/test/mock.h
> index 8b8031c13b2a..c46c90abc12a 100644
> --- a/include/test/mock.h
> +++ b/include/test/mock.h
> @@ -640,7 +640,7 @@ int mock_in_sequence(struct KUNIT_T *test, struct mock_expectation *first, ...);
>                                   return_type,                                 \
>                                   RETURN,                                      \
>                                   param_types...)                              \
> -               return_type name(PARAM_LIST_FROM_TYPES(param_types))           \
> +               return_type name(FUNC_PARAM_LIST_FROM_TYPES(param_types))      \
>                 {                                                              \
>                         struct mock *mock = MOCK_SOURCE(mock_source_ctx,       \
>                                                         handle_index);         \
> diff --git a/include/test/params.h b/include/test/params.h
> index 50d54035175d..ca4689dd0576 100644
> --- a/include/test/params.h
> +++ b/include/test/params.h
> @@ -264,6 +264,11 @@
>                                not_used,                                       \
>                                args)
>
> +#define FUNC_PARAM_LIST_FROM_TYPES(args...)                                   \
> +               IF(IS_EQUAL(NUM_VA_ARGS(args), 0))(void)                       \
> +               IF(IS_NOT_EQUAL(NUM_VA_ARGS(args), 0))                         \
> +               (PARAM_LIST_FROM_TYPES(args))
> +
>  #define PRODUCE_TYPE_NAME(context, type, index) #type
>  #define TYPE_NAMES_FROM_TYPES(handle_index, args...)                          \
>                 FOR_EACH_PARAM(PRODUCE_TYPE_NAME,                              \
> @@ -282,12 +287,17 @@
>                 IF(IS_EQUAL(index, ctrl_index))(struct mock *arg##ctrl_index)  \
>                 IF(IS_NOT_EQUAL(index, ctrl_index))(                           \
>                                 struct mock_param_matcher *arg##index)
> -#define MATCHER_PARAM_LIST_FROM_TYPES(ctrl_index, args...)                    \
> +#define MATCHER_PARAM_LIST_FROM_TYPES_INTERNAL(ctrl_index, args...)           \
>                 FOR_EACH_PARAM(PRODUCE_MATCHER_AND_ARG,                        \
>                                FILTER_NONE,                                    \
>                                ctrl_index,                                     \
>                                args)
>
> +#define MATCHER_PARAM_LIST_FROM_TYPES(ctrl_index, args...)                    \
> +               IF(IS_EQUAL(NUM_VA_ARGS(args), 0))(void)                       \
> +               IF(IS_NOT_EQUAL(NUM_VA_ARGS(args), 0))                         \
> +               (MATCHER_PARAM_LIST_FROM_TYPES_INTERNAL(ctrl_index, args))
> +
>  #define PRODUCE_ARG(context, type, index) arg##index
>  #define ARG_NAMES_FROM_TYPES(ctrl_index, args...)                             \
>                 FOR_EACH_PARAM(PRODUCE_ARG,                                    \
> diff --git a/test/mock-macro-test.c b/test/mock-macro-test.c
> index 14da7ebe752d..91a926558551 100644
> --- a/test/mock-macro-test.c
> +++ b/test/mock-macro-test.c
> @@ -59,6 +59,8 @@ DEFINE_VOID_CLASS_MOCK_HANDLE_INDEX(METHOD(test_void_ptr_func),
>
>  DEFINE_FUNCTION_MOCK(add, RETURNS(int), PARAMS(int, int));
>
> +DEFINE_FUNCTION_MOCK(no_param, RETURNS(int), PARAMS());
> +
>  struct mock_macro_context {
>         struct MOCK(test_struct) *mock_test_struct;
>         struct MOCK(void) *mock_void_ptr;
> @@ -217,7 +219,11 @@ static void mock_macro_test_generated_function_code_works(struct KUNIT_T *test)
>         handle = EXPECT_CALL(add(int_eq(test, 4), int_eq(test, 3)));
>         handle->action = int_return(test, 7);
>
> +       handle = EXPECT_CALL(no_param());
> +       handle->action = int_return(test, 0);
> +
>         EXPECT_EQ(test, 7, add(4, 3));
> +       EXPECT_EQ(test, 0, no_param());
>  }

I think this test case would be a bit more readable if restructured as
   219          handle = EXPECT_CALL(add(int_eq(test, 4), int_eq(test, 3)));
   220          handle->action = int_return(test, 7);
   221          EXPECT_EQ(test, 7, add(4, 3));
   222
   223          handle = EXPECT_CALL(no_param());
   224          handle->action = int_return(test, 0);
   225          EXPECT_EQ(test, 0, no_param());

I might also further suggest using a different return value than 0.
Some frameworks return 0-values by default when there's no specified
action, in which case this EXPECT_EQ() would pass even if we didn't
specify `int_return(test, 0)`.
That's not how KUnit works, so it's not actually relevant.

>
>  static int mock_macro_test_init(struct KUNIT_T *test)
> --
> 2.33.0
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/YUZBkZhQsF5SlcLb%40marsc.168.1.7.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ