On Thu, 22 Aug 2024 at 03:15, Michal Wajdeczko michal.wajdeczko@intel.com wrote:

Diagnostic message for failed KUNIT_ASSERT|EXPECT_NOT_ERR_OR_NULL shows only raw error code, like for this example:

void *myptr = ERR_PTR(-ENOMEM); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, myptr);

we will get:

[ ] Expected myptr is not error, but is: -12

but we can improve it by using more friendly error pointer format:

[ ] Expected myptr is not error, but is -ENOMEM

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com

Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com

---

I like this, though wouldn't mind also adding the numerical value afterwards. Up to you, though, I can live without.

It does break the 'kunit_test_ptr_not_err_assert_format' test in lib/kunit/assert_test.c: it should be fairly simple to update.

With the test fixed, this is: Reviewed-by: David Gow davidgow@google.com

-- David

-- David

lib/kunit/assert.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 867aa5c4bccf..6e4333d0c6a0 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -82,9 +82,9 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, ptr_assert->text); } else if (IS_ERR(ptr_assert->value)) { string_stream_add(stream,

•                             KUNIT_SUBTEST_INDENT "Expected %s is not error, but is: %ld\n",
  

•                             KUNIT_SUBTEST_INDENT "Expected %s is not error, but is %pe\n",
                              ptr_assert->text,
  

•                             PTR_ERR(ptr_assert->value));
  

•                             ptr_assert->value);
    }
    kunit_assert_print_msg(message, stream);
  

}

2.43.0

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

On Thu, 22 Aug 2024 at 03:15, Michal Wajdeczko michal.wajdeczko@intel.com wrote:

Diagnostic message for failed KUNIT_ASSERT|EXPECT_EQ shows in case of integers only raw values, like for this example:

bool flag; KUNIT_EXPECT_EQ(test, 0, kstrtobool("dunno", &flag));

we will get:

[ ] Expected 0 == kstrtobool("dunno", &flag), but [ ] kstrtobool("dunno", &flag) == -22 (0xffffffffffffffea)

but we can improve it if the value is within MAX_ERRNO range by using more friendly error format:

[ ] Expected 0 == kstrtobool("dunno", &flag), but [ ] kstrtobool("dunno", &flag) == -22 (-EINVAL)

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com

Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com

---

I wasn't sure about this at first, but looking at it, I think I like it, since the numeric value is still given. _Maybe_ it'd be nicer to include both the hex and the error name, but I suspect that's just going to clutter things up more.

So, Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

lib/kunit/assert.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 8da89043b734..9dec0551d0d0 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -128,15 +128,35 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, binary_assert->text->operation, binary_assert->text->right_text); if (!is_literal(binary_assert->text->left_text, binary_assert->left_value))

•           string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)\n",
  

•           if (IS_ERR_VALUE(binary_assert->left_value))
  

•                   string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (%pe)\n",
  

•                                     binary_assert->text->left_text,
  

•                                     binary_assert->left_value,
  

•                                     ERR_PTR(binary_assert->left_value));
  

•           else
  

•                   string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)\n",
  

•                                     binary_assert->text->left_text,
  

•                                     binary_assert->left_value,
  

•                                     binary_assert->left_value);
  

•   else if (IS_ERR_VALUE(binary_assert->left_value))
  

•           string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is %pe\n",
                              binary_assert->text->left_text,
  

•                             binary_assert->left_value,
  

•                             binary_assert->left_value);
  

•                             ERR_PTR(binary_assert->left_value));
    if (!is_literal(binary_assert->text->right_text, binary_assert->right_value))
  

•           string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)",
  

•           if (IS_ERR_VALUE(binary_assert->right_value))
  

•                   string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (%pe)",
  

•                                     binary_assert->text->right_text,
  

•                                     binary_assert->right_value,
  

•                                     ERR_PTR(binary_assert->right_value));
  

•           else
  

•                   string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)",
  

•                                     binary_assert->text->right_text,
  

•                                     binary_assert->right_value,
  

•                                     binary_assert->right_value);
  

•   else if (IS_ERR_VALUE(binary_assert->right_value))
  

•           string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s is %pe",
                              binary_assert->text->right_text,
  

•                             binary_assert->right_value,
  

•                             binary_assert->right_value);
  

•                             ERR_PTR(binary_assert->right_value));
    kunit_assert_print_msg(message, stream);
  

} EXPORT_SYMBOL_GPL(kunit_binary_assert_format); -- 2.43.0