Message-ID: <CAFd5g47G23c-FuPjiy_NVsep2juvcwgWh-6bY7uuVmx+fZgFJA@mail.gmail.com>
Date:   Mon, 30 Jan 2023 15:15:54 -0500
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendan.higgins@...ux.dev>,
        Kees Cook <keescook@...omium.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Daniel Latypov <dlatypov@...gle.com>,
        Rae Moar <rmoar@...gle.com>,
        Sadiya Kazi <sadiyakazi@...gle.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Joe Fradley <joefradley@...gle.com>,
        Steve Muckle <smuckle@...gle.com>,
        Jonathan Corbet <corbet@....net>, kunit-dev@...glegroups.com,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 1/2] kunit: Expose 'static stub' API to redirect functions

On Sat, Jan 28, 2023 at 2:49 AM David Gow <davidgow@...gle.com> wrote:
>
> Add a simple way of redirecting calls to functions by including a
> special prologue in the "real" function which checks to see if the
> replacement function should be called (and, if so, calls it).
>
> To redirect calls to a function, make the first (non-declaration) line
> of the function:
>
>         KUNIT_STATIC_STUB_REDIRECT(function_name, [function arguments]);
>
> (This will compile away to nothing if KUnit is not enabled, otherwise it
> will check if a redirection is active, call the replacement function,
> and return. This check is protected by a static branch, so has very
> little overhead when there are no KUnit tests running.)
>
> Calls to the real function can be redirected to a replacement using:
>
>         kunit_activate_static_stub(test, real_fn, replacement_fn);
>
> The redirection will only affect calls made from within the kthread of
> the current test, and will be automatically disabled when the test
> completes. It can also be manually disabled with
> kunit_deactivate_static_stub().
>
> The 'example' KUnit test suite has a more complete example.
>
> Co-developed-by: Daniel Latypov <dlatypov@...gle.com>
> Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> Signed-off-by: David Gow <davidgow@...gle.com>
> Reviewed-by: Brendan Higgins <brendanhiggins@...gle.com>

Still looks good overall to me, but I see one nit:

> ---
>
> This patch depends upon the 'hooks' implementation in
> https://lore.kernel.org/linux-kselftest/20230128071007.1134942-1-davidgow@google.com/
>
> Note that checkpatch.pl does warn about control flow in the
> KUNIT_STATIC_STUB_REDIRECT() macro. This is an intentional design choice
> (we think it makes the feature easier to use), though if there are
> strong objections, we can of course reconsider.
>
> Changes since v1:
> https://lore.kernel.org/all/20221208061841.2186447-2-davidgow@google.com/
> - Adapted to use the "hooks" mechanism
>   - See: https://lore.kernel.org/linux-kselftest/20230128071007.1134942-1-davidgow@google.com/
>   - Now works when KUnit itself is compiled as a module (CONFIG_KUNIT=m)
>
> Changes since RFC v2:
> https://lore.kernel.org/linux-kselftest/20220910212804.670622-2-davidgow@google.com/
> - Now uses the kunit_get_current_test() function, which uses the static
>   key to reduce overhead.
>   - Thanks Kees for the suggestion.
>   - Note that this does prevent redirections from working when
>     CONFIG_KUNIT=m -- this is a restriction of kunit_get_current_test()
>     which will be removed in a future patch.
> - Several tidy-ups to the inline documentation.
>
> Changes since RFC v1:
> https://lore.kernel.org/lkml/20220318021314.3225240-2-davidgow@google.com/
> - Use typecheck_fn() to fix typechecking in some cases (thanks Brendan)
>
> ---
>  include/kunit/static_stub.h    | 113 ++++++++++++++++++++++++++++++
>  include/kunit/test-bug.h       |   1 +
>  lib/kunit/Makefile             |   1 +
>  lib/kunit/hooks-impl.h         |   2 +
>  lib/kunit/kunit-example-test.c |  38 ++++++++++
>  lib/kunit/static_stub.c        | 123 +++++++++++++++++++++++++++++++++
>  6 files changed, 278 insertions(+)
>  create mode 100644 include/kunit/static_stub.h
>  create mode 100644 lib/kunit/static_stub.c
>
> diff --git a/include/kunit/static_stub.h b/include/kunit/static_stub.h
> new file mode 100644
> index 000000000000..047b68d65f1a
> --- /dev/null
> +++ b/include/kunit/static_stub.h
> @@ -0,0 +1,113 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * KUnit function redirection (static stubbing) API.
> + *
> + * Copyright (C) 2022, Google LLC.
> + * Author: David Gow <davidgow@...gle.com>
> + */
> +#ifndef _KUNIT_STATIC_STUB_H
> +#define _KUNIT_STATIC_STUB_H
> +
> +#if !IS_ENABLED(CONFIG_KUNIT)
> +
> +/* If CONFIG_KUNIT is not enabled, these stubs quietly disappear. */
> +#define KUNIT_TRIGGER_STATIC_STUB(real_fn_name, args...) do {} while (0)
> +
> +#else
> +
> +#include <kunit/test.h>
> +#include <kunit/test-bug.h>
> +
> +#include <linux/compiler.h> /* for {un,}likely() */
> +#include <linux/sched.h> /* for task_struct */
> +
> +
> +/**
> + * KUNIT_STATIC_STUB_REDIRECT() - call a replacement 'static stub' if one exists
> + * @real_fn_name: The name of this function (as an identifier, not a string)
> + * @args: All of the arguments passed to this function
> + *
> + * This is a function prologue which is used to allow calls to the current
> + * function to be redirected by a KUnit test. KUnit tests can call
> + * kunit_activate_static_stub() to pass a replacement function in. The
> + * replacement function will be called by KUNIT_TRIGGER_STATIC_STUB(), which
> + * will then return from the function. If the caller is not in a KUnit context,
> + * the function will continue execution as normal.
> + *
> + * Example:
> + *
> + * .. code-block:: c
> + *
> + *     int real_func(int n)
> + *     {
> + *             KUNIT_STATIC_STUB_REDIRECT(real_func, n);
> + *             return 0;
> + *     }
> + *
> + *     void replacement_func(int n)

nit: Pretty sure the return type should be `int`

> + *     {
> + *             return 42;
> + *     }
[...]

Powered by blists - more mailing lists