Re: [PATCH v3 3/4] Documentation: kunit: Warn that exit functions run even if init fails

From: Sadiya Kazi <sadiyakazi@google.com>
To: David Gow <davidgow@google.com>
Cc: Benjamin Berg <benjamin@sipsolutions.net>,
	Brendan Higgins <brendan.higgins@linux.dev>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Rae Moar <rmoar@google.com>, Daniel Latypov <dlatypov@google.com>,
	maxime@cerno.tech, Stephen Boyd <sboyd@kernel.org>,
	kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/4] Documentation: kunit: Warn that exit functions run even if init fails
Date: Tue, 25 Apr 2023 11:18:02 +0530	[thread overview]
Message-ID: <CAO2JNKUus7h7=Q4rdzYenuFgBqi6Y_SM2=XBLo86yoFKBt1qrA@mail.gmail.com> (raw)
In-Reply-To: <20230421040218.2156548-3-davidgow@google.com>

On Fri, Apr 21, 2023 at 9:32 AM David Gow <davidgow@google.com> wrote:
>
> KUnit's exit functions will run even if the corresponding init function
> fails. It's easy, when writing an exit function, to assume the init
> function succeeded, and (for example) access uninitialised memory or
> dereference NULL pointers.
>
> Note that this case exists and should be handled in the documentation.
>
> Suggested-by: Benjamin Berg <benjamin@sipsolutions.net>
> Link: https://lore.kernel.org/linux-kselftest/a39af0400abedb2e9b31d84c37551cecc3eed0e1.camel@sipsolutions.net/
> Signed-off-by: David Gow <davidgow@google.com>
> ---

Thank you, David. Except for a spelling error, this looks fine to me.
Reviewed-by: Sadiya Kazi <sadiyakazi@google.com>

Regards,
Sadiya
>
> No changes since v2:
> https://lore.kernel.org/linux-kselftest/20230419085426.1671703-3-davidgow@google.com/
>
> This patch was introduced in v2.
>
> ---
>  Documentation/dev-tools/kunit/usage.rst | 12 ++++++++++--
>  include/kunit/test.h                    |  3 +++
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 9f720f1317d3..f6d6c9a9ff54 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -166,7 +166,12 @@ many similar tests. In order to reduce duplication in these closely related
>  tests, most unit testing frameworks (including KUnit) provide the concept of a
>  *test suite*. A test suite is a collection of test cases for a unit of code
>  with optional setup and teardown functions that run before/after the whole
> -suite and/or every test case. For example:
> +suite and/or every test case.
> +
> +.. note::
> +   A test case will only run if it is associated with a test suite.
> +
> +For example:
>
>  .. code-block:: c
>
> @@ -196,7 +201,10 @@ after everything else. ``kunit_test_suite(example_test_suite)`` registers the
>  test suite with the KUnit test framework.
>
>  .. note::
> -   A test case will only run if it is associated with a test suite.
> +   The ``exit`` and ``suite_exit`` functions will run even if ``init`` or
> +   ``suite_init`` fail. Make sure that they can handle any inconsistent
> +   state which may result from ``init`` or ``suite_init`` encoutering errors

Nit: Spelling of "encountering"

> +   or exiting early.
>
>  ``kunit_test_suite(...)`` is a macro which tells the linker to put the
>  specified test suite in a special linker section so that it can be run by KUnit
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 57b309c6ca27..3028a1a3fcad 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -168,6 +168,9 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
>   * test case, similar to the notion of a *test fixture* or a *test class*
>   * in other unit testing frameworks like JUnit or Googletest.
>   *
> + * Note that @exit and @suite_exit will run even if @init or @suite_init
> + * fail: make sure they can handle any inconsistent state which may result.
> + *
>   * Every &struct kunit_case must be associated with a kunit_suite for KUnit
>   * to run it.
>   */
> --
> 2.40.0.634.g4ca3ef3211-goog
>