* Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
From: Stephen Boyd @ 2019-08-12 23:59 UTC (permalink / raw)
To: Brendan Higgins
Cc: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, shuah, tytso, yamada.masahiro, devicetree,
dri-devel, kunit-dev, linux-doc, linux-fsdevel, linux-kbuild,
linux-kernel, linux-kselftest, linux-nvdimm, linux-um,
Alexander.Levin, Tim.Bird, amir73il, dan.carpenter, daniel, jdike,
joel, julia.lawall, khilman, knut.omang, logang, mpe, pmladek,
rdunlap, richard, rientjes, rostedt, wfg
In-Reply-To: <20190812233336.GA224410@google.com>
Quoting Brendan Higgins (2019-08-12 16:33:36)
> On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote:
> > Quoting Brendan Higgins (2019-08-12 11:24:06)
> > > +void string_stream_clear(struct string_stream *stream)
> > > +{
> > > + struct string_stream_fragment *frag_container, *frag_container_safe;
> > > +
> > > + spin_lock(&stream->lock);
> > > + list_for_each_entry_safe(frag_container,
> > > + frag_container_safe,
> > > + &stream->fragments,
> > > + node) {
> > > + list_del(&frag_container->node);
> >
> > Shouldn't we free the allocation here? Otherwise, if some test is going
> > to add, add, clear, add, it's going to leak until the test is over?
>
> So basically this means I should add a kunit_kfree and
> kunit_resource_destroy (respective equivalents to devm_kfree, and
> devres_destroy) and use kunit_kfree here?
>
Yes, or drop the API entirely? Does anything need this functionality?
^ permalink raw reply
* Re: [PATCH v12 05/18] kunit: test: add the concept of expectations
From: Brendan Higgins @ 2019-08-13 0:33 UTC (permalink / raw)
To: Stephen Boyd
Cc: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, shuah, tytso, yamada.masahiro, devicetree,
dri-devel, kunit-dev, linux-doc, linux-fsdevel, linux-kbuild,
linux-kernel, linux-kselftest, linux-nvdimm, linux-um,
Alexander.Levin, Tim.Bird, amir73il, dan.carpenter, daniel, jdike,
joel, julia.lawall, khilman, knut.omang, logang, mpe, pmladek,
rdunlap, richard, rientjes, rostedt, wfg
In-Reply-To: <20190812235701.533E82063F@mail.kernel.org>
On Mon, Aug 12, 2019 at 04:57:00PM -0700, Stephen Boyd wrote:
> Quoting Brendan Higgins (2019-08-12 11:24:08)
> > Add support for expectations, which allow properties to be specified and
> > then verified in tests.
> >
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
> Just some minor nits again.
>
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index d0bf112910caf..2625bcfeb19ac 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -9,8 +9,10 @@
> > #ifndef _KUNIT_TEST_H
> > #define _KUNIT_TEST_H
> >
> > +#include <linux/kernel.h>
> > #include <linux/types.h>
> > #include <linux/slab.h>
> > +#include <kunit/assert.h>
>
> Can you alphabet sort these?
Sure. Will fix.
> >
> > struct kunit_resource;
> >
> > @@ -319,4 +321,845 @@ void __printf(3, 4) kunit_printk(const char *level,
> > #define kunit_err(test, fmt, ...) \
> > kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
> >
> > +/*
> > + * Generates a compile-time warning in case of comparing incompatible types.
> > + */
> > +#define __kunit_typecheck(lhs, rhs) \
> > + ((void) __typecheck(lhs, rhs))
>
> Is there a reason why this can't be inlined and the __kunit_typecheck()
> macro can't be removed?
No real reason anymore. I used it in multiple places before and we
weren't sure if we wanted to stick with the warnings that __typecheck
produces long term, but now that it is only used in one place, I guess
that doesn't make sense anymore. Will fix.
> > +
> > +/**
> > + * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> > + * @test: The test context object.
> [...]
> > + * @condition: an arbitrary boolean expression. The test fails when this does
> > + * not evaluate to true.
> > + *
> > + * This and expectations of the form `KUNIT_EXPECT_*` will cause the test case
> > + * to fail when the specified condition is not met; however, it will not prevent
> > + * the test case from continuing to run; this is otherwise known as an
> > + * *expectation failure*.
> > + */
> > +#define KUNIT_EXPECT_TRUE(test, condition) \
> > + KUNIT_TRUE_ASSERTION(test, KUNIT_EXPECTATION, condition)
>
> A lot of these macros seem double indented.
In a case you pointed out in the preceding patch, I was just keeping the
arguments column aligned.
In this case I am just indenting two tabs for a line continuation. I
thought I found other instances in the kernel that did this early on
(and that's also what the Linux kernel vim plugin wanted me to do).
After a couple of spot checks, it seems like one tab for this kind of
line continuation seems more common. I personally don't feel strongly
about any particular version. I just want to know now what the correct
indentation is for macros before I go through and change them all.
I think there are three cases:
#define macro0(param0, param1) \
a_really_long_macro(...)
In this first case, I use two tabs for the first indent, I think you are
telling me this should be one tab.
#define macro1(param0, param1) { \
statement_in_a_block0; \
statement_in_a_block1; \
... \
}
In this case, every line is in a block and is indented as it would be in
a function body. I think you are okay with this, and now that I am
thinking about it, what I think you are proposing for macro0 will make
these two cases more consistent.
#define macro2(param0, \
param1, \
param2, \
param3, \
..., \
paramn) ... \
In this last case, the body would be indented as in macro0, or macro1,
but the parameters passed into the macro are column aligned, consistent
with one of the acceptable ways of formatting function parameters that
don't fit on a single line.
In all cases, I put 1 space in between the closing parameter paren and
the line continuation `\`, if only one `\` is needed. Otherwise, I align
all the `\s` to the 80th column. Is this okay, or would you prefer that
I align them all to the 80th column, or something else?
> > +
> > +#define KUNIT_EXPECT_TRUE_MSG(test, condition, fmt, ...) \
> > + KUNIT_TRUE_MSG_ASSERTION(test, \
> > + KUNIT_EXPECTATION, \
> > + condition, \
> > + fmt, \
> > + ##__VA_ARGS__)
> > +
^ permalink raw reply
* Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
From: Brendan Higgins @ 2019-08-13 0:41 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190812235940.100842063F@mail.kernel.org>
On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 16:33:36)
> > On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote:
> > > Quoting Brendan Higgins (2019-08-12 11:24:06)
> > > > +void string_stream_clear(struct string_stream *stream)
> > > > +{
> > > > + struct string_stream_fragment *frag_container, *frag_container_safe;
> > > > +
> > > > + spin_lock(&stream->lock);
> > > > + list_for_each_entry_safe(frag_container,
> > > > + frag_container_safe,
> > > > + &stream->fragments,
> > > > + node) {
> > > > + list_del(&frag_container->node);
> > >
> > > Shouldn't we free the allocation here? Otherwise, if some test is going
> > > to add, add, clear, add, it's going to leak until the test is over?
> >
> > So basically this means I should add a kunit_kfree and
> > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > devres_destroy) and use kunit_kfree here?
> >
>
> Yes, or drop the API entirely? Does anything need this functionality?
Drop the kunit_resource API? I would strongly prefer not to.
string_stream uses it; the expectation stuff uses it via string
stream; some of the tests in this patchset allocate memory as part of
the test setup that uses it. The intention is that we would provide a
kunit_res_* version of many (hopefully eventually most) common
resources required by tests and it would be used in the same way that
the devm_* stuff is.
Nevertheless, I am fine adding the kunit_resource_destroy, etc. I just
wanted to make sure I understood what you were asking.
^ permalink raw reply
* Re: [PATCH 1/1] kbuild: recursive build of external kernel modules
From: Masahiro Yamada @ 2019-08-13 1:06 UTC (permalink / raw)
To: Shaun Tancheff
Cc: Shaun Tancheff, Linux Kbuild mailing list, Joe Lawrence,
James E . J . Bottomley, Jonathan Corbet, Martin K . Petersen,
Michal Marek, Shuah Khan, Thomas Renninger,
open list:DOCUMENTATION, Linux Kernel Mailing List,
Linux PM mailing list, linux-scsi
In-Reply-To: <CAJ48U8Xp40is+R1dMW8sXq77ZS5D_h+hHte5Mq5eOrtpb41Qxw@mail.gmail.com>
On Tue, Aug 13, 2019 at 2:34 AM Shaun Tancheff <shaun@tancheff.com> wrote:
>
> On Mon, Aug 12, 2019 at 10:24 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > On Fri, Aug 9, 2019 at 9:21 AM Shaun Tancheff <shaun@tancheff.com> wrote:
> > >
> > > When building a tree of external modules stage 2 fails
> > > silently as the root modules.order is empty.
> > >
> > > Modify the modules.order location to be fixed to the
> > > root when KBUILD_EXTMOD is specified and write all
> > > module paths to the single modules.order file.
> >
> > Could you try v5.3-rc4 please?
>
> So it seems we are using 'subdir-m' but that is now gone?
>
> Is there a recommend pattern for backward compatibility?
>
> Thanks!
Please convert
subdir-m += dir1
subdir-m += dir2
into
obj-m += dir1/
obj-m += dir2/
Thanks.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v12 09/18] kunit: test: add support for test abort
From: Stephen Boyd @ 2019-08-13 4:21 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190812182421.141150-10-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-08-12 11:24:12)
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 2625bcfeb19ac..93381f841e09f 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -13,6 +13,7 @@
> #include <linux/types.h>
> #include <linux/slab.h>
> #include <kunit/assert.h>
> +#include <kunit/try-catch.h>
>
> struct kunit_resource;
>
> @@ -167,6 +168,7 @@ struct kunit {
>
> /* private: internal use only. */
> const char *name; /* Read only after initialization! */
> + struct kunit_try_catch try_catch;
> /*
> * success starts as true, and may only be set to false during a test
> * case; thus, it is safe to update this across multiple threads using
> @@ -176,6 +178,11 @@ struct kunit {
> */
> bool success; /* Read only after test_case finishes! */
> spinlock_t lock; /* Gaurds all mutable test state. */
> + /*
> + * death_test may be both set and unset from multiple threads in a test
> + * case.
> + */
> + bool death_test; /* Protected by lock. */
> /*
> * Because resources is a list that may be updated multiple times (with
> * new resources) from any thread associated with a test case, we must
> @@ -184,6 +191,13 @@ struct kunit {
> struct list_head resources; /* Protected by lock. */
> };
>
> +static inline void kunit_set_death_test(struct kunit *test, bool death_test)
> +{
> + spin_lock(&test->lock);
> + test->death_test = death_test;
> + spin_unlock(&test->lock);
> +}
These getters and setters are using spinlocks again. It doesn't make any
sense. It probably needs a rework like was done for the other bool
member, success.
> +
> void kunit_init_test(struct kunit *test, const char *name);
>
> int kunit_run_tests(struct kunit_suite *suite);
> diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> new file mode 100644
> index 0000000000000..8a414a9af0b64
> --- /dev/null
> +++ b/include/kunit/try-catch.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * An API to allow a function, that may fail, to be executed, and recover in a
> + * controlled manner.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#ifndef _KUNIT_TRY_CATCH_H
> +#define _KUNIT_TRY_CATCH_H
> +
> +#include <linux/types.h>
> +
> +typedef void (*kunit_try_catch_func_t)(void *);
> +
> +struct kunit;
Forward declare struct completion?
> +
> +/*
> + * struct kunit_try_catch - provides a generic way to run code which might fail.
> + * @context: used to pass user data to the try and catch functions.
> + *
> + * kunit_try_catch provides a generic, architecture independent way to execute
> + * an arbitrary function of type kunit_try_catch_func_t which may bail out by
> + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, @try
> + * is stopped at the site of invocation and @catch is catch is called.
> + *
> + * struct kunit_try_catch provides a generic interface for the functionality
> + * needed to implement kunit->abort() which in turn is needed for implementing
> + * assertions. Assertions allow stating a precondition for a test simplifying
> + * how test cases are written and presented.
> + *
> + * Assertions are like expectations, except they abort (call
> + * kunit_try_catch_throw()) when the specified condition is not met. This is
> + * useful when you look at a test case as a logical statement about some piece
> + * of code, where assertions are the premises for the test case, and the
> + * conclusion is a set of predicates, rather expectations, that must all be
> + * true. If your premises are violated, it does not makes sense to continue.
> + */
> +struct kunit_try_catch {
> + /* private: internal use only. */
> + struct kunit *test;
> + struct completion *try_completion;
> + int try_result;
> + kunit_try_catch_func_t try;
> + kunit_try_catch_func_t catch;
Can these other variables be documented in the kernel doc? And should
context be marked as 'public'?
> + void *context;
> +};
> +
> +void kunit_try_catch_init(struct kunit_try_catch *try_catch,
> + struct kunit *test,
> + kunit_try_catch_func_t try,
> + kunit_try_catch_func_t catch);
> +
> +void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context);
> +
> +void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch);
> +
> +static inline int kunit_try_catch_get_result(struct kunit_try_catch *try_catch)
> +{
> + return try_catch->try_result;
> +}
> +
> +/*
> + * Exposed for testing only.
Ugh that's sad. I hope we don't expose more functions just for testing
in other cases.
> + */
> +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
> +
> +#endif /* _KUNIT_TRY_CATCH_H */
> diff --git a/kunit/test.c b/kunit/test.c
> index e5080a2c6b29c..995cb53fe4ee9 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
> @@ -7,13 +7,26 @@
> */
>
> #include <linux/kernel.h>
> +#include <linux/sched/debug.h>
> #include <kunit/test.h>
> +#include <kunit/try-catch.h>
>
> static void kunit_set_failure(struct kunit *test)
> {
> WRITE_ONCE(test->success, false);
> }
>
> +static bool kunit_get_death_test(struct kunit *test)
> +{
> + bool death_test;
> +
> + spin_lock(&test->lock);
> + death_test = test->death_test;
> + spin_unlock(&test->lock);
> +
> + return death_test;
> +}
> +
> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
> return vprintk_emit(0, level, NULL, 0, fmt, args);
> @@ -158,6 +171,21 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
> kunit_print_string_stream(test, stream);
> }
>
> +void __noreturn kunit_abort(struct kunit *test)
> +{
> + kunit_set_death_test(test, true);
> +
> + kunit_try_catch_throw(&test->try_catch);
> +
> + /*
> + * Throw could not abort from test.
> + *
> + * XXX: we should never reach this line! As kunit_try_catch_throw is
> + * marked __noreturn.
> + */
> + WARN_ONCE(true, "Throw could not abort from test!\n");
Should this just be a BUG_ON? It's supposedly impossible.
> +}
> +
> void kunit_do_assertion(struct kunit *test,
> struct kunit_assert *assert,
> bool pass,
> @@ -176,6 +204,9 @@ void kunit_do_assertion(struct kunit *test,
> kunit_fail(test, assert);
>
> va_end(args);
> +
> + if (assert->type == KUNIT_ASSERTION)
> + kunit_abort(test);
> }
>
> void kunit_init_test(struct kunit *test, const char *name)
> @@ -184,36 +215,154 @@ void kunit_init_test(struct kunit *test, const char *name)
> INIT_LIST_HEAD(&test->resources);
> test->name = name;
> test->success = true;
> + test->death_test = false;
> }
>
> /*
> - * Performs all logic to run a test case.
> + * Initializes and runs test case. Does not clean up or do post validations.
> */
> -static void kunit_run_case(struct kunit_suite *suite,
> - struct kunit_case *test_case)
> +static void kunit_run_case_internal(struct kunit *test,
> + struct kunit_suite *suite,
> + struct kunit_case *test_case)
> {
> - struct kunit test;
> -
> - kunit_init_test(&test, test_case->name);
> -
> if (suite->init) {
> int ret;
>
> - ret = suite->init(&test);
> + ret = suite->init(test);
> if (ret) {
> - kunit_err(&test, "failed to initialize: %d\n", ret);
> - kunit_set_failure(&test);
> - test_case->success = test.success;
> + kunit_err(test, "failed to initialize: %d\n", ret);
> + kunit_set_failure(test);
> return;
> }
> }
>
> - test_case->run_case(&test);
> + test_case->run_case(test);
> +}
> +
> +static void kunit_case_internal_cleanup(struct kunit *test)
> +{
> + kunit_cleanup(test);
> +}
>
> +/*
> + * Performs post validations and cleanup after a test case was run.
> + * XXX: Should ONLY BE CALLED AFTER kunit_run_case_internal!
> + */
> +static void kunit_run_case_cleanup(struct kunit *test,
> + struct kunit_suite *suite)
> +{
> if (suite->exit)
> - suite->exit(&test);
> + suite->exit(test);
> +
> + kunit_case_internal_cleanup(test);
> +}
> +
> +/*
> + * Handles an unexpected crash in a test case.
> + */
> +static void kunit_handle_test_crash(struct kunit *test,
> + struct kunit_suite *suite,
> + struct kunit_case *test_case)
> +{
> + kunit_err(test, "kunit test case crashed!");
Does this need a newline?
> + /*
> + * TODO(brendanhiggins@google.com): This prints the stack trace up
> + * through this frame, not up to the frame that caused the crash.
> + */
> + show_stack(NULL, NULL);
> +
> + kunit_case_internal_cleanup(test);
> +}
> +
> +struct kunit_try_catch_context {
> + struct kunit *test;
> + struct kunit_suite *suite;
> + struct kunit_case *test_case;
> +};
> +
> +static void kunit_try_run_case(void *data)
> +{
> + struct kunit_try_catch_context *ctx = data;
> + struct kunit *test = ctx->test;
> + struct kunit_suite *suite = ctx->suite;
> + struct kunit_case *test_case = ctx->test_case;
> +
> + /*
> + * kunit_run_case_internal may encounter a fatal error; if it does,
> + * abort will be called, this thread will exit, and finally the parent
> + * thread will resume control and handle any necessary clean up.
> + */
> + kunit_run_case_internal(test, suite, test_case);
> + /* This line may never be reached. */
> + kunit_run_case_cleanup(test, suite);
> +}
> +
> +static void kunit_catch_run_case(void *data)
> +{
> + struct kunit_try_catch_context *ctx = data;
> + struct kunit *test = ctx->test;
> + struct kunit_suite *suite = ctx->suite;
> + struct kunit_case *test_case = ctx->test_case;
> + int try_exit_code = kunit_try_catch_get_result(&test->try_catch);
> +
> + if (try_exit_code) {
> + kunit_set_failure(test);
> + /*
> + * Test case could not finish, we have no idea what state it is
> + * in, so don't do clean up.
> + */
> + if (try_exit_code == -ETIMEDOUT)
> + kunit_err(test, "test case timed out\n");
> + /*
> + * Unknown internal error occurred preventing test case from
> + * running, so there is nothing to clean up.
> + */
> + else
> + kunit_err(test, "internal error occurred preventing test case from running: %d\n",
> + try_exit_code);
Nitpick: I would add braces here because you make the if statement into
multi-line arms for each case.
> + return;
> + }
> +
> + if (kunit_get_death_test(test)) {
> + /*
> + * EXPECTED DEATH: kunit_run_case_internal encountered
> + * anticipated fatal error. Everything should be in a safe
> + * state.
> + */
> + kunit_run_case_cleanup(test, suite);
> + } else {
> + /*
> + * UNEXPECTED DEATH: kunit_run_case_internal encountered an
> + * unanticipated fatal error. We have no idea what the state of
> + * the test case is in.
> + */
> + kunit_handle_test_crash(test, suite, test_case);
> + kunit_set_failure(test);
Like was done here.
> + }
> +}
> +
> +/*
> + * Performs all logic to run a test case. It also catches most errors that
> + * occurs in a test case and reports them as failures.
s/occurs/occur/
> + */
> +static void kunit_run_case_catch_errors(struct kunit_suite *suite,
[...]
> diff --git a/kunit/try-catch.c b/kunit/try-catch.c
> new file mode 100644
> index 0000000000000..de580f074387b
> --- /dev/null
> +++ b/kunit/try-catch.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * An API to allow a function, that may fail, to be executed, and recover in a
> + * controlled manner.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#include <kunit/try-catch.h>
> +#include <kunit/test.h>
> +#include <linux/completion.h>
> +#include <linux/kthread.h>
> +
> +void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
> +{
> + try_catch->try_result = -EFAULT;
> + complete_and_exit(try_catch->try_completion, -EFAULT);
> +}
> +
> +static int kunit_generic_run_threadfn_adapter(void *data)
> +{
> + struct kunit_try_catch *try_catch = data;
> +
> + try_catch->try(try_catch->context);
> +
> + complete_and_exit(try_catch->try_completion, 0);
> +}
> +
> +void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> +{
> + DECLARE_COMPLETION_ONSTACK(try_completion);
> + struct kunit *test = try_catch->test;
> + struct task_struct *task_struct;
> + int exit_code, status;
> +
> + try_catch->context = context;
> + try_catch->try_completion = &try_completion;
> + try_catch->try_result = 0;
> + task_struct = kthread_run(kunit_generic_run_threadfn_adapter,
> + try_catch,
> + "kunit_try_catch_thread");
> + if (IS_ERR(task_struct)) {
> + try_catch->catch(try_catch->context);
> + return;
> + }
> +
> + /*
> + * TODO(brendanhiggins@google.com): We should probably have some type of
> + * variable timeout here. The only question is what that timeout value
> + * should be.
> + *
> + * The intention has always been, at some point, to be able to label
> + * tests with some type of size bucket (unit/small, integration/medium,
> + * large/system/end-to-end, etc), where each size bucket would get a
> + * default timeout value kind of like what Bazel does:
> + * https://docs.bazel.build/versions/master/be/common-definitions.html#test.size
> + * There is still some debate to be had on exactly how we do this. (For
> + * one, we probably want to have some sort of test runner level
> + * timeout.)
> + *
> + * For more background on this topic, see:
> + * https://mike-bland.com/2011/11/01/small-medium-large.html
> + */
> + status = wait_for_completion_timeout(&try_completion,
> + 300 * MSEC_PER_SEC); /* 5 min */
> + if (status < 0) {
wait_for_completion_timeout() doesn't return a negative value on
timeout. It returns 0. Please rename 'status' to 'time_remaining' and
test with if (!time_remaining) instead or some other suitably named
variable name indicating that the return value is the time remaining
before the timeout.
May also want to clamp this to the hung task timeout value, which is
typically less than 5 minutes. Otherwise, the hung task detector may
find the problem first before this timeout happens.
> + kunit_err(test, "try timed out\n");
> + try_catch->try_result = -ETIMEDOUT;
> + }
^ permalink raw reply
* Re: [PATCH v12 10/18] kunit: test: add tests for kunit test abort
From: Stephen Boyd @ 2019-08-13 4:24 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190812182421.141150-11-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-08-12 11:24:13)
> +
> +static int kunit_try_catch_test_init(struct kunit *test)
> +{
> + struct kunit_try_catch_test_context *ctx;
> +
> + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
Can this fail? Should return -ENOMEM in that case?
> + test->priv = ctx;
> +
> + ctx->try_catch = kunit_kmalloc(test,
> + sizeof(*ctx->try_catch),
> + GFP_KERNEL);
^ permalink raw reply
* Re: [PATCH v12 04/18] kunit: test: add assertion printing library
From: Brendan Higgins @ 2019-08-13 4:27 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g44huOiR9B0H1C2TtiPy63BDuwi_Qpb_exF3zmT3ttV8eg@mail.gmail.com>
On Mon, Aug 12, 2019 at 4:56 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Mon, Aug 12, 2019 at 4:46 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 11:24:07)
> > > Add `struct kunit_assert` and friends which provide a structured way to
> > > capture data from an expectation or an assertion (introduced later in
> > > the series) so that it may be printed out in the event of a failure.
> > >
> > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > > ---
> >
> > Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> >
> > Just some minor nits below
> >
> > > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > > new file mode 100644
> > > index 0000000000000..55f1b88b0cb4d
> > > --- /dev/null
> > > +++ b/include/kunit/assert.h
> > > @@ -0,0 +1,183 @@
> > [...]
> > > + struct string_stream *stream);
> > > +
> > > +struct kunit_fail_assert {
> > > + struct kunit_assert assert;
> > > +};
> > > +
> > > +void kunit_fail_assert_format(const struct kunit_assert *assert,
> > > + struct string_stream *stream);
> > > +
> > > +#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) { \
> > > + .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
> > > + type, \
> > > + kunit_fail_assert_format) \
> >
> > This one got indented one too many times?
>
> Not unless I have been using the wrong formatting for multiline
> macros. You can see this commit applied here:
> https://kunit.googlesource.com/linux/+/870964da2990920030990dd1ffb647ef408e52df/include/kunit/assert.h#59
>
> I have test, type, and kunit_fail_assert_format all column aligned (it
> just doesn't render nicely in the patch format).
Disregard that last comment. I just looked at the line immediately
above your comment and thought it looked correct. Sorry about that
(you were pointing out that the .assert line looked wrong, correct?).
> > > +}
> > > +
> > > +struct kunit_unary_assert {
> > > + struct kunit_assert assert;
> > > + const char *condition;
> > > + bool expected_true;
> > > +};
> > > +
> > > +void kunit_unary_assert_format(const struct kunit_assert *assert,
> > > + struct string_stream *stream);
> > > +
> > [...]
> > > +#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test, \
> > > + type, \
> > > + op_str, \
> > > + left_str, \
> > > + left_val, \
> > > + right_str, \
> > > + right_val) { \
> > > + .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
> > > + type, \
> > > + kunit_binary_str_assert_format), \
> > > + .operation = op_str, \
> > > + .left_text = left_str, \
> > > + .left_value = left_val, \
> > > + .right_text = right_str, \
> > > + .right_value = right_val \
> > > +}
> >
> > It would be nice to have kernel doc on these macros so we know how to
> > use them.
>
> Sounds good. Will fix.
^ permalink raw reply
* Re: [PATCH v12 12/18] kunit: test: add tests for KUnit managed resources
From: Stephen Boyd @ 2019-08-13 4:31 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Avinash Kondareddy, Brendan Higgins
In-Reply-To: <20190812182421.141150-13-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-08-12 11:24:15)
> +
> +static int kunit_resource_test_init(struct kunit *test)
> +{
> + struct kunit_test_resource_context *ctx =
> + kzalloc(sizeof(*ctx), GFP_KERNEL);
> +
> + if (!ctx)
> + return -ENOMEM;
Should this use the test assertion logic to make sure that it's not
failing allocations during init? BTW, maybe kunit allocation APIs should
fail the test if they fail to allocate in general. Unless we're unit
testing failure to allocate problems.
> +
> + test->priv = ctx;
> +
> + kunit_init_test(&ctx->test, "test_test_context");
> +
> + return 0;
^ permalink raw reply
* Re: [PATCH v12 14/18] kunit: defconfig: add defconfigs for building KUnit tests
From: Stephen Boyd @ 2019-08-13 4:38 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190812182421.141150-15-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-08-12 11:24:17)
> diff --git a/arch/um/configs/kunit_defconfig b/arch/um/configs/kunit_defconfig
> new file mode 100644
> index 0000000000000..bfe49689038f1
> --- /dev/null
> +++ b/arch/um/configs/kunit_defconfig
> @@ -0,0 +1,8 @@
> +CONFIG_OF=y
> +CONFIG_OF_UNITTEST=y
> +CONFIG_OF_OVERLAY=y
> +CONFIG_I2C=y
> +CONFIG_I2C_MUX=y
> +CONFIG_KUNIT=y
> +CONFIG_KUNIT_TEST=y
> +CONFIG_KUNIT_EXAMPLE_TEST=y
> diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
> new file mode 100644
> index 0000000000000..bfe49689038f1
> --- /dev/null
> +++ b/tools/testing/kunit/configs/all_tests.config
> @@ -0,0 +1,8 @@
> +CONFIG_OF=y
> +CONFIG_OF_UNITTEST=y
> +CONFIG_OF_OVERLAY=y
> +CONFIG_I2C=y
> +CONFIG_I2C_MUX=y
Are these above config options necessary? I don't think they're part of
the patch series anymore so it looks odd to enable the OF unittests and
i2c configs.
> +CONFIG_KUNIT=y
> +CONFIG_KUNIT_TEST=y
> +CONFIG_KUNIT_EXAMPLE_TEST=y
^ permalink raw reply
* Re: [PATCH v12 15/18] Documentation: kunit: add documentation for KUnit
From: Stephen Boyd @ 2019-08-13 4:46 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins, Felix Guo, Jonathan Corbet
In-Reply-To: <20190812182421.141150-16-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-08-12 11:24:18)
> Add documentation for KUnit, the Linux kernel unit testing framework.
> - Add intro and usage guide for KUnit
> - Add API reference
>
> Signed-off-by: Felix Guo <felixguoxiuping@gmail.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply
* Re: [PATCH v12 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
From: Stephen Boyd @ 2019-08-13 4:48 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Iurii Zaikin, Brendan Higgins
In-Reply-To: <20190812182421.141150-18-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-08-12 11:24:20)
> From: Iurii Zaikin <yzaikin@google.com>
>
> KUnit tests for initialized data behavior of proc_dointvec that is
> explicitly checked in the code. Includes basic parsing tests including
> int min/max overflow.
>
> Signed-off-by: Iurii Zaikin <yzaikin@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> Acked-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply
* Re: [PATCH v12 11/18] kunit: test: add the concept of assertions
From: Stephen Boyd @ 2019-08-13 4:55 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190812182421.141150-12-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-08-12 11:24:14)
> Add support for assertions which are like expectations except the test
> terminates if the assertion is not satisfied.
>
> The idea with assertions is that you use them to state all the
> preconditions for your test. Logically speaking, these are the premises
> of the test case, so if a premise isn't true, there is no point in
> continuing the test case because there are no conclusions that can be
> drawn without the premises. Whereas, the expectation is the thing you
> are trying to prove. It is not used universally in x-unit style test
> frameworks, but I really like it as a convention. You could still
> express the idea of a premise using the above idiom, but I think
> KUNIT_ASSERT_* states the intended idea perfectly.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> + * Sets an expectation that the values that @left and @right evaluate to are
> + * not equal. This is semantically equivalent to
> + * KUNIT_ASSERT_TRUE(@test, strcmp((@left), (@right))). See KUNIT_ASSERT_TRUE()
> + * for more information.
> + */
> +#define KUNIT_ASSERT_STRNEQ(test, left, right) \
> + KUNIT_BINARY_STR_NE_ASSERTION(test, \
> + KUNIT_ASSERTION, \
> + left, \
> + right)
> +
> +#define KUNIT_ASSERT_STRNEQ_MSG(test, left, right, fmt, ...) \
> + KUNIT_BINARY_STR_NE_MSG_ASSERTION(test, \
> + KUNIT_ASSERTION, \
> + left, \
> + right, \
> + fmt, \
Same question about tabbing too.
> diff --git a/kunit/test-test.c b/kunit/test-test.c
> index 88f4cdf03db2a..058f3fb37458a 100644
> --- a/kunit/test-test.c
> +++ b/kunit/test-test.c
> @@ -78,11 +78,13 @@ static int kunit_try_catch_test_init(struct kunit *test)
> struct kunit_try_catch_test_context *ctx;
>
> ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
Ah ok. Question still stands if kunit_kzalloc() should just have the
assertion on failure.
> test->priv = ctx;
>
> ctx->try_catch = kunit_kmalloc(test,
> sizeof(*ctx->try_catch),
> GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->try_catch);
>
^ permalink raw reply
* Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
From: Stephen Boyd @ 2019-08-13 4:56 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g44xciLPBhH_J3zUcY3TedWTijdnWgF055qffF+dAguhPQ@mail.gmail.com>
Quoting Brendan Higgins (2019-08-12 17:41:05)
> On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > > devres_destroy) and use kunit_kfree here?
> > >
> >
> > Yes, or drop the API entirely? Does anything need this functionality?
>
> Drop the kunit_resource API? I would strongly prefer not to.
No. I mean this API, string_stream_clear(). Does anything use it?
^ permalink raw reply
* Re: [PATCH v12 04/18] kunit: test: add assertion printing library
From: Stephen Boyd @ 2019-08-13 4:57 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g44GxE-p+Jk_46GYA-WWVHLW7w=yE+K_tbbdiniDfrk-2w@mail.gmail.com>
Quoting Brendan Higgins (2019-08-12 21:27:05)
> On Mon, Aug 12, 2019 at 4:56 PM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > On Mon, Aug 12, 2019 at 4:46 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-08-12 11:24:07)
> > > > +#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) { \
> > > > + .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
> > > > + type, \
> > > > + kunit_fail_assert_format) \
> > >
> > > This one got indented one too many times?
> >
> > Not unless I have been using the wrong formatting for multiline
> > macros. You can see this commit applied here:
> > https://kunit.googlesource.com/linux/+/870964da2990920030990dd1ffb647ef408e52df/include/kunit/assert.h#59
> >
> > I have test, type, and kunit_fail_assert_format all column aligned (it
> > just doesn't render nicely in the patch format).
>
> Disregard that last comment. I just looked at the line immediately
> above your comment and thought it looked correct. Sorry about that
> (you were pointing out that the .assert line looked wrong, correct?).
Yes. .assert is double tabbed?
^ permalink raw reply
* Re: [PATCH v12 09/18] kunit: test: add support for test abort
From: Brendan Higgins @ 2019-08-13 4:57 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813042159.46814206C2@mail.kernel.org>
On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:12)
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 2625bcfeb19ac..93381f841e09f 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -13,6 +13,7 @@
> > #include <linux/types.h>
> > #include <linux/slab.h>
> > #include <kunit/assert.h>
> > +#include <kunit/try-catch.h>
> >
> > struct kunit_resource;
> >
> > @@ -167,6 +168,7 @@ struct kunit {
> >
> > /* private: internal use only. */
> > const char *name; /* Read only after initialization! */
> > + struct kunit_try_catch try_catch;
> > /*
> > * success starts as true, and may only be set to false during a test
> > * case; thus, it is safe to update this across multiple threads using
> > @@ -176,6 +178,11 @@ struct kunit {
> > */
> > bool success; /* Read only after test_case finishes! */
> > spinlock_t lock; /* Gaurds all mutable test state. */
> > + /*
> > + * death_test may be both set and unset from multiple threads in a test
> > + * case.
> > + */
> > + bool death_test; /* Protected by lock. */
> > /*
> > * Because resources is a list that may be updated multiple times (with
> > * new resources) from any thread associated with a test case, we must
> > @@ -184,6 +191,13 @@ struct kunit {
> > struct list_head resources; /* Protected by lock. */
> > };
> >
> > +static inline void kunit_set_death_test(struct kunit *test, bool death_test)
> > +{
> > + spin_lock(&test->lock);
> > + test->death_test = death_test;
> > + spin_unlock(&test->lock);
> > +}
>
> These getters and setters are using spinlocks again. It doesn't make any
> sense. It probably needs a rework like was done for the other bool
> member, success.
No, this is intentional. death_test can transition from false to true
and then back to false within the same test. Maybe that deserves a
comment?
> > +
> > void kunit_init_test(struct kunit *test, const char *name);
> >
> > int kunit_run_tests(struct kunit_suite *suite);
> > diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> > new file mode 100644
> > index 0000000000000..8a414a9af0b64
> > --- /dev/null
> > +++ b/include/kunit/try-catch.h
> > @@ -0,0 +1,69 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * An API to allow a function, that may fail, to be executed, and recover in a
> > + * controlled manner.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#ifndef _KUNIT_TRY_CATCH_H
> > +#define _KUNIT_TRY_CATCH_H
> > +
> > +#include <linux/types.h>
> > +
> > +typedef void (*kunit_try_catch_func_t)(void *);
> > +
> > +struct kunit;
>
> Forward declare struct completion?
Sure. Will do.
> > +
> > +/*
> > + * struct kunit_try_catch - provides a generic way to run code which might fail.
> > + * @context: used to pass user data to the try and catch functions.
> > + *
> > + * kunit_try_catch provides a generic, architecture independent way to execute
> > + * an arbitrary function of type kunit_try_catch_func_t which may bail out by
> > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, @try
> > + * is stopped at the site of invocation and @catch is catch is called.
> > + *
> > + * struct kunit_try_catch provides a generic interface for the functionality
> > + * needed to implement kunit->abort() which in turn is needed for implementing
> > + * assertions. Assertions allow stating a precondition for a test simplifying
> > + * how test cases are written and presented.
> > + *
> > + * Assertions are like expectations, except they abort (call
> > + * kunit_try_catch_throw()) when the specified condition is not met. This is
> > + * useful when you look at a test case as a logical statement about some piece
> > + * of code, where assertions are the premises for the test case, and the
> > + * conclusion is a set of predicates, rather expectations, that must all be
> > + * true. If your premises are violated, it does not makes sense to continue.
> > + */
> > +struct kunit_try_catch {
> > + /* private: internal use only. */
> > + struct kunit *test;
> > + struct completion *try_completion;
> > + int try_result;
> > + kunit_try_catch_func_t try;
> > + kunit_try_catch_func_t catch;
>
> Can these other variables be documented in the kernel doc? And should
> context be marked as 'public'?
Sure, I can document them.
But I don't think context should be public; it should only be accessed
by kunit_try_catch_* functions. context should only be populated by
*_init, and will be passed into *try and *catch when they are called
internally.
> > + void *context;
> > +};
> > +
> > +void kunit_try_catch_init(struct kunit_try_catch *try_catch,
> > + struct kunit *test,
> > + kunit_try_catch_func_t try,
> > + kunit_try_catch_func_t catch);
> > +
> > +void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context);
> > +
> > +void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch);
> > +
> > +static inline int kunit_try_catch_get_result(struct kunit_try_catch *try_catch)
> > +{
> > + return try_catch->try_result;
> > +}
> > +
> > +/*
> > + * Exposed for testing only.
>
> Ugh that's sad. I hope we don't expose more functions just for testing
> in other cases.
I don't think I am in any other cases in this patchset. I agree that
it is generally bad to expose a private function for testing purposes,
but I didn't see a better way here.
> > + */
> > +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
> > +
> > +#endif /* _KUNIT_TRY_CATCH_H */
> > diff --git a/kunit/test.c b/kunit/test.c
> > index e5080a2c6b29c..995cb53fe4ee9 100644
> > --- a/kunit/test.c
> > +++ b/kunit/test.c
> > @@ -7,13 +7,26 @@
> > */
> >
> > #include <linux/kernel.h>
> > +#include <linux/sched/debug.h>
> > #include <kunit/test.h>
> > +#include <kunit/try-catch.h>
> >
> > static void kunit_set_failure(struct kunit *test)
> > {
> > WRITE_ONCE(test->success, false);
> > }
> >
> > +static bool kunit_get_death_test(struct kunit *test)
> > +{
> > + bool death_test;
> > +
> > + spin_lock(&test->lock);
> > + death_test = test->death_test;
> > + spin_unlock(&test->lock);
> > +
> > + return death_test;
> > +}
> > +
> > static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > {
> > return vprintk_emit(0, level, NULL, 0, fmt, args);
> > @@ -158,6 +171,21 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
> > kunit_print_string_stream(test, stream);
> > }
> >
> > +void __noreturn kunit_abort(struct kunit *test)
> > +{
> > + kunit_set_death_test(test, true);
> > +
> > + kunit_try_catch_throw(&test->try_catch);
> > +
> > + /*
> > + * Throw could not abort from test.
> > + *
> > + * XXX: we should never reach this line! As kunit_try_catch_throw is
> > + * marked __noreturn.
> > + */
> > + WARN_ONCE(true, "Throw could not abort from test!\n");
>
> Should this just be a BUG_ON? It's supposedly impossible.
It should be impossible; it will only reach this line if there is a
bug in kunit_try_catch_throw. The reason I didn't use BUG_ON was
because I previously got yelled at for having BUG_ON in this code
path.
Nevertheless, I think BUG_ON is more correct, so if you will stand by
it, then that's what I will do.
> > +}
> > +
> > void kunit_do_assertion(struct kunit *test,
> > struct kunit_assert *assert,
> > bool pass,
> > @@ -176,6 +204,9 @@ void kunit_do_assertion(struct kunit *test,
> > kunit_fail(test, assert);
> >
> > va_end(args);
> > +
> > + if (assert->type == KUNIT_ASSERTION)
> > + kunit_abort(test);
> > }
> >
> > void kunit_init_test(struct kunit *test, const char *name)
> > @@ -184,36 +215,154 @@ void kunit_init_test(struct kunit *test, const char *name)
> > INIT_LIST_HEAD(&test->resources);
> > test->name = name;
> > test->success = true;
> > + test->death_test = false;
> > }
> >
> > /*
> > - * Performs all logic to run a test case.
> > + * Initializes and runs test case. Does not clean up or do post validations.
> > */
> > -static void kunit_run_case(struct kunit_suite *suite,
> > - struct kunit_case *test_case)
> > +static void kunit_run_case_internal(struct kunit *test,
> > + struct kunit_suite *suite,
> > + struct kunit_case *test_case)
> > {
> > - struct kunit test;
> > -
> > - kunit_init_test(&test, test_case->name);
> > -
> > if (suite->init) {
> > int ret;
> >
> > - ret = suite->init(&test);
> > + ret = suite->init(test);
> > if (ret) {
> > - kunit_err(&test, "failed to initialize: %d\n", ret);
> > - kunit_set_failure(&test);
> > - test_case->success = test.success;
> > + kunit_err(test, "failed to initialize: %d\n", ret);
> > + kunit_set_failure(test);
> > return;
> > }
> > }
> >
> > - test_case->run_case(&test);
> > + test_case->run_case(test);
> > +}
> > +
> > +static void kunit_case_internal_cleanup(struct kunit *test)
> > +{
> > + kunit_cleanup(test);
> > +}
> >
> > +/*
> > + * Performs post validations and cleanup after a test case was run.
> > + * XXX: Should ONLY BE CALLED AFTER kunit_run_case_internal!
> > + */
> > +static void kunit_run_case_cleanup(struct kunit *test,
> > + struct kunit_suite *suite)
> > +{
> > if (suite->exit)
> > - suite->exit(&test);
> > + suite->exit(test);
> > +
> > + kunit_case_internal_cleanup(test);
> > +}
> > +
> > +/*
> > + * Handles an unexpected crash in a test case.
> > + */
> > +static void kunit_handle_test_crash(struct kunit *test,
> > + struct kunit_suite *suite,
> > + struct kunit_case *test_case)
> > +{
> > + kunit_err(test, "kunit test case crashed!");
>
> Does this need a newline?
Yep, nice catch. I thought I grepped for all the instance a while ago,
but I apparently missed this one.
> > + /*
> > + * TODO(brendanhiggins@google.com): This prints the stack trace up
> > + * through this frame, not up to the frame that caused the crash.
> > + */
> > + show_stack(NULL, NULL);
> > +
> > + kunit_case_internal_cleanup(test);
> > +}
> > +
> > +struct kunit_try_catch_context {
> > + struct kunit *test;
> > + struct kunit_suite *suite;
> > + struct kunit_case *test_case;
> > +};
> > +
> > +static void kunit_try_run_case(void *data)
> > +{
> > + struct kunit_try_catch_context *ctx = data;
> > + struct kunit *test = ctx->test;
> > + struct kunit_suite *suite = ctx->suite;
> > + struct kunit_case *test_case = ctx->test_case;
> > +
> > + /*
> > + * kunit_run_case_internal may encounter a fatal error; if it does,
> > + * abort will be called, this thread will exit, and finally the parent
> > + * thread will resume control and handle any necessary clean up.
> > + */
> > + kunit_run_case_internal(test, suite, test_case);
> > + /* This line may never be reached. */
> > + kunit_run_case_cleanup(test, suite);
> > +}
> > +
> > +static void kunit_catch_run_case(void *data)
> > +{
> > + struct kunit_try_catch_context *ctx = data;
> > + struct kunit *test = ctx->test;
> > + struct kunit_suite *suite = ctx->suite;
> > + struct kunit_case *test_case = ctx->test_case;
> > + int try_exit_code = kunit_try_catch_get_result(&test->try_catch);
> > +
> > + if (try_exit_code) {
> > + kunit_set_failure(test);
> > + /*
> > + * Test case could not finish, we have no idea what state it is
> > + * in, so don't do clean up.
> > + */
> > + if (try_exit_code == -ETIMEDOUT)
> > + kunit_err(test, "test case timed out\n");
> > + /*
> > + * Unknown internal error occurred preventing test case from
> > + * running, so there is nothing to clean up.
> > + */
> > + else
> > + kunit_err(test, "internal error occurred preventing test case from running: %d\n",
> > + try_exit_code);
>
> Nitpick: I would add braces here because you make the if statement into
> multi-line arms for each case.
Will do. I think it looks better with braces anyway.
> > + return;
> > + }
> > +
> > + if (kunit_get_death_test(test)) {
> > + /*
> > + * EXPECTED DEATH: kunit_run_case_internal encountered
> > + * anticipated fatal error. Everything should be in a safe
> > + * state.
> > + */
> > + kunit_run_case_cleanup(test, suite);
> > + } else {
> > + /*
> > + * UNEXPECTED DEATH: kunit_run_case_internal encountered an
> > + * unanticipated fatal error. We have no idea what the state of
> > + * the test case is in.
> > + */
> > + kunit_handle_test_crash(test, suite, test_case);
> > + kunit_set_failure(test);
>
> Like was done here.
Sorry, like what?
> > + }
> > +}
> > +
> > +/*
> > + * Performs all logic to run a test case. It also catches most errors that
> > + * occurs in a test case and reports them as failures.
>
> s/occurs/occur/
Damn, I should go over all these with spell check. Will fix, thanks!
> > + */
> > +static void kunit_run_case_catch_errors(struct kunit_suite *suite,
> [...]
> > diff --git a/kunit/try-catch.c b/kunit/try-catch.c
> > new file mode 100644
> > index 0000000000000..de580f074387b
> > --- /dev/null
> > +++ b/kunit/try-catch.c
> > @@ -0,0 +1,95 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * An API to allow a function, that may fail, to be executed, and recover in a
> > + * controlled manner.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#include <kunit/try-catch.h>
> > +#include <kunit/test.h>
> > +#include <linux/completion.h>
> > +#include <linux/kthread.h>
> > +
> > +void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
> > +{
> > + try_catch->try_result = -EFAULT;
> > + complete_and_exit(try_catch->try_completion, -EFAULT);
> > +}
> > +
> > +static int kunit_generic_run_threadfn_adapter(void *data)
> > +{
> > + struct kunit_try_catch *try_catch = data;
> > +
> > + try_catch->try(try_catch->context);
> > +
> > + complete_and_exit(try_catch->try_completion, 0);
> > +}
> > +
> > +void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> > +{
> > + DECLARE_COMPLETION_ONSTACK(try_completion);
> > + struct kunit *test = try_catch->test;
> > + struct task_struct *task_struct;
> > + int exit_code, status;
> > +
> > + try_catch->context = context;
> > + try_catch->try_completion = &try_completion;
> > + try_catch->try_result = 0;
> > + task_struct = kthread_run(kunit_generic_run_threadfn_adapter,
> > + try_catch,
> > + "kunit_try_catch_thread");
> > + if (IS_ERR(task_struct)) {
> > + try_catch->catch(try_catch->context);
> > + return;
> > + }
> > +
> > + /*
> > + * TODO(brendanhiggins@google.com): We should probably have some type of
> > + * variable timeout here. The only question is what that timeout value
> > + * should be.
> > + *
> > + * The intention has always been, at some point, to be able to label
> > + * tests with some type of size bucket (unit/small, integration/medium,
> > + * large/system/end-to-end, etc), where each size bucket would get a
> > + * default timeout value kind of like what Bazel does:
> > + * https://docs.bazel.build/versions/master/be/common-definitions.html#test.size
> > + * There is still some debate to be had on exactly how we do this. (For
> > + * one, we probably want to have some sort of test runner level
> > + * timeout.)
> > + *
> > + * For more background on this topic, see:
> > + * https://mike-bland.com/2011/11/01/small-medium-large.html
> > + */
> > + status = wait_for_completion_timeout(&try_completion,
> > + 300 * MSEC_PER_SEC); /* 5 min */
> > + if (status < 0) {
>
> wait_for_completion_timeout() doesn't return a negative value on
> timeout. It returns 0. Please rename 'status' to 'time_remaining' and
> test with if (!time_remaining) instead or some other suitably named
> variable name indicating that the return value is the time remaining
> before the timeout.
Crap, I knew that. Sorry, I wasn't thinking.
> May also want to clamp this to the hung task timeout value, which is
> typically less than 5 minutes. Otherwise, the hung task detector may
> find the problem first before this timeout happens.
Makes sense. Will fix.
> > + kunit_err(test, "try timed out\n");
> > + try_catch->try_result = -ETIMEDOUT;
> > + }
^ permalink raw reply
* Re: [PATCH v12 05/18] kunit: test: add the concept of expectations
From: Stephen Boyd @ 2019-08-13 5:02 UTC (permalink / raw)
To: Brendan Higgins
Cc: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, shuah, tytso, yamada.masahiro, devicetree,
dri-devel, kunit-dev, linux-doc, linux-fsdevel, linux-kbuild,
linux-kernel, linux-kselftest, linux-nvdimm, linux-um,
Alexander.Levin, Tim.Bird, amir73il, dan.carpenter, daniel, jdike,
joel, julia.lawall, khilman, knut.omang, logang, mpe, pmladek,
rdunlap, richard, rientjes, rostedt, wfg
In-Reply-To: <20190813003352.GA235915@google.com>
Quoting Brendan Higgins (2019-08-12 17:33:52)
> On Mon, Aug 12, 2019 at 04:57:00PM -0700, Stephen Boyd wrote:
> > Quoting Brendan Higgins (2019-08-12 11:24:08)
> > > + */
> > > +#define KUNIT_EXPECT_TRUE(test, condition) \
> > > + KUNIT_TRUE_ASSERTION(test, KUNIT_EXPECTATION, condition)
> >
> > A lot of these macros seem double indented.
>
> In a case you pointed out in the preceding patch, I was just keeping the
> arguments column aligned.
>
> In this case I am just indenting two tabs for a line continuation. I
> thought I found other instances in the kernel that did this early on
> (and that's also what the Linux kernel vim plugin wanted me to do).
> After a couple of spot checks, it seems like one tab for this kind of
> line continuation seems more common. I personally don't feel strongly
> about any particular version. I just want to know now what the correct
> indentation is for macros before I go through and change them all.
>
> I think there are three cases:
>
> #define macro0(param0, param1) \
> a_really_long_macro(...)
>
> In this first case, I use two tabs for the first indent, I think you are
> telling me this should be one tab.
Yes. Should be one.
>
> #define macro1(param0, param1) { \
> statement_in_a_block0; \
> statement_in_a_block1; \
> ... \
> }
>
> In this case, every line is in a block and is indented as it would be in
> a function body. I think you are okay with this, and now that I am
> thinking about it, what I think you are proposing for macro0 will make
> these two cases more consistent.
>
> #define macro2(param0, \
> param1, \
> param2, \
> param3, \
> ..., \
> paramn) ... \
>
> In this last case, the body would be indented as in macro0, or macro1,
> but the parameters passed into the macro are column aligned, consistent
> with one of the acceptable ways of formatting function parameters that
> don't fit on a single line.
>
> In all cases, I put 1 space in between the closing parameter paren and
> the line continuation `\`, if only one `\` is needed. Otherwise, I align
> all the `\s` to the 80th column. Is this okay, or would you prefer that
> I align them all to the 80th column, or something else?
>
This all sounds fine and I'm not nitpicking this style. Just the double
tabs making lines longer than required.
^ permalink raw reply
* Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
From: Brendan Higgins @ 2019-08-13 5:02 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813045623.F3D9520842@mail.kernel.org>
On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 17:41:05)
> > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > > > devres_destroy) and use kunit_kfree here?
> > > >
> > >
> > > Yes, or drop the API entirely? Does anything need this functionality?
> >
> > Drop the kunit_resource API? I would strongly prefer not to.
>
> No. I mean this API, string_stream_clear(). Does anything use it?
Oh, right. No.
However, now that I added the kunit_resource_destroy, I thought it
might be good to free the string_stream after I use it in each call to
kunit_assert->format(...) in which case I will be using this logic.
So I think the right thing to do is to expose string_stream_destroy so
kunit_do_assert can clean up when it's done, and then demote
string_stream_clear to static. Sound good?
^ permalink raw reply
* Re: [PATCH v12 04/18] kunit: test: add assertion printing library
From: Brendan Higgins @ 2019-08-13 5:03 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813045747.3AF0A206C2@mail.kernel.org>
On Mon, Aug 12, 2019 at 9:57 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 21:27:05)
> > On Mon, Aug 12, 2019 at 4:56 PM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> > >
> > > On Mon, Aug 12, 2019 at 4:46 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Brendan Higgins (2019-08-12 11:24:07)
> > > > > +#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) { \
> > > > > + .assert = KUNIT_INIT_ASSERT_STRUCT(test, \
> > > > > + type, \
> > > > > + kunit_fail_assert_format) \
> > > >
> > > > This one got indented one too many times?
> > >
> > > Not unless I have been using the wrong formatting for multiline
> > > macros. You can see this commit applied here:
> > > https://kunit.googlesource.com/linux/+/870964da2990920030990dd1ffb647ef408e52df/include/kunit/assert.h#59
> > >
> > > I have test, type, and kunit_fail_assert_format all column aligned (it
> > > just doesn't render nicely in the patch format).
> >
> > Disregard that last comment. I just looked at the line immediately
> > above your comment and thought it looked correct. Sorry about that
> > (you were pointing out that the .assert line looked wrong, correct?).
>
> Yes. .assert is double tabbed?
Yes it is. Sorry about the confusion. Will fix.
^ permalink raw reply
* Re: [PATCH v12 05/18] kunit: test: add the concept of expectations
From: Brendan Higgins @ 2019-08-13 5:04 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813050206.2A49C206C2@mail.kernel.org>
On Mon, Aug 12, 2019 at 10:02 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 17:33:52)
> > On Mon, Aug 12, 2019 at 04:57:00PM -0700, Stephen Boyd wrote:
> > > Quoting Brendan Higgins (2019-08-12 11:24:08)
> > > > + */
> > > > +#define KUNIT_EXPECT_TRUE(test, condition) \
> > > > + KUNIT_TRUE_ASSERTION(test, KUNIT_EXPECTATION, condition)
> > >
> > > A lot of these macros seem double indented.
> >
> > In a case you pointed out in the preceding patch, I was just keeping the
> > arguments column aligned.
> >
> > In this case I am just indenting two tabs for a line continuation. I
> > thought I found other instances in the kernel that did this early on
> > (and that's also what the Linux kernel vim plugin wanted me to do).
> > After a couple of spot checks, it seems like one tab for this kind of
> > line continuation seems more common. I personally don't feel strongly
> > about any particular version. I just want to know now what the correct
> > indentation is for macros before I go through and change them all.
> >
> > I think there are three cases:
> >
> > #define macro0(param0, param1) \
> > a_really_long_macro(...)
> >
> > In this first case, I use two tabs for the first indent, I think you are
> > telling me this should be one tab.
>
> Yes. Should be one.
>
> >
> > #define macro1(param0, param1) { \
> > statement_in_a_block0; \
> > statement_in_a_block1; \
> > ... \
> > }
> >
> > In this case, every line is in a block and is indented as it would be in
> > a function body. I think you are okay with this, and now that I am
> > thinking about it, what I think you are proposing for macro0 will make
> > these two cases more consistent.
> >
> > #define macro2(param0, \
> > param1, \
> > param2, \
> > param3, \
> > ..., \
> > paramn) ... \
> >
> > In this last case, the body would be indented as in macro0, or macro1,
> > but the parameters passed into the macro are column aligned, consistent
> > with one of the acceptable ways of formatting function parameters that
> > don't fit on a single line.
> >
> > In all cases, I put 1 space in between the closing parameter paren and
> > the line continuation `\`, if only one `\` is needed. Otherwise, I align
> > all the `\s` to the 80th column. Is this okay, or would you prefer that
> > I align them all to the 80th column, or something else?
> >
>
> This all sounds fine and I'm not nitpicking this style. Just the double
> tabs making lines longer than required.
Sounds good. Will do.
^ permalink raw reply
* Re: [PATCH v12 10/18] kunit: test: add tests for kunit test abort
From: Brendan Higgins @ 2019-08-13 5:06 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813042455.4A04320644@mail.kernel.org>
On Mon, Aug 12, 2019 at 9:24 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:13)
> > +
> > +static int kunit_try_catch_test_init(struct kunit *test)
> > +{
> > + struct kunit_try_catch_test_context *ctx;
> > +
> > + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
>
> Can this fail? Should return -ENOMEM in that case?
Yes, I should do that.
> > + test->priv = ctx;
> > +
> > + ctx->try_catch = kunit_kmalloc(test,
> > + sizeof(*ctx->try_catch),
> > + GFP_KERNEL);
^ permalink raw reply
* Re: [PATCH v12 11/18] kunit: test: add the concept of assertions
From: Brendan Higgins @ 2019-08-13 5:09 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190813045510.C1D6E206C2@mail.kernel.org>
On Mon, Aug 12, 2019 at 9:55 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:14)
> > Add support for assertions which are like expectations except the test
> > terminates if the assertion is not satisfied.
> >
> > The idea with assertions is that you use them to state all the
> > preconditions for your test. Logically speaking, these are the premises
> > of the test case, so if a premise isn't true, there is no point in
> > continuing the test case because there are no conclusions that can be
> > drawn without the premises. Whereas, the expectation is the thing you
> > are trying to prove. It is not used universally in x-unit style test
> > frameworks, but I really like it as a convention. You could still
> > express the idea of a premise using the above idiom, but I think
> > KUNIT_ASSERT_* states the intended idea perfectly.
> >
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
> > + * Sets an expectation that the values that @left and @right evaluate to are
> > + * not equal. This is semantically equivalent to
> > + * KUNIT_ASSERT_TRUE(@test, strcmp((@left), (@right))). See KUNIT_ASSERT_TRUE()
> > + * for more information.
> > + */
> > +#define KUNIT_ASSERT_STRNEQ(test, left, right) \
> > + KUNIT_BINARY_STR_NE_ASSERTION(test, \
> > + KUNIT_ASSERTION, \
> > + left, \
> > + right)
> > +
> > +#define KUNIT_ASSERT_STRNEQ_MSG(test, left, right, fmt, ...) \
> > + KUNIT_BINARY_STR_NE_MSG_ASSERTION(test, \
> > + KUNIT_ASSERTION, \
> > + left, \
> > + right, \
> > + fmt, \
>
> Same question about tabbing too.
Yep. WIll fix.
> > diff --git a/kunit/test-test.c b/kunit/test-test.c
> > index 88f4cdf03db2a..058f3fb37458a 100644
> > --- a/kunit/test-test.c
> > +++ b/kunit/test-test.c
> > @@ -78,11 +78,13 @@ static int kunit_try_catch_test_init(struct kunit *test)
> > struct kunit_try_catch_test_context *ctx;
> >
> > ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>
> Ah ok. Question still stands if kunit_kzalloc() should just have the
> assertion on failure.
Right. In the previous patch KUNIT_ASSERT_* doesn't exist yet, so I
can't use it. And rather than fall back to return -ENOMEM like I
should have, I evidently forgot to do that.
> > test->priv = ctx;
> >
> > ctx->try_catch = kunit_kmalloc(test,
> > sizeof(*ctx->try_catch),
> > GFP_KERNEL);
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->try_catch);
> >
^ permalink raw reply
* Re: [PATCH v12 16/18] MAINTAINERS: add entry for KUnit the unit testing framework
From: Stephen Boyd @ 2019-08-13 5:26 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190812182421.141150-17-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-08-12 11:24:19)
> Add myself as maintainer of KUnit, the Linux kernel's unit testing
> framework.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply
* Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
From: Stephen Boyd @ 2019-08-13 5:30 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g46PJNTOUAA4GOOrW==74Zy7u1sRESTanL_BXBn6QykscA@mail.gmail.com>
Quoting Brendan Higgins (2019-08-12 22:02:59)
> On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 17:41:05)
> > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > > > > devres_destroy) and use kunit_kfree here?
> > > > >
> > > >
> > > > Yes, or drop the API entirely? Does anything need this functionality?
> > >
> > > Drop the kunit_resource API? I would strongly prefer not to.
> >
> > No. I mean this API, string_stream_clear(). Does anything use it?
>
> Oh, right. No.
>
> However, now that I added the kunit_resource_destroy, I thought it
> might be good to free the string_stream after I use it in each call to
> kunit_assert->format(...) in which case I will be using this logic.
>
> So I think the right thing to do is to expose string_stream_destroy so
> kunit_do_assert can clean up when it's done, and then demote
> string_stream_clear to static. Sound good?
Ok, sure. I don't really see how clearing it explicitly when the
assertion prints vs. never allocating it to begin with is really any
different. Maybe I've missed something though.
^ permalink raw reply
* Re: [PATCH v12 09/18] kunit: test: add support for test abort
From: Stephen Boyd @ 2019-08-13 5:56 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g44XyQi-oprPcdgx-EPboQYaHY6Ocz8Te6NX2SxV=mVhQA@mail.gmail.com>
Quoting Brendan Higgins (2019-08-12 21:57:55)
> On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 11:24:12)
> > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > index 2625bcfeb19ac..93381f841e09f 100644
> > > --- a/include/kunit/test.h
> > > +++ b/include/kunit/test.h
> > > @@ -176,6 +178,11 @@ struct kunit {
> > > */
> > > bool success; /* Read only after test_case finishes! */
> > > spinlock_t lock; /* Gaurds all mutable test state. */
> > > + /*
> > > + * death_test may be both set and unset from multiple threads in a test
> > > + * case.
> > > + */
> > > + bool death_test; /* Protected by lock. */
> > > /*
> > > * Because resources is a list that may be updated multiple times (with
> > > * new resources) from any thread associated with a test case, we must
> > > @@ -184,6 +191,13 @@ struct kunit {
> > > struct list_head resources; /* Protected by lock. */
> > > };
> > >
> > > +static inline void kunit_set_death_test(struct kunit *test, bool death_test)
> > > +{
> > > + spin_lock(&test->lock);
> > > + test->death_test = death_test;
> > > + spin_unlock(&test->lock);
> > > +}
> >
> > These getters and setters are using spinlocks again. It doesn't make any
> > sense. It probably needs a rework like was done for the other bool
> > member, success.
>
> No, this is intentional. death_test can transition from false to true
> and then back to false within the same test. Maybe that deserves a
> comment?
Yes. How does it transition from true to false again?
Either way, having a spinlock around a read/write API doesn't make sense
because it just makes sure that two writes don't overlap, but otherwise
does nothing to keep things synchronized. For example a set to true
after a set to false when the two calls to set true or false aren't
synchronized means they can happen in any order. So I don't see how it
needs a spinlock. The lock needs to be one level higher.
>
> > > +
> > > void kunit_init_test(struct kunit *test, const char *name);
> > >
> > > int kunit_run_tests(struct kunit_suite *suite);
> > > diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> > > new file mode 100644
> > > index 0000000000000..8a414a9af0b64
> > > --- /dev/null
> > > +++ b/include/kunit/try-catch.h
[...]
> > > +
> > > +/*
> > > + * struct kunit_try_catch - provides a generic way to run code which might fail.
> > > + * @context: used to pass user data to the try and catch functions.
> > > + *
> > > + * kunit_try_catch provides a generic, architecture independent way to execute
> > > + * an arbitrary function of type kunit_try_catch_func_t which may bail out by
> > > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, @try
> > > + * is stopped at the site of invocation and @catch is catch is called.
> > > + *
> > > + * struct kunit_try_catch provides a generic interface for the functionality
> > > + * needed to implement kunit->abort() which in turn is needed for implementing
> > > + * assertions. Assertions allow stating a precondition for a test simplifying
> > > + * how test cases are written and presented.
> > > + *
> > > + * Assertions are like expectations, except they abort (call
> > > + * kunit_try_catch_throw()) when the specified condition is not met. This is
> > > + * useful when you look at a test case as a logical statement about some piece
> > > + * of code, where assertions are the premises for the test case, and the
> > > + * conclusion is a set of predicates, rather expectations, that must all be
> > > + * true. If your premises are violated, it does not makes sense to continue.
> > > + */
> > > +struct kunit_try_catch {
> > > + /* private: internal use only. */
> > > + struct kunit *test;
> > > + struct completion *try_completion;
> > > + int try_result;
> > > + kunit_try_catch_func_t try;
> > > + kunit_try_catch_func_t catch;
> >
> > Can these other variables be documented in the kernel doc? And should
> > context be marked as 'public'?
>
> Sure, I can document them.
>
> But I don't think context should be public; it should only be accessed
> by kunit_try_catch_* functions. context should only be populated by
> *_init, and will be passed into *try and *catch when they are called
> internally.
Ok. Then I guess just document them all but keep them all marked as
private.
>
> > > + */
> > > +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
> > > +
> > > +#endif /* _KUNIT_TRY_CATCH_H */
> > > diff --git a/kunit/test.c b/kunit/test.c
> > > index e5080a2c6b29c..995cb53fe4ee9 100644
> > > --- a/kunit/test.c
> > > +++ b/kunit/test.c
> > > @@ -158,6 +171,21 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
> > > kunit_print_string_stream(test, stream);
> > > }
> > >
> > > +void __noreturn kunit_abort(struct kunit *test)
> > > +{
> > > + kunit_set_death_test(test, true);
> > > +
> > > + kunit_try_catch_throw(&test->try_catch);
> > > +
> > > + /*
> > > + * Throw could not abort from test.
> > > + *
> > > + * XXX: we should never reach this line! As kunit_try_catch_throw is
> > > + * marked __noreturn.
> > > + */
> > > + WARN_ONCE(true, "Throw could not abort from test!\n");
> >
> > Should this just be a BUG_ON? It's supposedly impossible.
>
> It should be impossible; it will only reach this line if there is a
> bug in kunit_try_catch_throw. The reason I didn't use BUG_ON was
> because I previously got yelled at for having BUG_ON in this code
> path.
>
> Nevertheless, I think BUG_ON is more correct, so if you will stand by
> it, then that's what I will do.
Yeah BUG_ON is appropriate here and self documenting so please use it.
>
> > > + return;
> > > + }
> > > +
> > > + if (kunit_get_death_test(test)) {
> > > + /*
> > > + * EXPECTED DEATH: kunit_run_case_internal encountered
> > > + * anticipated fatal error. Everything should be in a safe
> > > + * state.
> > > + */
> > > + kunit_run_case_cleanup(test, suite);
> > > + } else {
> > > + /*
> > > + * UNEXPECTED DEATH: kunit_run_case_internal encountered an
> > > + * unanticipated fatal error. We have no idea what the state of
> > > + * the test case is in.
> > > + */
> > > + kunit_handle_test_crash(test, suite, test_case);
> > > + kunit_set_failure(test);
> >
> > Like was done here.
>
> Sorry, like what?
Just saying this has braces for the if-else.
^ permalink raw reply
* Re: [PATCH v12 10/18] kunit: test: add tests for kunit test abort
From: Stephen Boyd @ 2019-08-13 5:57 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g46LHq1sQaio2Vj5jt54YN-Y2HuCT8FbALQhJoekkYJ-uQ@mail.gmail.com>
Quoting Brendan Higgins (2019-08-12 22:06:04)
> On Mon, Aug 12, 2019 at 9:24 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 11:24:13)
> > > +
> > > +static int kunit_try_catch_test_init(struct kunit *test)
> > > +{
> > > + struct kunit_try_catch_test_context *ctx;
> > > +
> > > + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> >
> > Can this fail? Should return -ENOMEM in that case?
>
> Yes, I should do that.
Looks like it's asserted to not be an error. If it's pushed into the API
then there's nothing to do here, and you can have my reviewed-by on this
patch.
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox