On Tue, Oct 22, 2019 at 3:13 PM David Gow <davidgow@xxxxxxxxxx> wrote: > > On Sat, Oct 19, 2019 at 1:27 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > > > On Fri, Oct 18, 2019 at 02:55:49PM -0700, David Gow wrote: > > > + list4 = kzalloc(sizeof(*list4), GFP_KERNEL); > > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list4); > > > > Why not just use GFP_KERNEL | GFP_NOFAIL and remove the check? > > I've sent a new version of the patch out (v5) which uses __GFP_NOFAIL instead. > > The idea had been to exercise KUnit's assertion functionality, in the > hope that it'd allow the test to fail (but potentially allow other > tests to still run) in the case of allocation failure. Given that > we're only allocating enough to store ~4 pointers in total, though, > that's probably of little use. > > > kzalloc() can't return error pointers. If this were an IS_ERR_OR_NULL() > > check then it would generate a static checker warning, but static > > checkers don't know about KUNIT_ASSERT_NOT_ERR_OR_NULL() yet so you're > > safe. > > Alas, KUnit doesn't have a KUNIT_ASSERT_NOT_NULL() macro, and I'd > assumed it was not dangerous (even if not ideal) to check for error > pointers, even if kzalloc() can't return them. Maybe it would be good for us (not in this case, just generally speaking) to add a KUNIT_ASSERT_NOT_NULL() and friends? > Perhaps it'd make sense to add a convenient way of checking the > NULL-ness of pointers to KUnit (it's possible with the > KUNIT_ASSERT_PTR_EQ(), but requires a bit of casting to make the type > checker happy) in the future. Once KUnit is properly upstream, it may > be worth teaching the static analysis tools about these functions to > avoid having warnings in these sorts of tests. > > For now, though, (and for this test in particular), I agree with the > suggestion of just using __GFP_NOFAIL. > > Thanks a lot for the comments, > -- David
