Messages in this thread | | | Date | Wed, 19 Feb 2020 15:45:58 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH] vsprintf: sanely handle NULL passed to %pe |
| |
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.
I am open to crate another logic but it must be consistent. 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.
Sigh, I should have been more strict[*]. The function should have been called err_ptr() and located right below null_pointer().
[*] I am still trying to find a right balance between preventing nitpicking, bikeshedding, enforcing my style, and creating a mess.
> > Could you send updated patch, please? ;-) > > I'll wait a day or two for more comments. It doesn't seem very urgent.
Sure.
> >> 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.
Best Regards, Petr
| |