Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Andy Shevchenko]

On Wed, Feb 19, 2020 at 10:24 AM Rasmus Villemoes
<linux@xxxxxxxxxxxxxxxxxx> wrote:
>
> Extend %pe to pretty-print NULL in addition to ERR_PTRs,
> i.e. everything IS_ERR_OR_NULL().
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
One nit below, though.

> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> ---
> Something like this? The actual code change is +2,-1 with another +1
> for a test case.
>
>  Documentation/core-api/printk-formats.rst | 9 +++++----
>  lib/errname.c                             | 4 ++++
>  lib/test_printf.c                         | 1 +
>  lib/vsprintf.c                            | 4 ++--
>  4 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 8ebe46b1af39..964b55291445 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -86,10 +86,11 @@ Error Pointers
>
>         %pe     -ENOSPC
>
> -For printing error pointers (i.e. a pointer for which IS_ERR() is true)
> -as a symbolic error name. Error values for which no symbolic name is
> -known are printed in decimal, while a non-ERR_PTR passed as the
> -argument to %pe gets treated as ordinary %p.

> +For printing error pointers (i.e. a pointer for which IS_ERR() is
> +true) as a symbolic error name. Error values for which no symbolic

Why to reformat these lines?

> +name is known are printed in decimal. A NULL pointer is printed as
> +NULL. All other pointer values (i.e. anything !IS_ERR_OR_NULL()) get
> +treated as ordinary %p.
>
>  Symbols/Function Pointers
>  -------------------------
> diff --git a/lib/errname.c b/lib/errname.c
> index 0c4d3e66170e..7757bc00f564 100644
> --- a/lib/errname.c
> +++ b/lib/errname.c
> @@ -11,9 +11,13 @@
>   * allocated errnos (with EHWPOISON = 257 on parisc, and EDQUOT = 1133
>   * on mips), so this wastes a bit of space on those - though we
>   * special case the EDQUOT case.
> + *
> + * For the benefit of %pe being able to print any ERR_OR_NULL pointer
> + * symbolically, 0 is also treated specially.
>   */
>  #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
>  static const char *names_0[] = {
> +       [0] = "NULL",
>         E(E2BIG),
>         E(EACCES),
>         E(EADDRINUSE),
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 2d9f520d2f27..3a37d0e9e735 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -641,6 +641,7 @@ errptr(void)
>         test("[-EIO    ]", "[%-8pe]", ERR_PTR(-EIO));
>         test("[    -EIO]", "[%8pe]", ERR_PTR(-EIO));
>         test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER));
> +       test("[NULL]", "[%pe]", NULL);
>  #endif
>  }
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 7c488a1ce318..b7118d78eb20 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2247,8 +2247,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>         case 'x':
>                 return pointer_string(buf, end, ptr, spec);
>         case 'e':
> -               /* %pe with a non-ERR_PTR gets treated as plain %p */
> -               if (!IS_ERR(ptr))
> +               /* %pe with a non-ERR_OR_NULL ptr gets treated as plain %p */
> +               if (!IS_ERR_OR_NULL(ptr))
>                         break;
>                 return err_ptr(buf, end, ptr, spec);
>         }
> --
> 2.23.0
>


-- 
With Best Regards,
Andy Shevchenko