On 19/02/2020 15.45, Petr Mladek wrote:
> On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote:
>> On 19/02/2020 14.48, Petr Mladek wrote:
>>> On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote:
>>>> --- 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.
>>>> 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
