Received-SPF: pass (google.com: domain of linux-kernel+bounces-6085-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231218161044.215640-1-rf@opensource.cirrus.com> <20231218161044.215640-2-rf@opensource.cirrus.com>
In-Reply-To: <20231218161044.215640-2-rf@opensource.cirrus.com>
From: Rae Moar <rmoar@google.com>
Date: Tue, 19 Dec 2023 18:15:48 -0500
Message-ID: <CA+GJov7jyn461d-wyTMr=kN5TyFP=muox75rtj4M3kYeu4=JfQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] kunit: Add example of kunit_activate_static_stub()
 with pointer-to-function
To: Richard Fitzgerald <rf@opensource.cirrus.com>
Cc: brendan.higgins@linux.dev, davidgow@google.com, 
	linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com, 
	linux-kernel@vger.kernel.org, patches@opensource.cirrus.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Dec 18, 2023 at 11:10=E2=80=AFAM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Adds a variant of example_static_stub_test() that shows use of a
> pointer-to-function with kunit_activate_static_stub().
>
> A const pointer to the add_one() function is declared. This
> pointer-to-function is passed to kunit_activate_static_stub() and
> kunit_deactivate_static_stub() instead of passing add_one directly.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Hello!

This test looks good to me. However, I had issues applying this patch
so I think it needs rebasing due to the newest additions to
kselftest/kunit. But otherwise this patch looks good other than my
very small comment below.

Thanks!
-Rae

> ---
>  lib/kunit/kunit-example-test.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-tes=
t.c
> index d2f7a3c62c18..9e57f341dc37 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -168,6 +168,8 @@ static int subtract_one(int i)
>         return i - 1;
>  }
>
> +static int (* const add_one_fn_ptr)(int i) =3D add_one;

This is a bit of a nit but could you add a brief comment above this
pointer definition? This would then match the commenting on the other
functions in kunit-example-test and provide more context for those
looking at the example tests.

> +
>  /*
>   * This test shows the use of static stubs.
>   */
> @@ -187,6 +189,30 @@ static void example_static_stub_test(struct kunit *t=
est)
>         KUNIT_EXPECT_EQ(test, add_one(1), 2);
>  }
>
> +/*
> + * This test shows the use of static stubs when the function being
> + * replaced is provided as a pointer-to-function instead of the
> + * actual function. This is useful for providing access to static
> + * functions in a module by exporting a pointer to that function
> + * instead of having to change the static function to a non-static
> + * exported function.
> + */
> +static void example_static_stub_using_fn_ptr_test(struct kunit *test)
> +{
> +       /* By default, function is not stubbed. */
> +       KUNIT_EXPECT_EQ(test, add_one(1), 2);
> +
> +       /* Replace add_one() with subtract_one(). */
> +       kunit_activate_static_stub(test, add_one_fn_ptr, subtract_one);
> +
> +       /* add_one() is now replaced. */
> +       KUNIT_EXPECT_EQ(test, add_one(1), 0);
> +
> +       /* Return add_one() to normal. */
> +       kunit_deactivate_static_stub(test, add_one_fn_ptr);
> +       KUNIT_EXPECT_EQ(test, add_one(1), 2);
> +}
> +
>  static const struct example_param {
>         int value;
>  } example_params_array[] =3D {
> @@ -245,6 +271,7 @@ static struct kunit_case example_test_cases[] =3D {
>         KUNIT_CASE(example_mark_skipped_test),
>         KUNIT_CASE(example_all_expect_macros_test),
>         KUNIT_CASE(example_static_stub_test),
> +       KUNIT_CASE(example_static_stub_using_fn_ptr_test),
>         KUNIT_CASE_PARAM(example_params_test, example_gen_params),
>         KUNIT_CASE_SLOW(example_slow_test),
>         {}
> --
> 2.30.2
>