Hi,
On Tue, Oct 24, 2023 at 07:14:25PM +0200, Marco Pagani wrote:
I disagree. Actions make it easier to reason about, even when comparing assertion vs expectation
Like, for the call to sg_alloc_table and drm_gem_shmem_prime_import_sg_table(), the reasonable use of assert vs expect would be something like:
sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); KUNIT_ASSERT_NOT_NULL(test, sgt);
ret = sg_alloc_table(sgt, 1, GFP_KERNEL); KUNIT_ASSERT_EQ(test, ret, 0);
/* * Here, it's already not super clear whether you want to expect vs * assert. expect will make you handle the failure case later, assert will * force you to call kfree on sgt. Both kind of suck in their own ways. */
sg_init_one(sgt->sgl, buf, TEST_SIZE);
gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
/* * If the assert fails, we forgot to call sg_free_table(sgt) and kfree(sgt). */
KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE); KUNIT_EXPECT_NULL(test, gem_obj->filp); KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);
/* * And here we have to handle the case where the expectation was wrong, * but the test still continued. */
But if you're not using an action, you still have to call kfree(sgt), which means that you might still
shmem = to_drm_gem_shmem_obj(gem_obj); KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
/* * If the assertion fails, we now have to call drm_gem_shmem_free(shmem) */
/* The scatter/gather table is freed by drm_gem_shmem_free */ drm_gem_shmem_free(shmem);
/* everything's fine now */
The semantics around drm_gem_shmem_free make it a bit convoluted, but doing it using goto/labels, plus handling the assertions and error reporting would be difficult.
Using actions, we have:
sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); KUNIT_ASSERT_NOT_NULL(test, sgt);
ret = kunit_add_action_or_reset(test, kfree_wrapper, sgt); KUNIT_ASSERT_EQ(test, ret, 0);
ret = sg_alloc_table(sgt, 1, GFP_KERNEL); KUNIT_ASSERT_EQ(test, ret, 0);
ret = kunit_add_action_or_reset(test, sg_free_table_wrapper, sgt); KUNIT_ASSERT_EQ(test, ret, 0);
sg_init_one(sgt->sgl, buf, TEST_SIZE);
gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj); KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE); KUNIT_EXPECT_NULL(test, gem_obj->filp); KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);
/* drm_gem_shmem_free will free the struct sg_table itself */ kunit_remove_action(test, sg_free_table_wrapper, sgt); kunit_remove_action(test, kfree_wrapper, sgt);
shmem = to_drm_gem_shmem_obj(gem_obj); KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
ret = kunit_add_action_or_reset(test, drm_gem_shmem_free_wrapper, shmem); KUNIT_ASSERT_EQ(test, ret, 0);
The last one is arguable, but for the previous ones it makes error handling much more convenient and easy to reason about.
The wrappers are also a bit inconvenient to use, but it's mostly there to avoid a compiler warning at the moment.
This patch will help hopefully: https://lore.kernel.org/linux-kselftest/20230915050125.3609689-1-davidgow@go...
Maxime
linaro-mm-sig@lists.linaro.org