| Message ID | 20240821191412.2031-3-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Improve format of some assertion messages | expand |
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); > + else > + string_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); > + else > + string_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 >
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); + else + string_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); + else + string_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);
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> --- lib/kunit/assert.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)