On 19/02/2020 15.45, Petr Mladek wrote:

On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote:

quoted

On 19/02/2020 14.48, Petr Mladek wrote:

quoted

On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote:

quoted

--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

The test should go into null_pointer() instead of errptr().

Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that
way. But I should add a #else section that tests how %pe behaves without
CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch.

OK, we should agree on some structure first.

We already have two top level functions that test how a particular
pointer is printed using different pointer modifiers:

	null_pointer();     -> NULL with %p, %pX, %pE
	invalid_pointer();  -> random pointer with %p, %pX, %pE

Following this logic, errptr() should test how a pointer from IS_ERR() range
is printed using different pointer formats.

Oh please. I wrote test_printf.c originally and structured it with one
helper for each %p<whatever>. How are your additions null_pointer and
invalid_pointer good examples for what the existing style is?

707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 649)
test_pointer(void)
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 650) {
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 651)  plain();
3e5903eb9cff7 (Petr Mladek      2019-04-17 13:53:48 +0200 652)
null_pointer();
3e5903eb9cff7 (Petr Mladek      2019-04-17 13:53:48 +0200 653)
invalid_pointer();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 654)
symbol_ptr();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 655)
kernel_ptr();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 656)
struct_resource();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 657)  addr();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 658)
escaped_str();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 659)
hex_string();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 660)  mac();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 661)  ip();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 662)  uuid();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 663)  dentry();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 664)
struct_va_format();
4d42c44727a06 (Andy Shevchenko  2018-12-04 23:23:11 +0200 665)
struct_rtc_time();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 666)
struct_clk();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 667)  bitmap();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 668)
netdev_features();
edf14cdbf9a0e (Vlastimil Babka  2016-03-15 14:55:56 -0700 669)  flags();
57f5677e535ba (Rasmus Villemoes 2019-10-15 21:07:05 +0200 670)  errptr();
f1ce39df508de (Sakari Ailus     2019-10-03 15:32:19 +0300 671)
fwnode_pointer();

I am open to crate another logic but it must be consistent.

So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM.

If you want to check %pe with NULL in errptr(), you have to
split the other two functions per-modifier. IMHO, it is not
worth it.

Agreed, let's leave null_pointer and invalid_pointer alone.

quoted

quoted

quoted

BTW., your original patch for %p lacks corresponding update of
test_vsprintf.c. Please add appropriate test cases.

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 2d9f520d2f27..1726a678bccd 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c

@@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p)
 static void __init
 null_pointer(void)
 {
-	test_hashed("%p", NULL);
+	test(ZEROS "00000000", "%p", NULL);

No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases
(where one of course has to use explicit integers and not E* constants).

Yes, it would be great to add checks for %p, %px for IS_ERR() range.
But it is different story. The above change is for the original patch
and it was about NULL pointer handling.

Wrong. The original patch (i.e. Ilya's) had subject "vsprintf: don't
obfuscate NULL and error pointers" and did

+	if (IS_ERR_OR_NULL(ptr))

so the tests that should be part of that patch very much need to cover
both NULL and ERR_PTRs passed to plain %p.

Rasmus