From 2f9f21cdcd71d0c523676f551ea5c4f78d8e6f61 Mon Sep 17 00:00:00 2001 From: Lucas Stankus Date: Fri, 12 Mar 2021 16:11:26 -0300 Subject: [PATCH 1/6] kunit: Match parenthesis alignment to improve code readability Tidy up code by fixing the following checkpatch warnings: CHECK: Alignment should match open parenthesis CHECK: Lines should not end with a '(' Signed-off-by: Lucas Stankus Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/assert.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index e0ec7d6fed6f..acfbf86bddd6 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -25,7 +25,7 @@ void kunit_base_assert_format(const struct kunit_assert *assert, } string_stream_add(stream, "%s FAILED at %s:%d\n", - expect_or_assert, assert->file, assert->line); + expect_or_assert, assert->file, assert->line); } EXPORT_SYMBOL_GPL(kunit_base_assert_format); @@ -48,8 +48,9 @@ EXPORT_SYMBOL_GPL(kunit_fail_assert_format); void kunit_unary_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { - struct kunit_unary_assert *unary_assert = container_of( - assert, struct kunit_unary_assert, assert); + struct kunit_unary_assert *unary_assert; + + unary_assert = container_of(assert, struct kunit_unary_assert, assert); kunit_base_assert_format(assert, stream); if (unary_assert->expected_true) @@ -67,8 +68,10 @@ EXPORT_SYMBOL_GPL(kunit_unary_assert_format); void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { - struct kunit_ptr_not_err_assert *ptr_assert = container_of( - assert, struct kunit_ptr_not_err_assert, assert); + struct kunit_ptr_not_err_assert *ptr_assert; + + ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert, + assert); kunit_base_assert_format(assert, stream); if (!ptr_assert->value) { @@ -111,8 +114,10 @@ static bool is_literal(struct kunit *test, const char *text, long long value, void kunit_binary_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { - struct kunit_binary_assert *binary_assert = container_of( - assert, struct kunit_binary_assert, assert); + struct kunit_binary_assert *binary_assert; + + binary_assert = container_of(assert, struct kunit_binary_assert, + assert); kunit_base_assert_format(assert, stream); string_stream_add(stream, @@ -137,8 +142,10 @@ EXPORT_SYMBOL_GPL(kunit_binary_assert_format); void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { - struct kunit_binary_ptr_assert *binary_assert = container_of( - assert, struct kunit_binary_ptr_assert, assert); + struct kunit_binary_ptr_assert *binary_assert; + + binary_assert = container_of(assert, struct kunit_binary_ptr_assert, + assert); kunit_base_assert_format(assert, stream); string_stream_add(stream, @@ -159,8 +166,10 @@ EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format); void kunit_binary_str_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { - struct kunit_binary_str_assert *binary_assert = container_of( - assert, struct kunit_binary_str_assert, assert); + struct kunit_binary_str_assert *binary_assert; + + binary_assert = container_of(assert, struct kunit_binary_str_assert, + assert); kunit_base_assert_format(assert, stream); string_stream_add(stream, From acd976253c0ce98e92c766bd720bb00e4c2facb6 Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Fri, 2 Apr 2021 12:33:57 -0700 Subject: [PATCH 2/6] kunit: make KUNIT_EXPECT_STREQ() quote values, don't print literals Before: > Expected str == "world", but > str == hello > "world" == world After: > Expected str == "world", but > str == "hello" Note: like the literal ellision for integers, this doesn't handle the case of KUNIT_EXPECT_STREQ(test, "hello", "world") since we don't expect it to realistically happen in checked in tests. (If you really wanted a test to fail, KUNIT_FAIL("msg") exists) In that case, you'd get: > Expected "hello" == "world", but Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/assert.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index acfbf86bddd6..b972bda61c0c 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -163,6 +163,22 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, } EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format); +/* Checks if KUNIT_EXPECT_STREQ() args were string literals. + * Note: `text` will have ""s where as `value` will not. + */ +static bool is_str_literal(const char *text, const char *value) +{ + int len; + + len = strlen(text); + if (len < 2) + return false; + if (text[0] != '\"' || text[len - 1] != '\"') + return false; + + return strncmp(text + 1, value, len - 2) == 0; +} + void kunit_binary_str_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { @@ -177,12 +193,14 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, binary_assert->left_text, binary_assert->operation, binary_assert->right_text); - string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %s\n", - binary_assert->left_text, - binary_assert->left_value); - string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %s", - binary_assert->right_text, - binary_assert->right_value); + if (!is_str_literal(binary_assert->left_text, binary_assert->left_value)) + string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"\n", + binary_assert->left_text, + binary_assert->left_value); + if (!is_str_literal(binary_assert->right_text, binary_assert->right_value)) + string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == \"%s\"", + binary_assert->right_text, + binary_assert->right_value); kunit_assert_print_msg(assert, stream); } EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format); From 9854781dba371dda22880fc6acac7688fb5e2bae Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Mon, 22 Feb 2021 14:52:41 -0800 Subject: [PATCH 3/6] kunit: tool: make --kunitconfig accept dirs, add lib/kunit fragment TL;DR $ ./tools/testing/kunit/kunit.py run --kunitconfig=lib/kunit Per suggestion from Ted [1], we can reduce the amount of typing by assuming a convention that these files are named '.kunitconfig'. In the case of [1], we now have $ ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4 Also add in such a fragment for kunit itself so we can give that as an example more close to home (and thus less likely to be accidentally broken). [1] https://lore.kernel.org/linux-ext4/YCNF4yP1dB97zzwD@mit.edu/ Signed-off-by: Daniel Latypov Reviewed-by: David Gow Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/.kunitconfig | 3 +++ tools/testing/kunit/kunit.py | 4 +++- tools/testing/kunit/kunit_kernel.py | 2 ++ tools/testing/kunit/kunit_tool_test.py | 6 ++++++ 4 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 lib/kunit/.kunitconfig diff --git a/lib/kunit/.kunitconfig b/lib/kunit/.kunitconfig new file mode 100644 index 000000000000..9235b7d42d38 --- /dev/null +++ b/lib/kunit/.kunitconfig @@ -0,0 +1,3 @@ +CONFIG_KUNIT=y +CONFIG_KUNIT_TEST=y +CONFIG_KUNIT_EXAMPLE_TEST=y diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index d5144fcb03ac..5da8fb3762f9 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -184,7 +184,9 @@ def add_common_opts(parser) -> None: help='Run all KUnit tests through allyesconfig', action='store_true') parser.add_argument('--kunitconfig', - help='Path to Kconfig fragment that enables KUnit tests', + help='Path to Kconfig fragment that enables KUnit tests.' + ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" ' + 'will get automatically appended.', metavar='kunitconfig') def add_build_opts(parser) -> None: diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index f309a33256cd..89a7d4024e87 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -132,6 +132,8 @@ class LinuxSourceTree(object): return if kunitconfig_path: + if os.path.isdir(kunitconfig_path): + kunitconfig_path = os.path.join(kunitconfig_path, KUNITCONFIG_PATH) if not os.path.exists(kunitconfig_path): raise ConfigError(f'Specified kunitconfig ({kunitconfig_path}) does not exist') else: diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 1ad3049e9069..2e809dd956a7 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -251,6 +251,12 @@ class LinuxSourceTreeTest(unittest.TestCase): with tempfile.NamedTemporaryFile('wt') as kunitconfig: tree = kunit_kernel.LinuxSourceTree('', kunitconfig_path=kunitconfig.name) + def test_dir_kunitconfig(self): + with tempfile.TemporaryDirectory('') as dir: + with open(os.path.join(dir, '.kunitconfig'), 'w') as f: + pass + tree = kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir) + # TODO: add more test cases. From 359a376081d4fadfb073e3ddeb6bd6dc94d98341 Mon Sep 17 00:00:00 2001 From: Uriel Guajardo Date: Thu, 11 Mar 2021 07:23:13 -0800 Subject: [PATCH 4/6] kunit: support failure from dynamic analysis tools Add a kunit_fail_current_test() function to fail the currently running test, if any, with an error message. This is largely intended for dynamic analysis tools like UBSAN and for fakes. E.g. say I had a fake ops struct for testing and I wanted my `free` function to complain if it was called with an invalid argument, or caught a double-free. Most return void and have no normal means of signalling failure (e.g. super_operations, iommu_ops, etc.). Key points: * Always update current->kunit_test so anyone can use it. * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for CONFIG_KASAN=y * Create a new header so non-test code doesn't have to include all of (e.g. lib/ubsan.c) * Forward the file and line number to make it easier to track down failures * Declare the helper function for nice __printf() warnings about mismatched format strings even when KUnit is not enabled. Example output from kunit_fail_current_test("message"): [15:19:34] [FAILED] example_simple_test [15:19:34] # example_simple_test: initializing [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message [15:19:34] not ok 1 - example_simple_test Fixed minor check patch with checkpatch --fix option: Shuah Khan Signed-off-by: Daniel Latypov Signed-off-by: Uriel Guajardo Reviewed-by: Alan Maguire Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- include/kunit/test-bug.h | 29 +++++++++++++++++++++++++++++ lib/kunit/test.c | 39 +++++++++++++++++++++++++++++++++++---- 2 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 include/kunit/test-bug.h diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h new file mode 100644 index 000000000000..ce6f6edc7801 --- /dev/null +++ b/include/kunit/test-bug.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * KUnit API allowing dynamic analysis tools to interact with KUnit tests + * + * Copyright (C) 2020, Google LLC. + * Author: Uriel Guajardo + */ + +#ifndef _KUNIT_TEST_BUG_H +#define _KUNIT_TEST_BUG_H + +#define kunit_fail_current_test(fmt, ...) \ + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__) + +#if IS_BUILTIN(CONFIG_KUNIT) + +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, + const char *fmt, ...); + +#else + +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, + const char *fmt, ...) +{ +} + +#endif + +#endif /* _KUNIT_TEST_BUG_H */ diff --git a/lib/kunit/test.c b/lib/kunit/test.c index ec9494e914ef..2f6cc0123232 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -7,6 +7,7 @@ */ #include +#include #include #include #include @@ -16,6 +17,40 @@ #include "string-stream.h" #include "try-catch-impl.h" +#if IS_BUILTIN(CONFIG_KUNIT) +/* + * Fail the current test and print an error message to the log. + */ +void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...) +{ + va_list args; + int len; + char *buffer; + + if (!current->kunit_test) + return; + + kunit_set_failure(current->kunit_test); + + /* kunit_err() only accepts literals, so evaluate the args first. */ + va_start(args, fmt); + len = vsnprintf(NULL, 0, fmt, args) + 1; + va_end(args); + + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL); + if (!buffer) + return; + + va_start(args, fmt); + vsnprintf(buffer, len, fmt, args); + va_end(args); + + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer); + kunit_kfree(current->kunit_test, buffer); +} +EXPORT_SYMBOL_GPL(__kunit_fail_current_test); +#endif + /* * Append formatted message to log, size of which is limited to * KUNIT_LOG_SIZE bytes (including null terminating byte). @@ -273,9 +308,7 @@ static void kunit_try_run_case(void *data) struct kunit_suite *suite = ctx->suite; struct kunit_case *test_case = ctx->test_case; -#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) current->kunit_test = test; -#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT) */ /* * kunit_run_case_internal may encounter a fatal error; if it does, @@ -624,9 +657,7 @@ void kunit_cleanup(struct kunit *test) spin_unlock(&test->lock); kunit_remove_resource(test, res); } -#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) current->kunit_test = NULL; -#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)*/ } EXPORT_SYMBOL_GPL(kunit_cleanup); From f65968ac191bd5f31091ff132191bf2ce3aed6c8 Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Tue, 6 Apr 2021 10:29:01 -0700 Subject: [PATCH 5/6] kunit: fix -Wunused-function warning for __kunit_fail_current_test When CONFIG_KUNIT is not enabled, __kunit_fail_current_test() an empty static function. But GCC complains about unused static functions, *unless* they're static inline. So add inline to make GCC happy. Fixes: 359a376081d4 ("kunit: support failure from dynamic analysis tools") Reported-by: Stephen Rothwell Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- include/kunit/test-bug.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h index ce6f6edc7801..5fc58081d511 100644 --- a/include/kunit/test-bug.h +++ b/include/kunit/test-bug.h @@ -19,8 +19,8 @@ extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, #else -static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, - const char *fmt, ...) +static inline __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, + const char *fmt, ...) { } From de2fcb3e62013738f22bbb42cbd757d9a242574e Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Tue, 6 Apr 2021 15:51:00 -0700 Subject: [PATCH 6/6] Documentation: kunit: add tips for using current->kunit_test As of commit 359a376081d4 ("kunit: support failure from dynamic analysis tools"), we can use current->kunit_test to find the current kunit test. Mention this in tips.rst and give an example of how this can be used in conjunction with `test->priv` to pass around state and specifically implement something like mocking. There's a lot more we could go into on that topic, but given that example is already longer than every other "tip" on this page, we just point to the API docs and leave filling in the blanks as an exercise to the reader. Also give an example of kunit_fail_current_test(). Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- Documentation/dev-tools/kunit/tips.rst | 78 +++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 2 deletions(-) diff --git a/Documentation/dev-tools/kunit/tips.rst b/Documentation/dev-tools/kunit/tips.rst index a6ca0af14098..8d8c238f7f79 100644 --- a/Documentation/dev-tools/kunit/tips.rst +++ b/Documentation/dev-tools/kunit/tips.rst @@ -78,8 +78,82 @@ Similarly to the above, it can be useful to add test-specific logic. void test_only_hook(void) { } #endif -TODO(dlatypov@google.com): add an example of using ``current->kunit_test`` in -such a hook when it's not only updated for ``CONFIG_KASAN=y``. +This test-only code can be made more useful by accessing the current kunit +test, see below. + +Accessing the current test +-------------------------- + +In some cases, you need to call test-only code from outside the test file, e.g. +like in the example above or if you're providing a fake implementation of an +ops struct. +There is a ``kunit_test`` field in ``task_struct``, so you can access it via +``current->kunit_test``. + +Here's a slightly in-depth example of how one could implement "mocking": + +.. code-block:: c + + #include /* for current */ + + struct test_data { + int foo_result; + int want_foo_called_with; + }; + + static int fake_foo(int arg) + { + struct kunit *test = current->kunit_test; + struct test_data *test_data = test->priv; + + KUNIT_EXPECT_EQ(test, test_data->want_foo_called_with, arg); + return test_data->foo_result; + } + + static void example_simple_test(struct kunit *test) + { + /* Assume priv is allocated in the suite's .init */ + struct test_data *test_data = test->priv; + + test_data->foo_result = 42; + test_data->want_foo_called_with = 1; + + /* In a real test, we'd probably pass a pointer to fake_foo somewhere + * like an ops struct, etc. instead of calling it directly. */ + KUNIT_EXPECT_EQ(test, fake_foo(1), 42); + } + + +Note: here we're able to get away with using ``test->priv``, but if you wanted +something more flexible you could use a named ``kunit_resource``, see :doc:`api/test`. + +Failing the current test +------------------------ + +But sometimes, you might just want to fail the current test. In that case, we +have ``kunit_fail_current_test(fmt, args...)`` which is defined in ```` and +doesn't require pulling in ````. + +E.g. say we had an option to enable some extra debug checks on some data structure: + +.. code-block:: c + + #include + + #ifdef CONFIG_EXTRA_DEBUG_CHECKS + static void validate_my_data(struct data *data) + { + if (is_valid(data)) + return; + + kunit_fail_current_test("data %p is invalid", data); + + /* Normal, non-KUnit, error reporting code here. */ + } + #else + static void my_debug_function(void) { } + #endif + Customizing error messages --------------------------