| Message ID | 39b4278f-35d2-4071-a3aa-ec49705272af@moroto.mountain |
|---|---|
| State | Accepted |
| Commit | ed1a72fb0d646c983c85b62144fb1d134a8edb71 |
| Headers | show |
| Series | kunit: Fix a NULL vs IS_ERR() bug | expand |
On Thu, 11 Jan 2024 at 02:55, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > The kunit_device_register() function doesn't return NULL, it returns > error pointers. Change the KUNIT_ASSERT_NOT_NULL() to check for > ERR_OR_NULL(). > > Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- Nice catch, thanks! Reviewed-by: David Gow <davidgow@google.com> > It's a pity that there isn't a KUNIT_ASSERT_NOT_ERR_PTR() macro... I think we'll add one, but I'm not yet totally convinced that it would be better than using ASSERT_NOT_ERR_OR_NULL() in cases like this, where we're: 1. In a test; and, 2. using the pointer afterwards, expecting it to be valid (dereferencing it and/or passing it to functions which will) This is largely because it'd be nicer, if the pointer is NULL (due to a bug), to get a more explicit assertion failure, rather than a crash. It does make the test code less indicative of how the APIs are meant to be used elsewhere, though, and annoys the static analysis, though. Thoughts? -- David > lib/kunit/kunit-test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c > index c4259d910356..f7980ef236a3 100644 > --- a/lib/kunit/kunit-test.c > +++ b/lib/kunit/kunit-test.c > @@ -720,7 +720,7 @@ static void kunit_device_cleanup_test(struct kunit *test) > long action_was_run = 0; > > test_device = kunit_device_register(test, "my_device"); > - KUNIT_ASSERT_NOT_NULL(test, test_device); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_device); > > /* Add an action to verify cleanup. */ > devm_add_action(test_device, test_dev_action, &action_was_run); > -- > 2.43.0 >
On Fri, Jan 12, 2024 at 07:39:14AM +0800, David Gow wrote: > On Thu, 11 Jan 2024 at 02:55, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > The kunit_device_register() function doesn't return NULL, it returns > > error pointers. Change the KUNIT_ASSERT_NOT_NULL() to check for > > ERR_OR_NULL(). > > > > Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > Nice catch, thanks! > > Reviewed-by: David Gow <davidgow@google.com> > > > It's a pity that there isn't a KUNIT_ASSERT_NOT_ERR_PTR() macro... > > I think we'll add one, but I'm not yet totally convinced that it would > be better than using ASSERT_NOT_ERR_OR_NULL() in cases like this, > where we're: > 1. In a test; and, > 2. using the pointer afterwards, expecting it to be valid > (dereferencing it and/or passing it to functions which will) > > This is largely because it'd be nicer, if the pointer is NULL (due to > a bug), to get a more explicit assertion failure, rather than a crash. > It does make the test code less indicative of how the APIs are meant > to be used elsewhere, though, and annoys the static analysis, though. > > Thoughts? It doesn't annoy any static checkers because nothing looks for it. Expecting that this test code might be buggier than normal code probably isn't unreasonable so I guess that makes sense. regards, dan carpenter
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index c4259d910356..f7980ef236a3 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -720,7 +720,7 @@ static void kunit_device_cleanup_test(struct kunit *test) long action_was_run = 0; test_device = kunit_device_register(test, "my_device"); - KUNIT_ASSERT_NOT_NULL(test, test_device); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_device); /* Add an action to verify cleanup. */ devm_add_action(test_device, test_dev_action, &action_was_run);
The kunit_device_register() function doesn't return NULL, it returns error pointers. Change the KUNIT_ASSERT_NOT_NULL() to check for ERR_OR_NULL(). Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- It's a pity that there isn't a KUNIT_ASSERT_NOT_ERR_PTR() macro... lib/kunit/kunit-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)