On Thu, 22 Aug 2024 at 03:15, Michal Wajdeczko michal.wajdeczko@intel.com wrote:
Diagnostic message for failed KUNIT_ASSERT|EXPECT_PTR_EQ|NE|NULL shows only raw pointer value, like for this example:
void *ptr1 = ERR_PTR(-ENOMEM); void *ptr2 = NULL; KUNIT_EXPECT_PTR_EQ(test, ptr1, ptr2); KUNIT_EXPECT_NULL(test, ptr1);
we will get:
[ ] Expected ptr1 == ptr2, but [ ] ptr1 == fffffffffffffff4 [ ] ptr2 == 0000000000000000 [ ] Expected ptr1 == ((void *)0), but [ ] ptr1 == ffffffffffffffe4 [ ] ((void *)0) == 0000000000000000
but we can improve this by detecting whether pointer was NULL or error, and use friendly error pointer format if possible:
[ ] Expected ptr1 == ptr2, but [ ] ptr1 is -ENOMEM [ ] ptr2 is NULL [ ] Expected ptr1 == ((void *)0), but [ ] ptr1 is -ENOMEM [ ] ((void *)0) is NULL
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
I have some mixed feelings about this one. Personally, I'd rather this continue to use '==' rather than 'is', just for consistency in case anyone wants to parse these.
Equally, I'd like to have the actual value printed for error pointers. The PTR_NULL assertions are not intended for use with error pointers, and the PTR_{EQ,NE} ones may or may not treat high pointers as actual addresses, or as errors. (We often need the exact value in debugging some of the usercopy tests, which do horrific things like rely on pointer wraparound, so have non-error 0xffffffxx pointers around.)
I'd personally go for, e.g, "ptr1 == fffffffffffffff4 (-ENOMEM)".
Thanks, -- David
lib/kunit/assert.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 6e4333d0c6a0..8da89043b734 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -155,12 +155,28 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, binary_assert->text->left_text, binary_assert->text->operation, binary_assert->text->right_text);
string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",binary_assert->text->left_text,binary_assert->left_value);string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px",binary_assert->text->right_text,binary_assert->right_value);
if (!binary_assert->left_value)string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is NULL\n",binary_assert->text->left_text);else if (IS_ERR(binary_assert->left_value))string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is %pe\n",binary_assert->text->left_text,binary_assert->left_value);elsestring_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",binary_assert->text->left_text,binary_assert->left_value);if (!binary_assert->right_value)string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is NULL\n",binary_assert->text->right_text);else if (IS_ERR(binary_assert->right_value))string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is %pe\n",binary_assert->text->right_text,binary_assert->right_value);elsestring_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",binary_assert->text->right_text,binary_assert->right_value); kunit_assert_print_msg(message, stream);} EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format); -- 2.43.0