[PATCH] printf: add support for printing symbolic error codes

[PATCH] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

It has been suggested several times to extend vsnprintf() to be able
to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
another attempt. Rather than adding another %p extension, simply teach
plain %p to convert ERR_PTRs. While the primary use case is

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %p\n", foo);
    return PTR_ERR(foo);
  }

it is also more helpful to get a symbolic error code (or, worst case,
a decimal number) in case an ERR_PTR is accidentally passed to some
%p<something>, rather than the (efault) that check_pointer() would
result in.

With my embedded hat on, I've made it possible to remove this.

I've tested that the #ifdeffery in errcode.c is sufficient to make
this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
0day bot will tell me which ones I've missed.

The symbols to include have been found by massaging the output of

  find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'

In the cases where some common aliasing exists
(e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
to the bottom so that one takes precedence.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/errcode.h |  14 +++
 lib/Kconfig.debug       |   8 ++
 lib/Makefile            |   1 +
 lib/errcode.c           | 215 ++++++++++++++++++++++++++++++++++++++++
 lib/test_printf.c       |  14 +++
 lib/vsprintf.c          |  28 +++++-
 6 files changed, 278 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/errcode.h
 create mode 100644 lib/errcode.c

diff --git a/include/linux/errcode.h b/include/linux/errcode.h
new file mode 100644
index 000000000000..9dc4cfaa37ab
--- /dev/null
+++ b/include/linux/errcode.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ERRCODE_H
+#define _LINUX_ERRCODE_H
+
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+const char *errcode(int code);
+#else
+static inline const char *errcode(int code)
+{
+	return NULL;
+}
+#endif
+
+#endif /* _LINUX_ERRCODE_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5960e2980a8a..dc1b20872774 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -164,6 +164,14 @@ config DYNAMIC_DEBUG
 	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
 	  information.
 
+config SYMBOLIC_ERRCODE
+	bool "Support symbolic error codes in printf"
+	help
+	  If you say Y here, the kernel's printf implementation will
+	  be able to print symbolic errors such as ENOSPC instead of
+	  the number 28. It makes the kernel image slightly larger
+	  (about 3KB), but can make the kernel logs easier to read.
+
 endmenu # "printk and dmesg options"
 
 menu "Compile-time checks and compiler options"
diff --git a/lib/Makefile b/lib/Makefile
index 29c02a924973..664ecf10ee2a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -185,6 +185,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
 obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
 
 obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
+obj-$(CONFIG_SYMBOLIC_ERRCODE) += errcode.o
 
 obj-$(CONFIG_NLATTR) += nlattr.o
 
diff --git a/lib/errcode.c b/lib/errcode.c
new file mode 100644
index 000000000000..7dcf97d5307f
--- /dev/null
+++ b/lib/errcode.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/build_bug.h>
+#include <linux/errno.h>
+#include <linux/errcode.h>
+#include <linux/kernel.h>
+
+/*
+ * Ensure these tables to not accidentally become gigantic if some
+ * huge errno makes it in. On most architectures, the first table will
+ * only have about 140 entries, but mips and parisc have more sparsely
+ * 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.
+ */
+#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
+static const char *codes_0[] = {
+	E(E2BIG),
+	E(EACCES),
+	E(EADDRINUSE),
+	E(EADDRNOTAVAIL),
+	E(EADV),
+	E(EAFNOSUPPORT),
+	E(EALREADY),
+	E(EBADE),
+	E(EBADF),
+	E(EBADFD),
+	E(EBADMSG),
+	E(EBADR),
+	E(EBADRQC),
+	E(EBADSLT),
+	E(EBFONT),
+	E(EBUSY),
+#ifdef ECANCELLED
+	E(ECANCELLED),
+#endif
+	E(ECHILD),
+	E(ECHRNG),
+	E(ECOMM),
+	E(ECONNABORTED),
+	E(ECONNRESET),
+	E(EDEADLOCK),
+	E(EDESTADDRREQ),
+	E(EDOM),
+	E(EDOTDOT),
+#ifndef CONFIG_MIPS
+	E(EDQUOT),
+#endif
+	E(EEXIST),
+	E(EFAULT),
+	E(EFBIG),
+	E(EHOSTDOWN),
+	E(EHOSTUNREACH),
+	E(EHWPOISON),
+	E(EIDRM),
+	E(EILSEQ),
+#ifdef EINIT
+	E(EINIT),
+#endif
+	E(EINPROGRESS),
+	E(EINTR),
+	E(EINVAL),
+	E(EIO),
+	E(EISCONN),
+	E(EISDIR),
+	E(EISNAM),
+	E(EKEYEXPIRED),
+	E(EKEYREJECTED),
+	E(EKEYREVOKED),
+	E(EL2HLT),
+	E(EL2NSYNC),
+	E(EL3HLT),
+	E(EL3RST),
+	E(ELIBACC),
+	E(ELIBBAD),
+	E(ELIBEXEC),
+	E(ELIBMAX),
+	E(ELIBSCN),
+	E(ELNRNG),
+	E(ELOOP),
+	E(EMEDIUMTYPE),
+	E(EMFILE),
+	E(EMLINK),
+	E(EMSGSIZE),
+	E(EMULTIHOP),
+	E(ENAMETOOLONG),
+	E(ENAVAIL),
+	E(ENETDOWN),
+	E(ENETRESET),
+	E(ENETUNREACH),
+	E(ENFILE),
+	E(ENOANO),
+	E(ENOBUFS),
+	E(ENOCSI),
+	E(ENODATA),
+	E(ENODEV),
+	E(ENOENT),
+	E(ENOEXEC),
+	E(ENOKEY),
+	E(ENOLCK),
+	E(ENOLINK),
+	E(ENOMEDIUM),
+	E(ENOMEM),
+	E(ENOMSG),
+	E(ENONET),
+	E(ENOPKG),
+	E(ENOPROTOOPT),
+	E(ENOSPC),
+	E(ENOSR),
+	E(ENOSTR),
+#ifdef ENOSYM
+	E(ENOSYM),
+#endif
+	E(ENOSYS),
+	E(ENOTBLK),
+	E(ENOTCONN),
+	E(ENOTDIR),
+	E(ENOTEMPTY),
+	E(ENOTNAM),
+	E(ENOTRECOVERABLE),
+	E(ENOTSOCK),
+	E(ENOTTY),
+	E(ENOTUNIQ),
+	E(ENXIO),
+	E(EOPNOTSUPP),
+	E(EOVERFLOW),
+	E(EOWNERDEAD),
+	E(EPERM),
+	E(EPFNOSUPPORT),
+	E(EPIPE),
+#ifdef EPROCLIM
+	E(EPROCLIM),
+#endif
+	E(EPROTO),
+	E(EPROTONOSUPPORT),
+	E(EPROTOTYPE),
+	E(ERANGE),
+	E(EREMCHG),
+#ifdef EREMDEV
+	E(EREMDEV),
+#endif
+	E(EREMOTE),
+	E(EREMOTEIO),
+#ifdef EREMOTERELEASE
+	E(EREMOTERELEASE),
+#endif
+	E(ERESTART),
+	E(ERFKILL),
+	E(EROFS),
+#ifdef ERREMOTE
+	E(ERREMOTE),
+#endif
+	E(ESHUTDOWN),
+	E(ESOCKTNOSUPPORT),
+	E(ESPIPE),
+	E(ESRCH),
+	E(ESRMNT),
+	E(ESTALE),
+	E(ESTRPIPE),
+	E(ETIME),
+	E(ETIMEDOUT),
+	E(ETOOMANYREFS),
+	E(ETXTBSY),
+	E(EUCLEAN),
+	E(EUNATCH),
+	E(EUSERS),
+	E(EXDEV),
+	E(EXFULL),
+
+	E(ECANCELED),
+	E(EAGAIN), /* EWOULDBLOCK */
+	E(ECONNREFUSED), /* EREFUSED */
+	E(EDEADLK), /* EDEADLK */
+};
+#undef E
+
+#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
+static const char *codes_512[] = {
+	E(ERESTARTSYS),
+	E(ERESTARTNOINTR),
+	E(ERESTARTNOHAND),
+	E(ENOIOCTLCMD),
+	E(ERESTART_RESTARTBLOCK),
+	E(EPROBE_DEFER),
+	E(EOPENSTALE),
+	E(ENOPARAM),
+
+	E(EBADHANDLE),
+	E(ENOTSYNC),
+	E(EBADCOOKIE),
+	E(ENOTSUPP),
+	E(ETOOSMALL),
+	E(ESERVERFAULT),
+	E(EBADTYPE),
+	E(EJUKEBOX),
+	E(EIOCBQUEUED),
+	E(ERECALLCONFLICT),
+};
+#undef E
+
+const char *errcode(int err)
+{
+	/* Might as well accept both -EIO and EIO. */
+	if (err < 0)
+		err = -err;
+	if (err <= 0) /* INT_MIN or 0 */
+		return NULL;
+	if (err < ARRAY_SIZE(codes_0))
+		return codes_0[err];
+	if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
+		return codes_512[err - 512];
+	/* But why? */
+	if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
+		return "EDQUOT";
+	return NULL;
+}
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 944eb50f3862..0401a2341245 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -212,6 +212,7 @@ test_string(void)
 #define PTR_STR "ffff0123456789ab"
 #define PTR_VAL_NO_CRNG "(____ptrval____)"
 #define ZEROS "00000000"	/* hex 32 zero bits */
+#define FFFFS "ffffffff"
 
 static int __init
 plain_format(void)
@@ -243,6 +244,7 @@ plain_format(void)
 #define PTR_STR "456789ab"
 #define PTR_VAL_NO_CRNG "(ptrval)"
 #define ZEROS ""
+#define FFFFS ""
 
 static int __init
 plain_format(void)
@@ -588,6 +590,17 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+static void __init
+errptr(void)
+{
+	test("-1234", "%p", ERR_PTR(-1234));
+	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
+	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
+#endif
+}
+
 static void __init
 test_pointer(void)
 {
@@ -610,6 +623,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	errptr();
 }
 
 static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..4b67fc23f3f2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -21,6 +21,7 @@
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/errcode.h>
 #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
 #include <linux/types.h>
 #include <linux/string.h>
@@ -2111,6 +2112,31 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
+	/* %px means the user explicitly wanted the pointer formatted as a hex value. */
+	if (*fmt == 'x')
+		return pointer_string(buf, end, ptr, spec);
+
+	/* If it's an ERR_PTR, try to print its symbolic representation. */
+	if (IS_ERR(ptr)) {
+		long err = PTR_ERR(ptr);
+		const char *sym = errcode(-err);
+		if (sym)
+			return string_nocheck(buf, end, sym, spec);
+		/*
+		 * Funky, somebody passed ERR_PTR(-1234) or some other
+		 * non-existing Efoo - or more likely
+		 * CONFIG_SYMBOLIC_ERRCODE=n. None of the
+		 * %p<something> extensions can make any sense of an
+		 * ERR_PTR(), and if this was just a plain %p, the
+		 * user is still better off getting the decimal
+		 * representation rather than the hash value that
+		 * ptr_to_id() would generate.
+		 */
+		spec.flags |= SIGN;
+		spec.base = 10;
+		return number(buf, end, err, spec);
+	}
+
 	switch (*fmt) {
 	case 'F':
 	case 'f':
@@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return flags_string(buf, end, ptr, spec, fmt);
 	case 'O':
 		return kobject_string(buf, end, ptr, spec, fmt);
-	case 'x':
-		return pointer_string(buf, end, ptr, spec);
 	}
 
 	/* default is to _not_ leak addresses, hash before printing */
-- 
2.20.1

Re: [PATCH] printf: add support for printing symbolic error codes

[Joe Perches]

On Fri, 2019-08-30 at 23:46 +0200, Rasmus Villemoes wrote:
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is

Thanks, this all this seems reasonable except for

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		return flags_string(buf, end, ptr, spec, fmt);
>  	case 'O':
>  		return kobject_string(buf, end, ptr, spec, fmt);
> -	case 'x':
> -		return pointer_string(buf, end, ptr, spec);
>  	}
>  
>  	/* default is to _not_ leak addresses, hash before printing */

why remove this?

Re: [PATCH] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 30/08/2019 23.53, Joe Perches wrote:
> 
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
>> @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>  		return flags_string(buf, end, ptr, spec, fmt);
>>  	case 'O':
>>  		return kobject_string(buf, end, ptr, spec, fmt);
>> -	case 'x':
>> -		return pointer_string(buf, end, ptr, spec);
>>  	}
>>  
>>  	/* default is to _not_ leak addresses, hash before printing */
> 
> why remove this?
> 

The handling of %px is moved above the test for ptr being an ERR_PTR, so
that %px, ptr continues to be (roughly) equivalent to %08lx, (long)ptr.

Rasmus

Re: [PATCH] printf: add support for printing symbolic error codes

[Joe Perches]

On Sat, 2019-08-31 at 00:03 +0200, Rasmus Villemoes wrote:
> On 30/08/2019 23.53, Joe Perches wrote:
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > []
> > > @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> > >  		return flags_string(buf, end, ptr, spec, fmt);
> > >  	case 'O':
> > >  		return kobject_string(buf, end, ptr, spec, fmt);
> > > -	case 'x':
> > > -		return pointer_string(buf, end, ptr, spec);
> > >  	}
> > >  
> > >  	/* default is to _not_ leak addresses, hash before printing */
> > 
> > why remove this?
> > 
> 
> The handling of %px is moved above the test for ptr being an ERR_PTR, so
> that %px, ptr continues to be (roughly) equivalent to %08lx, (long)ptr.

Ah.
Pity the flow of the switch/case is disrupted.
That now deserves a separate comment.

But why not just extend check_pointer_msg?

Re: [PATCH] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 31/08/2019 00.21, Joe Perches wrote:
> On Sat, 2019-08-31 at 00:03 +0200, Rasmus Villemoes wrote:
>> On 30/08/2019 23.53, Joe Perches wrote:
>>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> []
>>>> @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>>>  		return flags_string(buf, end, ptr, spec, fmt);
>>>>  	case 'O':
>>>>  		return kobject_string(buf, end, ptr, spec, fmt);
>>>> -	case 'x':
>>>> -		return pointer_string(buf, end, ptr, spec);
>>>>  	}
>>>>  
>>>>  	/* default is to _not_ leak addresses, hash before printing */
>>>
>>> why remove this?
>>>
>>
>> The handling of %px is moved above the test for ptr being an ERR_PTR, so
>> that %px, ptr continues to be (roughly) equivalent to %08lx, (long)ptr.
> 
> Ah.
> Pity the flow of the switch/case is disrupted.

Agree, but I don't think it's that bad.

> That now deserves a separate comment.

You mean a comment like /* %px means the user explicitly wanted the
pointer formatted as a hex value. */. Or do you want (the|an additional)
comment somewhere inside the switch()?

> But why not just extend check_pointer_msg?

Partly because that would rely on all %p<foo> actually eventually
passing ptr through to that (notably plain %p does not), partly because
the way check_pointer_msg works means that it has to return a string for
its caller to print - which is ok when the errcode is found, but breaks
if it needs to format a decimal. It can't even snprintf() to a stack
buffer and return that, because, well, you can't do that, and it would
be a silly recursive snprintf anyway.

OK, so perhaps you meant check_pointer() where it might be doable, but
again, with plain %p and possibly others we don't get to that.

Rasmus

RE: [PATCH] printf: add support for printing symbolic error codes

[David Laight]

From: Rasmus Villemoes
> Sent: 30 August 2019 23:51
...
> > But why not just extend check_pointer_msg?
> 
> Partly because that would rely on all %p<foo> actually eventually
> passing ptr through to that (notably plain %p does not), partly because
> the way check_pointer_msg works means that it has to return a string for
> its caller to print - which is ok when the errcode is found, but breaks
> if it needs to format a decimal. It can't even snprintf() to a stack
> buffer and return that, because, well, you can't do that, and it would
> be a silly recursive snprintf anyway.

Perhaps you could use NULL or "" to mean 'just print the value'.
Then you might manage to use the test for NULL to print the errno strings.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] printf: add support for printing symbolic error codes

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

Hi Rasmus,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rasmus-Villemoes/printf-add-support-for-printing-symbolic-error-codes/20190831-162013
config: x86_64-randconfig-b002-201934 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-11) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:0:0:
   include/linux/errcode.h: In function 'errcode':
>> include/linux/errcode.h:10:9: error: 'NULL' undeclared (first use in this function)
     return NULL;
            ^~~~
   include/linux/errcode.h:10:9: note: each undeclared identifier is reported only once for each function it appears in

vim +/NULL +10 include/linux/errcode.h

     4	
     5	#ifdef CONFIG_SYMBOLIC_ERRCODE
     6	const char *errcode(int code);
     7	#else
     8	static inline const char *errcode(int code)
     9	{
  > 10		return NULL;
    11	}
    12	#endif
    13	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37666 bytes --]

Re: [PATCH] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1757 bytes --]

Hello Rasmus,

On 8/30/19 11:46 PM, Rasmus Villemoes wrote:
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is
> 
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %p\n", foo);
>     return PTR_ERR(foo);
>   }
>
> it is also more helpful to get a symbolic error code (or, worst case,
> a decimal number) in case an ERR_PTR is accidentally passed to some
> %p<something>, rather than the (efault) that check_pointer() would
> result in.

Your text suggests that only cases that formerly emitted "(efault)" are
affected. I didn't check this but if this is the case, I like your patch.

> With my embedded hat on, I've made it possible to remove this.

I wonder if the config item should only be configurable if EXPERT is
enabled.

> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.
> 
> The symbols to include have been found by massaging the output of
> 
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
> 
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Apart from the above minor issues:

Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 02/09/2019 17.29, Uwe Kleine-König wrote:
> Hello Rasmus,
> 
> On 8/30/19 11:46 PM, Rasmus Villemoes wrote:
>> It has been suggested several times to extend vsnprintf() to be able
>> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
>> another attempt. Rather than adding another %p extension, simply teach
>> plain %p to convert ERR_PTRs. While the primary use case is
>>
>>   if (IS_ERR(foo)) {
>>     pr_err("Sorry, can't do that: %p\n", foo);
>>     return PTR_ERR(foo);
>>   }
>>
>> it is also more helpful to get a symbolic error code (or, worst case,
>> a decimal number) in case an ERR_PTR is accidentally passed to some
>> %p<something>, rather than the (efault) that check_pointer() would
>> result in.
> 
> Your text suggests that only cases that formerly emitted "(efault)" are
> affected. I didn't check this but if this is the case, I like your patch.

Well, sort of. Depends on whether this is plain %p or %p<something>.

In the former case, the pointer would have been treated as any other
"valid" kernel pointer, hence passed through the ptr_to_id() and printed
as the result of, roughly, siphash((long)ptr) - i.e. some hex number
that has nothing directly to do with the -EIO that was passed in.
Moreover, while the printed value would be the same for a given error
code during a given boot, on the next boot, the hashing would use a
different seed, so would result in another "random" hex value being
printed - which one can easily imagine makes debugging harder. With my
patch, these would always result in "EIO" (or its decimal
representation) being printed.

In the latter case, yes, all the %p extensions that would somehow
dereference ptr would then be caught in the check_pointer() and instead
of printing (efault), we'd (again) get the symbolic error code.

In both cases, I see printing the symbolic errno as a clear improvement
- completely independent of whether we somehow want to make it
"officially allowed" to deliberately pass ERR_PTRs (and I see that I
forgot to update Documentation). So while that is really the main point
of the patch, IMO the patch can already be justified by the above.

[A few of the %p extensions do not dereference ptr (e.g. the %pS aka
print the symbol name) - I think they just print ptr as a hex value if
no symbol is found (or !CONFIG_KALLSYMS). I can't imagine how an ERR_PTR
would be passed to %pS, though, and again, getting the symbolic error
(or the decimal -22) should be better than getting -22 printed as a hex
string.]

>> With my embedded hat on, I've made it possible to remove this.
> 
> I wonder if the config item should only be configurable if EXPERT is
> enabled.

Maybe. Or one could make it default y and then only make it possible to
deselect if CONFIG_EXPERT - only really space-constrained embedded
devices would want this disabled, I think. But I prefer keeping it
simple, i.e. keeping it as-is for now.

> Apart from the above minor issues:
> 
> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>

Thanks. The buildbot apparently tried to compile the errcode.h header by
itself and complained that NULL was not defined, so I'll respin to make
it happy, and add a note to Documentation/. Can I include that ack even
if I don't change the Kconfig logic?

Thanks,
Rasmus

Re: [PATCH] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 474 bytes --]

On 9/4/19 11:13 AM, Rasmus Villemoes wrote:
> On 02/09/2019 17.29, Uwe Kleine-König wrote:
>> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> 
> Thanks. The buildbot apparently tried to compile the errcode.h header by
> itself and complained that NULL was not defined, so I'll respin to make
> it happy, and add a note to Documentation/. Can I include that ack even
> if I don't change the Kconfig logic?

Yeah, feel free to keep it.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] printf: add support for printing symbolic error codes

[Andy Shevchenko]

On Sat, Aug 31, 2019 at 12:48 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is
>
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %p\n", foo);
>     return PTR_ERR(foo);
>   }
>
> it is also more helpful to get a symbolic error code (or, worst case,
> a decimal number) in case an ERR_PTR is accidentally passed to some
> %p<something>, rather than the (efault) that check_pointer() would
> result in.
>
> With my embedded hat on, I've made it possible to remove this.
>
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.
>
> The symbols to include have been found by massaging the output of
>
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.

> +/*
> + * Ensure these tables to not accidentally become gigantic if some
> + * huge errno makes it in. On most architectures, the first table will
> + * only have about 140 entries, but mips and parisc have more sparsely
> + * 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.
> + */
> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err

Hmm... Perhaps better to define the upper boundary with something like

#define __E_POSIX_UPPER_BOUNDARY 300 // name sucks, I know

> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err

Similar to 550?

> +const char *errcode(int err)
> +{
> +       /* Might as well accept both -EIO and EIO. */
> +       if (err < 0)
> +               err = -err;
> +       if (err <= 0) /* INT_MIN or 0 */
> +               return NULL;
> +       if (err < ARRAY_SIZE(codes_0))
> +               return codes_0[err];
> +       if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
> +               return codes_512[err - 512];
> +       /* But why? */
> +       if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
> +               return "EDQUOT";

Another possibility is to initialize the errors at run time with radix tree.

> +       return NULL;
> +}

> @@ -2111,6 +2112,31 @@ static noinline_for_stack
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>               struct printf_spec spec)
>  {
> +       /* %px means the user explicitly wanted the pointer formatted as a hex value. */
> +       if (*fmt == 'x')
> +               return pointer_string(buf, end, ptr, spec);

But instead of breaking switch case apart can we use...

> +
> +       /* If it's an ERR_PTR, try to print its symbolic representation. */
> +       if (IS_ERR(ptr)) {

...  if (IS_ERR() && *fmt != 'x') {
here?

> +               long err = PTR_ERR(ptr);
> +               const char *sym = errcode(-err);
> +               if (sym)
> +                       return string_nocheck(buf, end, sym, spec);
> +               /*
> +                * Funky, somebody passed ERR_PTR(-1234) or some other
> +                * non-existing Efoo - or more likely
> +                * CONFIG_SYMBOLIC_ERRCODE=n. None of the
> +                * %p<something> extensions can make any sense of an
> +                * ERR_PTR(), and if this was just a plain %p, the
> +                * user is still better off getting the decimal
> +                * representation rather than the hash value that
> +                * ptr_to_id() would generate.
> +                */
> +               spec.flags |= SIGN;
> +               spec.base = 10;
> +               return number(buf, end, err, spec);
> +       }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 4116 bytes --]

On 9/4/19 6:19 PM, Andy Shevchenko wrote:
> On Sat, Aug 31, 2019 at 12:48 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> It has been suggested several times to extend vsnprintf() to be able
>> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
>> another attempt. Rather than adding another %p extension, simply teach
>> plain %p to convert ERR_PTRs. While the primary use case is
>>
>>   if (IS_ERR(foo)) {
>>     pr_err("Sorry, can't do that: %p\n", foo);
>>     return PTR_ERR(foo);
>>   }
>>
>> it is also more helpful to get a symbolic error code (or, worst case,
>> a decimal number) in case an ERR_PTR is accidentally passed to some
>> %p<something>, rather than the (efault) that check_pointer() would
>> result in.
>>
>> With my embedded hat on, I've made it possible to remove this.
>>
>> I've tested that the #ifdeffery in errcode.c is sufficient to make
>> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
>> 0day bot will tell me which ones I've missed.
>>
>> The symbols to include have been found by massaging the output of
>>
>>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>>
>> In the cases where some common aliasing exists
>> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
>> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
>> to the bottom so that one takes precedence.
> 
>> +/*
>> + * Ensure these tables to not accidentally become gigantic if some
>> + * huge errno makes it in. On most architectures, the first table will
>> + * only have about 140 entries, but mips and parisc have more sparsely
>> + * 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.
>> + */
>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> 
> Hmm... Perhaps better to define the upper boundary with something like
> 
> #define __E_POSIX_UPPER_BOUNDARY 300 // name sucks, I know
> 
>> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
> 
> Similar to 550?

I'd not add "POSIX" in the name. Given that the arrays are called
codes_0 and codes_512 I don't think using plain numbers hurts much and
choosing a good name is hard, so I suggest to keep the explicit numbers.

>> +const char *errcode(int err)
>> +{
>> +       /* Might as well accept both -EIO and EIO. */
>> +       if (err < 0)
>> +               err = -err;
>> +       if (err <= 0) /* INT_MIN or 0 */
>> +               return NULL;
>> +       if (err < ARRAY_SIZE(codes_0))
>> +               return codes_0[err];
>> +       if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
>> +               return codes_512[err - 512];
>> +       /* But why? */
>> +       if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
>> +               return "EDQUOT";
> 
> Another possibility is to initialize the errors at run time with radix tree.

The idea was to save space. But when using a radix tree this has
overhead compared to the lists here, and you still need a map for
error-code -> error-name to initialize the radix tree. Also a lookup is
slower than with the idea implemented here. So it's bigger, slower and
more complicated ... I don't think we should do that.

> 
>> +       return NULL;
>> +}
> 
>> @@ -2111,6 +2112,31 @@ static noinline_for_stack
>>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>               struct printf_spec spec)
>>  {
>> +       /* %px means the user explicitly wanted the pointer formatted as a hex value. */
>> +       if (*fmt == 'x')
>> +               return pointer_string(buf, end, ptr, spec);
> 
> But instead of breaking switch case apart can we use...
> 
>> +
>> +       /* If it's an ERR_PTR, try to print its symbolic representation. */
>> +       if (IS_ERR(ptr)) {
> 
> ...  if (IS_ERR() && *fmt != 'x') {
> here?

I don't feel strong here, works either way for me.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 04/09/2019 18.28, Uwe Kleine-König wrote:
> On 9/4/19 6:19 PM, Andy Shevchenko wrote:
>> On Sat, Aug 31, 2019 at 12:48 AM Rasmus Villemoes
>> <linux@rasmusvillemoes.dk> wrote:
>>>
>>
>>> +/*
>>> + * Ensure these tables to not accidentally become gigantic if some
>>> + * huge errno makes it in. On most architectures, the first table will
>>> + * only have about 140 entries, but mips and parisc have more sparsely
>>> + * 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.
>>> + */
>>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
>>
>> Hmm... Perhaps better to define the upper boundary with something like
>>
>> #define __E_POSIX_UPPER_BOUNDARY 300 // name sucks, I know
>>
>>> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
>>
>> Similar to 550?
> 
> I'd not add "POSIX" in the name. Given that the arrays are called
> codes_0 and codes_512 I don't think using plain numbers hurts much and
> choosing a good name is hard, so I suggest to keep the explicit numbers.

I agree, adding random macro names for these essentially arbitrary (and
one-time use) numbers doesn't make sense. Remember that the sizing of
the arrays is done automatically by gcc. I suppose an alternative is to
drop the BUILD_BUG_ON_ZEROs from the E() defines and then just have some
static_assert(ARRAY_SIZE(codes_0) < 300) - but the advantage of the
above is that one gets to know _which_ E* has a huge value (that is how
I caught EDQUOT on MIPS).

>>> +const char *errcode(int err)
>>> +{
>>> +       /* Might as well accept both -EIO and EIO. */
>>> +       if (err < 0)
>>> +               err = -err;
>>> +       if (err <= 0) /* INT_MIN or 0 */
>>> +               return NULL;
>>> +       if (err < ARRAY_SIZE(codes_0))
>>> +               return codes_0[err];
>>> +       if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
>>> +               return codes_512[err - 512];
>>> +       /* But why? */
>>> +       if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
>>> +               return "EDQUOT";
>>
>> Another possibility is to initialize the errors at run time with radix tree.
> 
> The idea was to save space. But when using a radix tree this has
> overhead compared to the lists here, and you still need a map for
> error-code -> error-name to initialize the radix tree. Also a lookup is
> slower than with the idea implemented here. So it's bigger, slower and
> more complicated ... I don't think we should do that.

Yes, a radix tree is unlikely to end up saving space at all.
Moreover, any initialization at run-time means there's some window where
we don't have them, and printk() should work as early as possible (and I
really don't want to add any kind of synchronization "are we initialized
yet", just see what that did to the pointer hashing). So I'll stick with
the arrays.

>>> @@ -2111,6 +2112,31 @@ static noinline_for_stack
>>>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>>               struct printf_spec spec)
>>>  {
>>> +       /* %px means the user explicitly wanted the pointer formatted as a hex value. */
>>> +       if (*fmt == 'x')
>>> +               return pointer_string(buf, end, ptr, spec);
>>
>> But instead of breaking switch case apart can we use...
>>
>>> +
>>> +       /* If it's an ERR_PTR, try to print its symbolic representation. */
>>> +       if (IS_ERR(ptr)) {
>>
>> ...  if (IS_ERR() && *fmt != 'x') {
>> here?

This makes sense, I think I'll do it that way. Thanks.

Rasmus

[PATCH v2] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

It has been suggested several times to extend vsnprintf() to be able
to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
another attempt. Rather than adding another %p extension, simply teach
plain %p to convert ERR_PTRs. While the primary use case is

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %p\n", foo);
    return PTR_ERR(foo);
  }

it is also more helpful to get a symbolic error code (or, worst case,
a decimal number) in case an ERR_PTR is accidentally passed to some
%p<something>, rather than the (efault) that check_pointer() would
result in.

With my embedded hat on, I've made it possible to remove this.

I've tested that the #ifdeffery in errcode.c is sufficient to make
this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
0day bot will tell me which ones I've missed.

The symbols to include have been found by massaging the output of

  find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'

In the cases where some common aliasing exists
(e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
to the bottom so that one takes precedence.

Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v2:
- add #include <linux/stddef.h> to errcode.h (0day)
- keep 'x' handling in switch() (Andy)
- add paragraph to Documentation/core-api/printk-formats.rst
- add ack from Uwe

 Documentation/core-api/printk-formats.rst |   8 +
 include/linux/errcode.h                   |  16 ++
 lib/Kconfig.debug                         |   8 +
 lib/Makefile                              |   1 +
 lib/errcode.c                             | 215 ++++++++++++++++++++++
 lib/test_printf.c                         |  14 ++
 lib/vsprintf.c                            |  26 +++
 7 files changed, 288 insertions(+)
 create mode 100644 include/linux/errcode.h
 create mode 100644 lib/errcode.c

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcb..7d3bf3cb207b 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -66,6 +66,14 @@ might be printed instead of the unreachable information::
 	(efault) data on invalid address
 	(einval) invalid data on a valid address
 
+Error pointers, i.e. pointers for which IS_ERR() is true, are handled
+as follows: If CONFIG_SYMBOLIC_ERRCODE is set, an appropriate symbolic
+constant is printed. For example, '"%p", PTR_ERR(-ENOSPC)' results in
+"ENOSPC", while '"%p", PTR_ERR(-EWOULDBLOCK)' results in "EAGAIN"
+(since EAGAIN == EWOULDBLOCK, and the former is the most common). If
+CONFIG_SYMBOLIC_ERRCODE is not set, ERR_PTRs are printed as their
+decimal representation ("-28" and "-11" for the two examples).
+
 Plain Pointers
 --------------
 
diff --git a/include/linux/errcode.h b/include/linux/errcode.h
new file mode 100644
index 000000000000..c6a4c1b04f9c
--- /dev/null
+++ b/include/linux/errcode.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ERRCODE_H
+#define _LINUX_ERRCODE_H
+
+#include <linux/stddef.h>
+
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+const char *errcode(int err);
+#else
+static inline const char *errcode(int err)
+{
+	return NULL;
+}
+#endif
+
+#endif /* _LINUX_ERRCODE_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5960e2980a8a..dc1b20872774 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -164,6 +164,14 @@ config DYNAMIC_DEBUG
 	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
 	  information.
 
+config SYMBOLIC_ERRCODE
+	bool "Support symbolic error codes in printf"
+	help
+	  If you say Y here, the kernel's printf implementation will
+	  be able to print symbolic errors such as ENOSPC instead of
+	  the number 28. It makes the kernel image slightly larger
+	  (about 3KB), but can make the kernel logs easier to read.
+
 endmenu # "printk and dmesg options"
 
 menu "Compile-time checks and compiler options"
diff --git a/lib/Makefile b/lib/Makefile
index 29c02a924973..664ecf10ee2a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -185,6 +185,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
 obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
 
 obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
+obj-$(CONFIG_SYMBOLIC_ERRCODE) += errcode.o
 
 obj-$(CONFIG_NLATTR) += nlattr.o
 
diff --git a/lib/errcode.c b/lib/errcode.c
new file mode 100644
index 000000000000..7dcf97d5307f
--- /dev/null
+++ b/lib/errcode.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/build_bug.h>
+#include <linux/errno.h>
+#include <linux/errcode.h>
+#include <linux/kernel.h>
+
+/*
+ * Ensure these tables to not accidentally become gigantic if some
+ * huge errno makes it in. On most architectures, the first table will
+ * only have about 140 entries, but mips and parisc have more sparsely
+ * 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.
+ */
+#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
+static const char *codes_0[] = {
+	E(E2BIG),
+	E(EACCES),
+	E(EADDRINUSE),
+	E(EADDRNOTAVAIL),
+	E(EADV),
+	E(EAFNOSUPPORT),
+	E(EALREADY),
+	E(EBADE),
+	E(EBADF),
+	E(EBADFD),
+	E(EBADMSG),
+	E(EBADR),
+	E(EBADRQC),
+	E(EBADSLT),
+	E(EBFONT),
+	E(EBUSY),
+#ifdef ECANCELLED
+	E(ECANCELLED),
+#endif
+	E(ECHILD),
+	E(ECHRNG),
+	E(ECOMM),
+	E(ECONNABORTED),
+	E(ECONNRESET),
+	E(EDEADLOCK),
+	E(EDESTADDRREQ),
+	E(EDOM),
+	E(EDOTDOT),
+#ifndef CONFIG_MIPS
+	E(EDQUOT),
+#endif
+	E(EEXIST),
+	E(EFAULT),
+	E(EFBIG),
+	E(EHOSTDOWN),
+	E(EHOSTUNREACH),
+	E(EHWPOISON),
+	E(EIDRM),
+	E(EILSEQ),
+#ifdef EINIT
+	E(EINIT),
+#endif
+	E(EINPROGRESS),
+	E(EINTR),
+	E(EINVAL),
+	E(EIO),
+	E(EISCONN),
+	E(EISDIR),
+	E(EISNAM),
+	E(EKEYEXPIRED),
+	E(EKEYREJECTED),
+	E(EKEYREVOKED),
+	E(EL2HLT),
+	E(EL2NSYNC),
+	E(EL3HLT),
+	E(EL3RST),
+	E(ELIBACC),
+	E(ELIBBAD),
+	E(ELIBEXEC),
+	E(ELIBMAX),
+	E(ELIBSCN),
+	E(ELNRNG),
+	E(ELOOP),
+	E(EMEDIUMTYPE),
+	E(EMFILE),
+	E(EMLINK),
+	E(EMSGSIZE),
+	E(EMULTIHOP),
+	E(ENAMETOOLONG),
+	E(ENAVAIL),
+	E(ENETDOWN),
+	E(ENETRESET),
+	E(ENETUNREACH),
+	E(ENFILE),
+	E(ENOANO),
+	E(ENOBUFS),
+	E(ENOCSI),
+	E(ENODATA),
+	E(ENODEV),
+	E(ENOENT),
+	E(ENOEXEC),
+	E(ENOKEY),
+	E(ENOLCK),
+	E(ENOLINK),
+	E(ENOMEDIUM),
+	E(ENOMEM),
+	E(ENOMSG),
+	E(ENONET),
+	E(ENOPKG),
+	E(ENOPROTOOPT),
+	E(ENOSPC),
+	E(ENOSR),
+	E(ENOSTR),
+#ifdef ENOSYM
+	E(ENOSYM),
+#endif
+	E(ENOSYS),
+	E(ENOTBLK),
+	E(ENOTCONN),
+	E(ENOTDIR),
+	E(ENOTEMPTY),
+	E(ENOTNAM),
+	E(ENOTRECOVERABLE),
+	E(ENOTSOCK),
+	E(ENOTTY),
+	E(ENOTUNIQ),
+	E(ENXIO),
+	E(EOPNOTSUPP),
+	E(EOVERFLOW),
+	E(EOWNERDEAD),
+	E(EPERM),
+	E(EPFNOSUPPORT),
+	E(EPIPE),
+#ifdef EPROCLIM
+	E(EPROCLIM),
+#endif
+	E(EPROTO),
+	E(EPROTONOSUPPORT),
+	E(EPROTOTYPE),
+	E(ERANGE),
+	E(EREMCHG),
+#ifdef EREMDEV
+	E(EREMDEV),
+#endif
+	E(EREMOTE),
+	E(EREMOTEIO),
+#ifdef EREMOTERELEASE
+	E(EREMOTERELEASE),
+#endif
+	E(ERESTART),
+	E(ERFKILL),
+	E(EROFS),
+#ifdef ERREMOTE
+	E(ERREMOTE),
+#endif
+	E(ESHUTDOWN),
+	E(ESOCKTNOSUPPORT),
+	E(ESPIPE),
+	E(ESRCH),
+	E(ESRMNT),
+	E(ESTALE),
+	E(ESTRPIPE),
+	E(ETIME),
+	E(ETIMEDOUT),
+	E(ETOOMANYREFS),
+	E(ETXTBSY),
+	E(EUCLEAN),
+	E(EUNATCH),
+	E(EUSERS),
+	E(EXDEV),
+	E(EXFULL),
+
+	E(ECANCELED),
+	E(EAGAIN), /* EWOULDBLOCK */
+	E(ECONNREFUSED), /* EREFUSED */
+	E(EDEADLK), /* EDEADLK */
+};
+#undef E
+
+#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
+static const char *codes_512[] = {
+	E(ERESTARTSYS),
+	E(ERESTARTNOINTR),
+	E(ERESTARTNOHAND),
+	E(ENOIOCTLCMD),
+	E(ERESTART_RESTARTBLOCK),
+	E(EPROBE_DEFER),
+	E(EOPENSTALE),
+	E(ENOPARAM),
+
+	E(EBADHANDLE),
+	E(ENOTSYNC),
+	E(EBADCOOKIE),
+	E(ENOTSUPP),
+	E(ETOOSMALL),
+	E(ESERVERFAULT),
+	E(EBADTYPE),
+	E(EJUKEBOX),
+	E(EIOCBQUEUED),
+	E(ERECALLCONFLICT),
+};
+#undef E
+
+const char *errcode(int err)
+{
+	/* Might as well accept both -EIO and EIO. */
+	if (err < 0)
+		err = -err;
+	if (err <= 0) /* INT_MIN or 0 */
+		return NULL;
+	if (err < ARRAY_SIZE(codes_0))
+		return codes_0[err];
+	if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
+		return codes_512[err - 512];
+	/* But why? */
+	if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
+		return "EDQUOT";
+	return NULL;
+}
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 944eb50f3862..0401a2341245 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -212,6 +212,7 @@ test_string(void)
 #define PTR_STR "ffff0123456789ab"
 #define PTR_VAL_NO_CRNG "(____ptrval____)"
 #define ZEROS "00000000"	/* hex 32 zero bits */
+#define FFFFS "ffffffff"
 
 static int __init
 plain_format(void)
@@ -243,6 +244,7 @@ plain_format(void)
 #define PTR_STR "456789ab"
 #define PTR_VAL_NO_CRNG "(ptrval)"
 #define ZEROS ""
+#define FFFFS ""
 
 static int __init
 plain_format(void)
@@ -588,6 +590,17 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+static void __init
+errptr(void)
+{
+	test("-1234", "%p", ERR_PTR(-1234));
+	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
+	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
+#endif
+}
+
 static void __init
 test_pointer(void)
 {
@@ -610,6 +623,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	errptr();
 }
 
 static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..bfa5c3025965 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -21,6 +21,7 @@
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/errcode.h>
 #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
 #include <linux/types.h>
 #include <linux/string.h>
@@ -2111,6 +2112,31 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
+	/*
+	 * If it's an ERR_PTR, try to print its symbolic
+	 * representation, except for %px, where the user explicitly
+	 * wanted the pointer formatted as a hex value.
+	 */
+	if (IS_ERR(ptr) && *fmt != 'x') {
+		long err = PTR_ERR(ptr);
+		const char *sym = errcode(-err);
+		if (sym)
+			return string_nocheck(buf, end, sym, spec);
+		/*
+		 * Funky, somebody passed ERR_PTR(-1234) or some other
+		 * non-existing Efoo - or more likely
+		 * CONFIG_SYMBOLIC_ERRCODE=n. None of the
+		 * %p<something> extensions can make any sense of an
+		 * ERR_PTR(), and if this was just a plain %p, the
+		 * user is still better off getting the decimal
+		 * representation rather than the hash value that
+		 * ptr_to_id() would generate.
+		 */
+		spec.flags |= SIGN;
+		spec.base = 10;
+		return number(buf, end, err, spec);
+	}
+
 	switch (*fmt) {
 	case 'F':
 	case 'f':
-- 
2.20.1

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Andy Shevchenko]

On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is
>
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %p\n", foo);
>     return PTR_ERR(foo);
>   }
>
> it is also more helpful to get a symbolic error code (or, worst case,
> a decimal number) in case an ERR_PTR is accidentally passed to some
> %p<something>, rather than the (efault) that check_pointer() would
> result in.
>
> With my embedded hat on, I've made it possible to remove this.
>
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.
>
> The symbols to include have been found by massaging the output of
>
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.

> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err

From long term prospective 300 and 550 hard coded here may be forgotten.

> +const char *errcode(int err)
We got long, why not to use long type for it?

> +{
> +       /* Might as well accept both -EIO and EIO. */
> +       if (err < 0)
> +               err = -err;

> +       if (err <= 0) /* INT_MIN or 0 */
> +               return NULL;
> +       if (err < ARRAY_SIZE(codes_0))

> +               return codes_0[err];

It won't work if one of the #ifdef:s in the array fails.
Would it?

> +       if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
> +               return codes_512[err - 512];
> +       /* But why? */
> +       if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
> +               return "EDQUOT";
> +       return NULL;
> +}

> +               long err = PTR_ERR(ptr);
> +               const char *sym = errcode(-err);

Do we need additional sign change if we already have such check inside
errcode()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Joe Perches]

On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> > It has been suggested several times to extend vsnprintf() to be able
> > to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> > another attempt. Rather than adding another %p extension, simply teach
> > plain %p to convert ERR_PTRs. While the primary use case is
> > 
> >   if (IS_ERR(foo)) {
> >     pr_err("Sorry, can't do that: %p\n", foo);
> >     return PTR_ERR(foo);
> >   }
> > 
> > it is also more helpful to get a symbolic error code (or, worst case,
> > a decimal number) in case an ERR_PTR is accidentally passed to some
> > %p<something>, rather than the (efault) that check_pointer() would
> > result in.
> > 
> > With my embedded hat on, I've made it possible to remove this.
> > 
> > I've tested that the #ifdeffery in errcode.c is sufficient to make
> > this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> > 0day bot will tell me which ones I've missed.
> > 
> > The symbols to include have been found by massaging the output of
> > 
> >   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
> > 
> > In the cases where some common aliasing exists
> > (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> > I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> > to the bottom so that one takes precedence.
> > +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> > +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
> 
> From long term prospective 300 and 550 hard coded here may be forgotten.
> 
> > +const char *errcode(int err)
> We got long, why not to use long type for it?
> 
> > +{
> > +       /* Might as well accept both -EIO and EIO. */
> > +       if (err < 0)
> > +               err = -err;
> > +       if (err <= 0) /* INT_MIN or 0 */
> > +               return NULL;
> > +       if (err < ARRAY_SIZE(codes_0))
> > +               return codes_0[err];
> 
> It won't work if one of the #ifdef:s in the array fails.
> Would it?
> 
> > +       if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
> > +               return codes_512[err - 512];
> > +       /* But why? */
> > +       if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
> > +               return "EDQUOT";
> > +       return NULL;
> > +}
> > +               long err = PTR_ERR(ptr);
> > +               const char *sym = errcode(-err);
> 
> Do we need additional sign change if we already have such check inside
> errcode()?

How is EBUSY differentiated from ZERO_SIZE_PTR ?

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 11/09/2019 02.15, Joe Perches wrote:
> On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
>> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
>> <linux@rasmusvillemoes.dk> wrote:

>>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
>>> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
>>
>> From long term prospective 300 and 550 hard coded here may be forgotten.

No? The point of the BUILD_BUG_ON_ZEROs is that if you add a new
Esomething, you'll get an instant build error if Esomething doesn't fit
nicely in the array you put it in. Then one can go back and figure out
whether the limit should be raised, a new codes_foo should be created,
or if it's early enough so it's not ABI yet, simply change Esomething to
a saner value.

A much bigger problem is that it's possible to add something to some
errno.h without updating this table, but there's no good solution for
that, I'm afraid. However, new Esomething are very rarely added, and
printf() will still handle it gracefully until somebody notices.

>>> +const char *errcode(int err)
>> We got long, why not to use long type for it?

Because errno values by definition have type int - and the linux syscall
ABI very clearly limits values to [1,4095]. I can change the type used
in vsnprintf.c if you prefer.

>>> +{
>>> +       /* Might as well accept both -EIO and EIO. */
>>> +       if (err < 0)
>>> +               err = -err;
>>> +       if (err <= 0) /* INT_MIN or 0 */
>>> +               return NULL;
>>> +       if (err < ARRAY_SIZE(codes_0))
>>> +               return codes_0[err];
>>
>> It won't work if one of the #ifdef:s in the array fails.
>> Would it?

I don't understand what you mean. How can an ifdef fail(?), and what
exactly won't work?

>>> +}
>>> +               long err = PTR_ERR(ptr);
>>> +               const char *sym = errcode(-err);
>>
>> Do we need additional sign change if we already have such check inside
>> errcode()?

Nah, but I went back and forth on this and ended up falling between two
stools. I think I'll drop the handling of negative arguments to
errcode(), the INT_MIN case makes that slightly ugly anyway.

> How is EBUSY differentiated from ZERO_SIZE_PTR ?

Huh? ZERO_SIZE_PTR aka (void*)(16L) is not IS_ERR(), so we won't get
here in that case.

Rasmus

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2060 bytes --]

On 9/11/19 8:43 AM, Rasmus Villemoes wrote:
> On 11/09/2019 02.15, Joe Perches wrote:
>> On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
>>> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
>>> <linux@rasmusvillemoes.dk> wrote:
> 
>>>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
>>>> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
>>>
>>> From long term prospective 300 and 550 hard coded here may be forgotten.
> 
> No? The point of the BUILD_BUG_ON_ZEROs is that if you add a new
> Esomething, you'll get an instant build error if Esomething doesn't fit
> nicely in the array you put it in. Then one can go back and figure out
> whether the limit should be raised, a new codes_foo should be created,
> or if it's early enough so it's not ABI yet, simply change Esomething to
> a saner value.
> 
> A much bigger problem is that it's possible to add something to some
> errno.h without updating this table, but there's no good solution for
> that, I'm afraid. However, new Esomething are very rarely added, and
> printf() will still handle it gracefully until somebody notices.
> 
>>>> +const char *errcode(int err)
>>> We got long, why not to use long type for it?
> 
> Because errno values by definition have type int - and the linux syscall
> ABI very clearly limits values to [1,4095]. I can change the type used
> in vsnprintf.c if you prefer.
> 
>>>> +{
>>>> +       /* Might as well accept both -EIO and EIO. */
>>>> +       if (err < 0)
>>>> +               err = -err;
>>>> +       if (err <= 0) /* INT_MIN or 0 */
>>>> +               return NULL;
>>>> +       if (err < ARRAY_SIZE(codes_0))
>>>> +               return codes_0[err];
>>>
>>> It won't work if one of the #ifdef:s in the array fails.
>>> Would it?
> 
> I don't understand what you mean. How can an ifdef fail(?), and what
> exactly won't work?

I think Joe means: What happens if codes_0[57] is "" because there is no
ESOMETHING with value 57.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 11/09/2019 11.37, Uwe Kleine-König wrote:
> On 9/11/19 8:43 AM, Rasmus Villemoes wrote:
>> On 11/09/2019 02.15, Joe Perches wrote:
>>> On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
>>>> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
>>>> <linux@rasmusvillemoes.dk> wrote:
>>>>> +{
>>>>> +       /* Might as well accept both -EIO and EIO. */
>>>>> +       if (err < 0)
>>>>> +               err = -err;
>>>>> +       if (err <= 0) /* INT_MIN or 0 */
>>>>> +               return NULL;
>>>>> +       if (err < ARRAY_SIZE(codes_0))
>>>>> +               return codes_0[err];
>>>>
>>>> It won't work if one of the #ifdef:s in the array fails.
>>>> Would it?
>>
>> I don't understand what you mean. How can an ifdef fail(?), and what
>> exactly won't work?
> 
> I think Joe means: What happens if codes_0[57] is "" because there is no
> ESOMETHING with value 57.

[That was Andy, not Joe, I was lazy and replied to both in one email
since Joe quoted all of Andy's questions].

So first, for good measure, codes_0[57] may be NULL but not "". Second,
if we're passed 57 but no Exxx on this architecture has the value 57,
then yes, we return NULL, just as if we're passed -18 or 0 or 1234. But
57 (or -57, or ERR_PTR(-57)) would realistically never find its way into
an err variable (be it pointer or integer) in the first place when no
ESOMETHING has the value 57.

Except for the case where somebody in the future adds #define ESOMETHING
57 to their asm/errno.h and starts using ESOMETHING, without updating
the table in errcode.c. But if that's the case, letting the caller
handle the "sorry, I can't translate 57 to some string" is still the
right thing to do.

Rasmus

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Pavel Machek]

On Mon 2019-09-09 22:38:25, Rasmus Villemoes wrote:
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is

For the record, I hate manually decoding errors, so I like this patch.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2000 bytes --]

Hello Rasmus,

On 9/9/19 10:38 PM, Rasmus Villemoes wrote:
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is
> 
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %p\n", foo);
>     return PTR_ERR(foo);
>   }
> 
> it is also more helpful to get a symbolic error code (or, worst case,
> a decimal number) in case an ERR_PTR is accidentally passed to some
> %p<something>, rather than the (efault) that check_pointer() would
> result in.
> 
> With my embedded hat on, I've made it possible to remove this.
> 
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.
> 
> The symbols to include have been found by massaging the output of
> 
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
> 
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.
> 
> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Even with my ack already given I still think having %pE (or %pe) for
ints holding an error code is sensible. So I wonder if it would be a
good idea to split this patch into one that introduces errcode() and
then the patch that teaches vsprintf about emitting its return value for
error valued pointers. Then I could rebase my initial patch for %pe on
top of your first one.

Other than that I wonder how we can go forward from here. So I think it
is time for v3 which picks up the few suggestions.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 16/09/2019 14.23, Uwe Kleine-König wrote:
> Hello Rasmus,
> 
> On 9/9/19 10:38 PM, Rasmus Villemoes wrote:
>> It has been suggested several times to extend vsnprintf() to be able
>> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
>> another attempt. Rather than adding another %p extension, simply teach
>> plain %p to convert ERR_PTRs. While the primary use case is
>>
>>   if (IS_ERR(foo)) {
>>     pr_err("Sorry, can't do that: %p\n", foo);
>>     return PTR_ERR(foo);
>>   }
>>
>> it is also more helpful to get a symbolic error code (or, worst case,
>> a decimal number) in case an ERR_PTR is accidentally passed to some
>> %p<something>, rather than the (efault) that check_pointer() would
>> result in.
>>
>> With my embedded hat on, I've made it possible to remove this.
>>
>> I've tested that the #ifdeffery in errcode.c is sufficient to make
>> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
>> 0day bot will tell me which ones I've missed.
>>
>> The symbols to include have been found by massaging the output of
>>
>>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>>
>> In the cases where some common aliasing exists
>> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
>> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
>> to the bottom so that one takes precedence.
>>
>> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> Even with my ack already given I still think having %pE (or %pe) for
> ints holding an error code is sensible. 

I don't understand why you'd want an explicit %p<something> to do what
%p does by itself - in fact, with the current vsnprintf implementation,
"%pe", ERR_PTR(-EFOO) would already do what you want (since after %p is
processed, all alphanumeric are skipped whether they were interpreted or
not). So we could "reserve" %pe perhaps in order to make the call sites
a little more readable, but no code change in vsnprintf.c would be
necessary.

Or did you mean %pe with the argument being an (int*), so one would do

  if (err < 0)
    pr_err("bad: %pe\n", &err);

Maybe I'd buy that one, though I don't think it's much worse to do

  if (err < 0)
    pr_err("bad: %p\n", ERR_PTR(err));

Also, the former has less type safety/type genericity than the latter;
if err happens to be a long (or s8 or s16) the former won't work while
the latter will.

Or perhaps you meant introduce a %d<something> extension? I still think
that's a bad idea, and I've in the meantime found another reason
(covering %d in particular): Netdevices can be given a name containing
exactly one occurrence of %d (or no % at all), and then the actual name
will be determined based on that pattern. These patterns are settable
from userspace. And everything of course breaks horribly if somebody set
a name to "bla%deth" and that got turned into "blaEPERMth".

> So I wonder if it would be a
> good idea to split this patch into one that introduces errcode() and
> then the patch that teaches vsprintf about emitting its return value for
> error valued pointers. Then I could rebase my initial patch for %pe on
> top of your first one.

Well, I think my patch as-is is simple enough, there's not much point
separating the few lines in vsnprintf() from the introduction of
errcode() (which, realistically, will never have other callers).

> Other than that I wonder how we can go forward from here. So I think it
> is time for v3 which picks up the few suggestions.

Yes, I have actually prepared a v3, was just waiting for additional
comments on my responses to the v2 review comments.

Rasmus

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3786 bytes --]

On 9/16/19 3:23 PM, Rasmus Villemoes wrote:
> On 16/09/2019 14.23, Uwe Kleine-König wrote:
>> Hello Rasmus,
>>
>> On 9/9/19 10:38 PM, Rasmus Villemoes wrote:
>>> It has been suggested several times to extend vsnprintf() to be able
>>> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
>>> another attempt. Rather than adding another %p extension, simply teach
>>> plain %p to convert ERR_PTRs. While the primary use case is
>>>
>>>   if (IS_ERR(foo)) {
>>>     pr_err("Sorry, can't do that: %p\n", foo);
>>>     return PTR_ERR(foo);
>>>   }
>>>
>>> it is also more helpful to get a symbolic error code (or, worst case,
>>> a decimal number) in case an ERR_PTR is accidentally passed to some
>>> %p<something>, rather than the (efault) that check_pointer() would
>>> result in.
>>>
>>> With my embedded hat on, I've made it possible to remove this.
>>>
>>> I've tested that the #ifdeffery in errcode.c is sufficient to make
>>> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
>>> 0day bot will tell me which ones I've missed.
>>>
>>> The symbols to include have been found by massaging the output of
>>>
>>>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>>>
>>> In the cases where some common aliasing exists
>>> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
>>> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
>>> to the bottom so that one takes precedence.
>>>
>>> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>
>> Even with my ack already given I still think having %pE (or %pe) for
>> ints holding an error code is sensible. 
> 
> I don't understand why you'd want an explicit %p<something> to do what
> %p does by itself - in fact, with the current vsnprintf implementation,
> "%pe", ERR_PTR(-EFOO) would already do what you want (since after %p is
> processed, all alphanumeric are skipped whether they were interpreted or
> not). So we could "reserve" %pe perhaps in order to make the call sites
> a little more readable, but no code change in vsnprintf.c would be
> necessary.

Sorry, I meant I still consider %de (or %dE) sensible which I suggested
at the start of this thread.

> Or perhaps you meant introduce a %d<something> extension? I still think
> that's a bad idea, and I've in the meantime found another reason
> (covering %d in particular): Netdevices can be given a name containing
> exactly one occurrence of %d (or no % at all), and then the actual name
> will be determined based on that pattern. These patterns are settable
> from userspace. And everything of course breaks horribly if somebody set
> a name to "bla%deth" and that got turned into "blaEPERMth".

Sure, this should not happen. I don't see imminent danger though.
(ethernet IDs are usually positive, right?)

I think having a possibility to print error codes in an int is
beneficial, as otherwise I'd have to convert to a pointer first when
printing the code which is IMHO unnecessary burden.

>> So I wonder if it would be a
>> good idea to split this patch into one that introduces errcode() and
>> then the patch that teaches vsprintf about emitting its return value for
>> error valued pointers. Then I could rebase my initial patch for %pe on
>> top of your first one.
> 
> Well, I think my patch as-is is simple enough, there's not much point
> separating the few lines in vsnprintf() from the introduction of
> errcode() (which, realistically, will never have other callers).

Fine if your series goes in soon. If not I'd like to use errcode()
without having to discuss the changes to how pointers are printed.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

It has been suggested several times to extend vsnprintf() to be able
to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
another attempt. Rather than adding another %p extension, simply teach
plain %p to convert ERR_PTRs. While the primary use case is

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %p\n", foo);
    return PTR_ERR(foo);
  }

it is also more helpful to get a symbolic error code (or, worst case,
a decimal number) in case an ERR_PTR is accidentally passed to some
%p<something>, rather than the (efault) that check_pointer() would
result in.

With my embedded hat on, I've made it possible to remove this.

I've tested that the #ifdeffery in errcode.c is sufficient to make
this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
0day bot will tell me which ones I've missed.

The symbols to include have been found by massaging the output of

  find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'

In the cases where some common aliasing exists
(e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
to the bottom so that one takes precedence.

Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---

Andrew: please consider picking this up, even if we're already in the
merge window. Quite a few people have said they'd like to see
something like this, it's a debug improvement in its own right (the
"ERR_PTR accidentally passed to printf" case), the printf tests pass,
and it's much easier to start adding (and testing) users around the
tree once this is in master.

v3:
- only accept positive errno values in errcode()
- change type of err variable in pointer() from long to int
v2:
- add #include <linux/stddef.h> to errcode.h (0day)
- keep 'x' handling in switch() (Andy)
- add paragraph to Documentation/core-api/printk-formats.rst
- add ack from Uwe

 Documentation/core-api/printk-formats.rst |   8 +
 include/linux/errcode.h                   |  16 ++
 lib/Kconfig.debug                         |   8 +
 lib/Makefile                              |   1 +
 lib/errcode.c                             | 212 ++++++++++++++++++++++
 lib/test_printf.c                         |  14 ++
 lib/vsprintf.c                            |  26 +++
 7 files changed, 285 insertions(+)
 create mode 100644 include/linux/errcode.h
 create mode 100644 lib/errcode.c

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcb..7d3bf3cb207b 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -66,6 +66,14 @@ might be printed instead of the unreachable information::
 	(efault) data on invalid address
 	(einval) invalid data on a valid address
 
+Error pointers, i.e. pointers for which IS_ERR() is true, are handled
+as follows: If CONFIG_SYMBOLIC_ERRCODE is set, an appropriate symbolic
+constant is printed. For example, '"%p", PTR_ERR(-ENOSPC)' results in
+"ENOSPC", while '"%p", PTR_ERR(-EWOULDBLOCK)' results in "EAGAIN"
+(since EAGAIN == EWOULDBLOCK, and the former is the most common). If
+CONFIG_SYMBOLIC_ERRCODE is not set, ERR_PTRs are printed as their
+decimal representation ("-28" and "-11" for the two examples).
+
 Plain Pointers
 --------------
 
diff --git a/include/linux/errcode.h b/include/linux/errcode.h
new file mode 100644
index 000000000000..c6a4c1b04f9c
--- /dev/null
+++ b/include/linux/errcode.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ERRCODE_H
+#define _LINUX_ERRCODE_H
+
+#include <linux/stddef.h>
+
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+const char *errcode(int err);
+#else
+static inline const char *errcode(int err)
+{
+	return NULL;
+}
+#endif
+
+#endif /* _LINUX_ERRCODE_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5960e2980a8a..dc1b20872774 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -164,6 +164,14 @@ config DYNAMIC_DEBUG
 	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
 	  information.
 
+config SYMBOLIC_ERRCODE
+	bool "Support symbolic error codes in printf"
+	help
+	  If you say Y here, the kernel's printf implementation will
+	  be able to print symbolic errors such as ENOSPC instead of
+	  the number 28. It makes the kernel image slightly larger
+	  (about 3KB), but can make the kernel logs easier to read.
+
 endmenu # "printk and dmesg options"
 
 menu "Compile-time checks and compiler options"
diff --git a/lib/Makefile b/lib/Makefile
index c5892807e06f..9f14edc7ef63 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -181,6 +181,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
 obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
 
 obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
+obj-$(CONFIG_SYMBOLIC_ERRCODE) += errcode.o
 
 obj-$(CONFIG_NLATTR) += nlattr.o
 
diff --git a/lib/errcode.c b/lib/errcode.c
new file mode 100644
index 000000000000..3e519b13245e
--- /dev/null
+++ b/lib/errcode.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/build_bug.h>
+#include <linux/errno.h>
+#include <linux/errcode.h>
+#include <linux/kernel.h>
+
+/*
+ * Ensure these tables to not accidentally become gigantic if some
+ * huge errno makes it in. On most architectures, the first table will
+ * only have about 140 entries, but mips and parisc have more sparsely
+ * 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.
+ */
+#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
+static const char *codes_0[] = {
+	E(E2BIG),
+	E(EACCES),
+	E(EADDRINUSE),
+	E(EADDRNOTAVAIL),
+	E(EADV),
+	E(EAFNOSUPPORT),
+	E(EALREADY),
+	E(EBADE),
+	E(EBADF),
+	E(EBADFD),
+	E(EBADMSG),
+	E(EBADR),
+	E(EBADRQC),
+	E(EBADSLT),
+	E(EBFONT),
+	E(EBUSY),
+#ifdef ECANCELLED
+	E(ECANCELLED),
+#endif
+	E(ECHILD),
+	E(ECHRNG),
+	E(ECOMM),
+	E(ECONNABORTED),
+	E(ECONNRESET),
+	E(EDEADLOCK),
+	E(EDESTADDRREQ),
+	E(EDOM),
+	E(EDOTDOT),
+#ifndef CONFIG_MIPS
+	E(EDQUOT),
+#endif
+	E(EEXIST),
+	E(EFAULT),
+	E(EFBIG),
+	E(EHOSTDOWN),
+	E(EHOSTUNREACH),
+	E(EHWPOISON),
+	E(EIDRM),
+	E(EILSEQ),
+#ifdef EINIT
+	E(EINIT),
+#endif
+	E(EINPROGRESS),
+	E(EINTR),
+	E(EINVAL),
+	E(EIO),
+	E(EISCONN),
+	E(EISDIR),
+	E(EISNAM),
+	E(EKEYEXPIRED),
+	E(EKEYREJECTED),
+	E(EKEYREVOKED),
+	E(EL2HLT),
+	E(EL2NSYNC),
+	E(EL3HLT),
+	E(EL3RST),
+	E(ELIBACC),
+	E(ELIBBAD),
+	E(ELIBEXEC),
+	E(ELIBMAX),
+	E(ELIBSCN),
+	E(ELNRNG),
+	E(ELOOP),
+	E(EMEDIUMTYPE),
+	E(EMFILE),
+	E(EMLINK),
+	E(EMSGSIZE),
+	E(EMULTIHOP),
+	E(ENAMETOOLONG),
+	E(ENAVAIL),
+	E(ENETDOWN),
+	E(ENETRESET),
+	E(ENETUNREACH),
+	E(ENFILE),
+	E(ENOANO),
+	E(ENOBUFS),
+	E(ENOCSI),
+	E(ENODATA),
+	E(ENODEV),
+	E(ENOENT),
+	E(ENOEXEC),
+	E(ENOKEY),
+	E(ENOLCK),
+	E(ENOLINK),
+	E(ENOMEDIUM),
+	E(ENOMEM),
+	E(ENOMSG),
+	E(ENONET),
+	E(ENOPKG),
+	E(ENOPROTOOPT),
+	E(ENOSPC),
+	E(ENOSR),
+	E(ENOSTR),
+#ifdef ENOSYM
+	E(ENOSYM),
+#endif
+	E(ENOSYS),
+	E(ENOTBLK),
+	E(ENOTCONN),
+	E(ENOTDIR),
+	E(ENOTEMPTY),
+	E(ENOTNAM),
+	E(ENOTRECOVERABLE),
+	E(ENOTSOCK),
+	E(ENOTTY),
+	E(ENOTUNIQ),
+	E(ENXIO),
+	E(EOPNOTSUPP),
+	E(EOVERFLOW),
+	E(EOWNERDEAD),
+	E(EPERM),
+	E(EPFNOSUPPORT),
+	E(EPIPE),
+#ifdef EPROCLIM
+	E(EPROCLIM),
+#endif
+	E(EPROTO),
+	E(EPROTONOSUPPORT),
+	E(EPROTOTYPE),
+	E(ERANGE),
+	E(EREMCHG),
+#ifdef EREMDEV
+	E(EREMDEV),
+#endif
+	E(EREMOTE),
+	E(EREMOTEIO),
+#ifdef EREMOTERELEASE
+	E(EREMOTERELEASE),
+#endif
+	E(ERESTART),
+	E(ERFKILL),
+	E(EROFS),
+#ifdef ERREMOTE
+	E(ERREMOTE),
+#endif
+	E(ESHUTDOWN),
+	E(ESOCKTNOSUPPORT),
+	E(ESPIPE),
+	E(ESRCH),
+	E(ESRMNT),
+	E(ESTALE),
+	E(ESTRPIPE),
+	E(ETIME),
+	E(ETIMEDOUT),
+	E(ETOOMANYREFS),
+	E(ETXTBSY),
+	E(EUCLEAN),
+	E(EUNATCH),
+	E(EUSERS),
+	E(EXDEV),
+	E(EXFULL),
+
+	E(ECANCELED),
+	E(EAGAIN), /* EWOULDBLOCK */
+	E(ECONNREFUSED), /* EREFUSED */
+	E(EDEADLK), /* EDEADLK */
+};
+#undef E
+
+#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
+static const char *codes_512[] = {
+	E(ERESTARTSYS),
+	E(ERESTARTNOINTR),
+	E(ERESTARTNOHAND),
+	E(ENOIOCTLCMD),
+	E(ERESTART_RESTARTBLOCK),
+	E(EPROBE_DEFER),
+	E(EOPENSTALE),
+	E(ENOPARAM),
+
+	E(EBADHANDLE),
+	E(ENOTSYNC),
+	E(EBADCOOKIE),
+	E(ENOTSUPP),
+	E(ETOOSMALL),
+	E(ESERVERFAULT),
+	E(EBADTYPE),
+	E(EJUKEBOX),
+	E(EIOCBQUEUED),
+	E(ERECALLCONFLICT),
+};
+#undef E
+
+const char *errcode(int err)
+{
+	if (err <= 0)
+		return NULL;
+	if (err < ARRAY_SIZE(codes_0))
+		return codes_0[err];
+	if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
+		return codes_512[err - 512];
+	/* But why? */
+	if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
+		return "EDQUOT";
+	return NULL;
+}
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 944eb50f3862..0401a2341245 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -212,6 +212,7 @@ test_string(void)
 #define PTR_STR "ffff0123456789ab"
 #define PTR_VAL_NO_CRNG "(____ptrval____)"
 #define ZEROS "00000000"	/* hex 32 zero bits */
+#define FFFFS "ffffffff"
 
 static int __init
 plain_format(void)
@@ -243,6 +244,7 @@ plain_format(void)
 #define PTR_STR "456789ab"
 #define PTR_VAL_NO_CRNG "(ptrval)"
 #define ZEROS ""
+#define FFFFS ""
 
 static int __init
 plain_format(void)
@@ -588,6 +590,17 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+static void __init
+errptr(void)
+{
+	test("-1234", "%p", ERR_PTR(-1234));
+	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
+	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
+#endif
+}
+
 static void __init
 test_pointer(void)
 {
@@ -610,6 +623,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	errptr();
 }
 
 static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..299fce317eb3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -21,6 +21,7 @@
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/errcode.h>
 #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
 #include <linux/types.h>
 #include <linux/string.h>
@@ -2111,6 +2112,31 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
+	/*
+	 * If it's an ERR_PTR, try to print its symbolic
+	 * representation, except for %px, where the user explicitly
+	 * wanted the pointer formatted as a hex value.
+	 */
+	if (IS_ERR(ptr) && *fmt != 'x') {
+		int err = PTR_ERR(ptr);
+		const char *sym = errcode(-err);
+		if (sym)
+			return string_nocheck(buf, end, sym, spec);
+		/*
+		 * Funky, somebody passed ERR_PTR(-1234) or some other
+		 * non-existing Efoo - or more likely
+		 * CONFIG_SYMBOLIC_ERRCODE=n. None of the
+		 * %p<something> extensions can make any sense of an
+		 * ERR_PTR(), and if this was just a plain %p, the
+		 * user is still better off getting the decimal
+		 * representation rather than the hash value that
+		 * ptr_to_id() would generate.
+		 */
+		spec.flags |= SIGN;
+		spec.base = 10;
+		return number(buf, end, err, spec);
+	}
+
 	switch (*fmt) {
 	case 'F':
 	case 'f':
-- 
2.20.1

Re: [PATCH v3] printf: add support for printing symbolic error codes

[Petr Mladek]

First, I am sorry that I replay so late. I was traveling the previous
two weeks and was not able to follow the discussion about this patch.

I am fine with adding this feature. But I would like to do it
a cleaner way, see below.


On Tue 2019-09-17 08:59:59, Rasmus Villemoes wrote:
> With my embedded hat on, I've made it possible to remove this.
> 
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.

Please, remove the above two paragraphs in the final patch. They make
sense for review but not for git history.


> Andrew: please consider picking this up, even if we're already in the
> merge window. Quite a few people have said they'd like to see
> something like this, it's a debug improvement in its own right (the
> "ERR_PTR accidentally passed to printf" case), the printf tests pass,
> and it's much easier to start adding (and testing) users around the
> tree once this is in master.

This change would deserve to spend some time in linux-next. Anyway,
it is already too late for 5.4.


> diff --git a/include/linux/errcode.h b/include/linux/errcode.h
> new file mode 100644
> index 000000000000..c6a4c1b04f9c
> --- /dev/null
> +++ b/include/linux/errcode.h

The word "code" is quite ambiguous. I am not sure if it is the name or
the number. I think that it is actually both (the relation between
the two.

Both "man 3 errno" and
https://www.gnu.org/software/libc/manual/html_node/Checking-for-Errors.html#Checking-for-Errors
talks about numbers and symbolic names.

Please use errname or so instead of errcode everywhere.


> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 5960e2980a8a..dc1b20872774 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -164,6 +164,14 @@ config DYNAMIC_DEBUG
>  	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
>  	  information.
>  
> +config SYMBOLIC_ERRCODE

What is the exact reason to make this configurable, please?

Nobody was really against this feature. The only question
was if it was worth the code complexity and maintenance.
If we are going to have the code then we should use it.

Then there was a concerns that it might be too big for embedded
people. But did it come from people working on embedded kernel
or was it just theoretical?

I would personally enable it when CONFIG_PRINTK is enabled.
We could always introduce a new config option later when
anyone really wants to disable it.

> --- /dev/null
> +++ b/lib/errcode.c
> @@ -0,0 +1,212 @@
> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> +static const char *codes_0[] = {
> +	E(E2BIG),

I really like the way how the array is initialized.


> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 944eb50f3862..0401a2341245 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> +static void __init
> +errptr(void)
> +{
> +	test("-1234", "%p", ERR_PTR(-1234));
> +	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
> +#ifdef CONFIG_SYMBOLIC_ERRCODE
> +	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
> +	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));

I like that you check more values. But please split it to check
only one value per line to make it better readable.


> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137..299fce317eb3 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2111,6 +2112,31 @@ static noinline_for_stack
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	      struct printf_spec spec)
>  {
> +	/*
> +	 * If it's an ERR_PTR, try to print its symbolic
> +	 * representation, except for %px, where the user explicitly
> +	 * wanted the pointer formatted as a hex value.
> +	 */
> +	if (IS_ERR(ptr) && *fmt != 'x') {

We had similar code before the commit 3e5903eb9cff70730171 ("vsprintf:
Prevent crash when dereferencing invalid pointers"). Note that the
original code kept the original value also for *fmt == 'K'.

Anyway, the above commit tried to unify the handling of special
values:

   + use the same strings for special values
   + check for special values only when pointer is dereferenced

This patch makes it inconsistent again. I mean that the code will:

   + check for (null) and (efault) only when the pointer is
     dereferenced

   + check for err codes in more situations but not in all
     and not in %s

   + use another style to print the error (uppercase without
      brackets)


I would like to keep it consistent. My proposal is:

1. Print the plain error code name only when
   a new %pe modifier is used. This will be useful
   in the error messages, e.g.

	void *p = ERR_PTR(-ENOMEM);
	if (IS_ERR(foo)) {
		pr_err("Sorry, can't do that: %pe\n", foo);
	return PTR_ERR(foo);

   would produce

	Sorry, can't do that: -ENOMEM


2. Use error code names also in check_pointer_msg() instead
   of (efault). But put it into brackets to distinguish it
   from the expected value, for example:

      /* valid pointer */
      phys_addr_t addr = 0xab;
      printk("value: %pa\n", &addr);
      /* known error code */
      printk("value: %pa\n", ERR_PTR(-ENOMEM));
      /* unknown error code */
      printk("value: %pa\n", ERR_PTR(-1234));

   would produce:

     value: 0xab
     value: (-ENOMEM)
     value: (-1234)


3. Unify the style for the null pointer:

    + use (NULL) instead of (null)

4. Do not use error code names for internal vsprintf error
   to avoid confusion. For example:

    + replace the one (einval) with (%piS-err) or so

How does that sound, please?

Best Regards,
Petr

Re: [PATCH v3] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 25/09/2019 16.36, Petr Mladek wrote:
> First, I am sorry that I replay so late. I was traveling the previous
> two weeks and was not able to follow the discussion about this patch.
> 
> I am fine with adding this feature. But I would like to do it
> a cleaner way, see below.
> 
> 
> On Tue 2019-09-17 08:59:59, Rasmus Villemoes wrote:
>> With my embedded hat on, I've made it possible to remove this.
>>
>> I've tested that the #ifdeffery in errcode.c is sufficient to make
>> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
>> 0day bot will tell me which ones I've missed.
> 
> Please, remove the above two paragraphs in the final patch. They make
> sense for review but not for git history.

Agree for the latter, but not the former - I do want to explain why it's
possible to configure out; see also below.

> This change would deserve to spend some time in linux-next. Anyway,
> it is already too late for 5.4.

Yes, it's of course way too late now. Perhaps I should ask you to take
it via the printk tree? Anyway, let's see what we can agree to.

> The word "code" is quite ambiguous. I am not sure if it is the name or
> the number. I think that it is actually both (the relation between
> the two.
> 
> Both "man 3 errno" and
> https://www.gnu.org/software/libc/manual/html_node/Checking-for-Errors.html#Checking-for-Errors
> talks about numbers and symbolic names.
> 
> Please use errname or so instead of errcode everywhere.

OK. I wasn't too happy about errcode anyway - but I wanted to avoid
"str" being in there to avoid anyone thinking it was a strerror(). So
"CONFIG_SYMBOLIC_ERRNAME" and errname() seems fine with the above
justification.

>> +config SYMBOLIC_ERRCODE
> 
> What is the exact reason to make this configurable, please?
> 
> Nobody was really against this feature. The only question
> was if it was worth the code complexity and maintenance.
> If we are going to have the code then we should use it.
> 
> Then there was a concerns that it might be too big for embedded
> people. But did it come from people working on embedded kernel
> or was it just theoretical?

I am one such person, and while 3K may not be a lot, death by a thousand
paper cuts...

> I would personally enable it when CONFIG_PRINTK is enabled.

Agree. So let's make it 'default y if PRINTK'? These are only/mostly
useful when destined for dmesg, I can't imagine any of the sysfs/procfs
uses of vsprintf() to want this. So if somebody has gone to the rather
extremely length of disabling printk (which even I rarely do), they
really want the absolute minimal kernel, and would not benefit from this
anyway. While for the common case, it gets enabled for anyone that just
updates their defconfig and accepts new default values.

> We could always introduce a new config option later when
> anyone really wants to disable it.

No, because by the time the kernel has grown too large for some target,
it's almost impossible to start figuring out which small pieces can be
chopped away with suitable config options, and even harder to get
upstream to accept such configurability ("why, that would only gain you
3K...").

>> --- /dev/null
>> +++ b/lib/errcode.c
>> @@ -0,0 +1,212 @@
>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
>> +static const char *codes_0[] = {
>> +	E(E2BIG),
> 
> I really like the way how the array is initialized.

Thanks.

> 
>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>> index 944eb50f3862..0401a2341245 100644
>> --- a/lib/test_printf.c
>> +++ b/lib/test_printf.c
>> +static void __init
>> +errptr(void)
>> +{
>> +	test("-1234", "%p", ERR_PTR(-1234));
>> +	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
>> +#ifdef CONFIG_SYMBOLIC_ERRCODE
>> +	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
>> +	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
> 
> I like that you check more values. But please split it to check
> only one value per line to make it better readable.

Hm, ok, but I actually do it this way on purpose - I want to ensure that
the test where one passes a random not-zero-but-too-small buffer size
sometimes hits in the middle of the %p output, sometimes just before and
sometimes just after. The very reason I added test_printf was because
there were some %p extensions that violated the basic rule of
snprintf()'s return value and/or wrote beyond the provided buffer.

Not a big deal, so if you insist I'll break it up.

> 
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index b0967cf17137..299fce317eb3 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -2111,6 +2112,31 @@ static noinline_for_stack
>>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>  	      struct printf_spec spec)
>>  {
>> +	/*
>> +	 * If it's an ERR_PTR, try to print its symbolic
>> +	 * representation, except for %px, where the user explicitly
>> +	 * wanted the pointer formatted as a hex value.
>> +	 */
>> +	if (IS_ERR(ptr) && *fmt != 'x') {
> 
> We had similar code before the commit 3e5903eb9cff70730171 ("vsprintf:
> Prevent crash when dereferencing invalid pointers"). Note that the
> original code kept the original value also for *fmt == 'K'.
> 
> Anyway, the above commit tried to unify the handling of special
> values:
> 
>    + use the same strings for special values
>    + check for special values only when pointer is dereferenced
> 
> This patch makes it inconsistent again. I mean that the code will:
> 
>    + check for (null) and (efault) only when the pointer is
>      dereferenced
> 
>    + check for err codes in more situations but not in all
>      and not in %s
>
>    + use another style to print the error (uppercase without
>       brackets)
> 
> 
> I would like to keep it consistent. My proposal is:
> 
> 1. Print the plain error code name only when
>    a new %pe modifier is used. This will be useful
>    in the error messages, e.g.
> 
> 	void *p = ERR_PTR(-ENOMEM);
> 	if (IS_ERR(foo)) {
> 		pr_err("Sorry, can't do that: %pe\n", foo);
> 	return PTR_ERR(foo);
> 
>    would produce
> 
> 	Sorry, can't do that: -ENOMEM

Well, we can certainly do that. However, I didn't want that for two
reasons: (1) I want plain %p to be more useful when passed an ERR_PTR.
Printing the value, possibly symbolically, doesn't leak anything about
kernel addresses, so the hashing is pointless and just makes the
printk() less useful - and non-repeatable across reboots, making
debugging needlessly harder. (2) With a dedicated extension, we have to
define what happens if a non-ERR_PTR gets passed as %pe argument. [and
(3), we're running out of %p<foo> namespace].

So, if you have some good answer to (2) I can do that - but if the
answer is "fall through to handling it as just a normal %p", well, then
we haven't really won much. And I don't see what else we could do -
print "(!ERR_PTR)"?

> 2. Use error code names also in check_pointer_msg() instead
>    of (efault). But put it into brackets to distinguish it
>    from the expected value, for example:
> 
>       /* valid pointer */
>       phys_addr_t addr = 0xab;
>       printk("value: %pa\n", &addr);
>       /* known error code */
>       printk("value: %pa\n", ERR_PTR(-ENOMEM));
>       /* unknown error code */
>       printk("value: %pa\n", ERR_PTR(-1234));
> 
>    would produce:
> 
>      value: 0xab
>      value: (-ENOMEM)
>      value: (-1234)

Yes, I think this is a good idea. But only for ERR_PTRs, for other
obviously-not-a-kernel-address pointer values (i.e. the < PAGE_SIZE
case) we still need some other string.

So how about I try to add something like this so that
would-be-dereferenced ERR_PTRs get printed symbolically in brackets,
while I move the check for IS_ERR() to after the switch() (i.e. before
handing over to do the hashing)? Then all ERR_PTRs get printed
symbolically - except for %px and possibly %pK which are explicitly
"print this value".

> 3. Unify the style for the null pointer:
> 
>     + use (NULL) instead of (null)

Yes, that's better. But somewhat out of scope for this patch.

> 4. Do not use error code names for internal vsprintf error
>    to avoid confusion. For example:
> 
>     + replace the one (einval) with (%piS-err) or so
> 
> How does that sound, please?

Oh, yes, I never was a fan of the (einval) (efault) strings.

Rasmus

Re: [PATCH v3] printf: add support for printing symbolic error codes

[Petr Mladek]

On Sun 2019-09-29 22:09:28, Rasmus Villemoes wrote:
> > On Tue 2019-09-17 08:59:59, Rasmus Villemoes wrote:
> >> With my embedded hat on, I've made it possible to remove this.
> >>
> >> I've tested that the #ifdeffery in errcode.c is sufficient to make
> >> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> >> 0day bot will tell me which ones I've missed.
> > 
> > Please, remove the above two paragraphs in the final patch. They make
> > sense for review but not for git history.
>
> Agree for the latter, but not the former - I do want to explain why it's
> possible to configure out; see also below.

I see. It was too cryptic for me. I did not get the proper meaning
and context ;-)

> > This change would deserve to spend some time in linux-next. Anyway,
> > it is already too late for 5.4.
> 
> Yes, it's of course way too late now. Perhaps I should ask you to take
> it via the printk tree? Anyway, let's see what we can agree to.

Yup, I could take it via printk.git.

> >> +config SYMBOLIC_ERRCODE
> > 
> > What is the exact reason to make this configurable, please?
> 
> I am one such person, and while 3K may not be a lot, death by a thousand
> paper cuts...
> 
> > I would personally enable it when CONFIG_PRINTK is enabled.
> 
> Agree. So let's make it 'default y if PRINTK'?

Yeah, it makes perfect sense to me.

> > We could always introduce a new config option later when
> > anyone really wants to disable it.
> 
> No, because by the time the kernel has grown too large for some target,
> it's almost impossible to start figuring out which small pieces can be
> chopped away with suitable config options

OK, if you are the embedded guy and you would appreciate the config
then let's have it. Just please add it by a separate patch,
ideally with some numbers.

> and even harder to get
> upstream to accept such configurability ("why, that would only gain you
> 3K...").

I remeber LWN articles about shrinking the kernel for embeded systems
and that every kB was important.


> >> --- /dev/null
> >> +++ b/lib/errcode.c
> >> @@ -0,0 +1,212 @@
> >> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> >> +static const char *codes_0[] = {
> >> +	E(E2BIG),
> > 
> > I really like the way how the array is initialized.
> 
> Thanks.
> 
> > 
> >> diff --git a/lib/test_printf.c b/lib/test_printf.c
> >> index 944eb50f3862..0401a2341245 100644
> >> --- a/lib/test_printf.c
> >> +++ b/lib/test_printf.c
> >> +static void __init
> >> +errptr(void)
> >> +{
> >> +	test("-1234", "%p", ERR_PTR(-1234));
> >> +	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
> >> +#ifdef CONFIG_SYMBOLIC_ERRCODE
> >> +	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
> >> +	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
> > 
> > I like that you check more values. But please split it to check
> > only one value per line to make it better readable.
> 
> Hm, ok, but I actually do it this way on purpose - I want to ensure that
> the test where one passes a random not-zero-but-too-small buffer size
> sometimes hits in the middle of the %p output, sometimes just before and
> sometimes just after.

Is this really tested? do_test() function uses buffer for 256 characters.
There are some consistency checks. But any of your test string
is not truncated by the size of the buffer. Do I miss anything, please?

> The very reason I added test_printf was because
> there were some %p extensions that violated the basic rule of
> snprintf()'s return value and/or wrote beyond the provided buffer.

This sounds like a serious bug. Are your aware of any still
existing %p extension that causes this?

> Not a big deal, so if you insist I'll break it up.

Yes, it is not a big deal. But I would still prefer to
understand what is tested. And it is better to have
more tests focused on different aspects than a single
magic one.


> > 
> >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> >> index b0967cf17137..299fce317eb3 100644
> >> --- a/lib/vsprintf.c
> >> +++ b/lib/vsprintf.c
> >> @@ -2111,6 +2112,31 @@ static noinline_for_stack
> >>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >>  	      struct printf_spec spec)
> >>  {
> >> +	/*
> >> +	 * If it's an ERR_PTR, try to print its symbolic
> >> +	 * representation, except for %px, where the user explicitly
> >> +	 * wanted the pointer formatted as a hex value.
> >> +	 */
> >> +	if (IS_ERR(ptr) && *fmt != 'x') {
> > 
> > We had similar code before the commit 3e5903eb9cff70730171 ("vsprintf:
> > Prevent crash when dereferencing invalid pointers"). Note that the
> > original code kept the original value also for *fmt == 'K'.
> > 
> > Anyway, the above commit tried to unify the handling of special
> > values:
> > 
> >    + use the same strings for special values
> >    + check for special values only when pointer is dereferenced
> > 
> > This patch makes it inconsistent again. I mean that the code will:
> > 
> >    + check for (null) and (efault) only when the pointer is
> >      dereferenced
> > 
> >    + check for err codes in more situations but not in all
> >      and not in %s
> >
> >    + use another style to print the error (uppercase without
> >       brackets)
> > 
> > 
> > I would like to keep it consistent. My proposal is:
> > 
> > 1. Print the plain error code name only when
> >    a new %pe modifier is used. This will be useful
> >    in the error messages, e.g.
> > 
> > 	void *p = ERR_PTR(-ENOMEM);
> > 	if (IS_ERR(foo)) {
> > 		pr_err("Sorry, can't do that: %pe\n", foo);
> > 	return PTR_ERR(foo);
> > 
> >    would produce
> > 
> > 	Sorry, can't do that: -ENOMEM
> 
> Well, we can certainly do that. However, I didn't want that for two
> reasons: (1) I want plain %p to be more useful when passed an ERR_PTR.
> Printing the value, possibly symbolically, doesn't leak anything about
> kernel addresses, so the hashing is pointless and just makes the
> printk() less useful - and non-repeatable across reboots, making
> debugging needlessly harder. (2) With a dedicated extension, we have to
> define what happens if a non-ERR_PTR gets passed as %pe argument. [and
> (3), we're running out of %p<foo> namespace].
>
> So, if you have some good answer to (2) I can do that - but if the
> answer is "fall through to handling it as just a normal %p", well, then
> we haven't really won much. And I don't see what else we could do -
> print "(!ERR_PTR)"?

This made me to remember the long discussion about filtering kernel
pointers, see
https://lkml.kernel.org/r/1506816410-10230-1-git-send-email-me@tobin.cc

It was basically about that %p might leak information. %pK failed
because it did not force people to remove the dangerous %p calls.
It ended with hashing %p to actually force people to convert %p
into something more useful and safe.

IMHO, the most extreme was a wish to get rid of %p and %pa
completely, see
https://lkml.kernel.org/r/CA+55aFwac2BzgZs-X1_VhekkuGfuLqNui2+2DbvLiDyptS-rXQ@mail.gmail.com

I do not think that exposing ERR_PTR values might be dangerous.
Well, it would be great to confirm this by some security guys.
Anyway, I do not think that it is a good idea to make %p
more useful again.

I would print the hashed pointer value when the value passed
to %pe is out of range.

Best Regards,
Petr

Re: [PATCH v3] printf: add support for printing symbolic error codes

[Andrew Morton]

On Tue, 17 Sep 2019 08:59:59 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is
> 
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %p\n", foo);
>     return PTR_ERR(foo);
>   }
> 
> it is also more helpful to get a symbolic error code (or, worst case,
> a decimal number) in case an ERR_PTR is accidentally passed to some
> %p<something>, rather than the (efault) that check_pointer() would
> result in.
> 
> With my embedded hat on, I've made it possible to remove this.
> 
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.
> 
> The symbols to include have been found by massaging the output of
> 
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
> 
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.

Looks reasonable to me.

Is there any existing kernel code which presently uses this?  Can we
get some conversions done to demonstrate and hopefully test the
feature?

[PATCH v4 0/1] printf: add support for printing symbolic error names

[Rasmus Villemoes]

This is a bit much for under the ---, so a separate cover letter for
this single patch.

v4: Dropped Uwe's ack since it's changed quite a bit. Change
errcode->errname as suggested by Petr. Make it 'default y if PRINTK'
so it's available in the common case, while those who have gone to
great lengths to shave their kernel to the bare minimum are not
affected.

Also require the caller to use %pe instead of printing all ERR_PTRs
symbolically. I can see some value in having the call site explicitly
indicate that they're printing an ERR_PTR (i.e., having the %pe), but
I also still believe it would make sense to print ordinary %p,
ERR_PTR() symbolically instead of as a random hash value that's not
stable across reboots. But in the interest of getting this in, I'll
leave that for now. It's easy enough to do later by just changing the
"case 'e'" to do a break (with an updated comment), then do an
IS_ERR() check after the switch.

Something I've glossed over in previous versions, and nobody has
commented on, is that I produced "ENOSPC" while the 'fallback' would
print "-28" (i.e., there's no minus in the symbolic case). I don't
care much either way, but here I've tried to show how I'd do it if we
want the minus also in the symbolic case. At first, I tried just using
the standard idiom

  if (buf < end)
    *buf = '-';
  buf++;

followed by string(sym, ...). However, that doesn't work very well if
one wants to honour field width - for that to work, the whole string
including - must come from the errname() lookup and be handled by
string(). The simplest seemed to be to just unconditionally prefix all
strings with "-" when building the tables, and then change errname()
back to supporting both positive and negative error numbers.

As I said, I don't care much either way, so if somebody thinks this is
too complicated and would prefer just printing "ENOSPC" (because
really the minus doesn't offer much except that it's perhaps easier to
recognize for a kernel developer) just speak up.

I've also given some thought to Petr's suggestion of how to improve
the handling of ERR_PTRs that are accidentally passed to
%p<something-that-would-dereference-it>. But I'll do that as a
separate series on top - for now I think this should go into -next if
nobody complains loudly.

Rasmus Villemoes (1):
  printf: add support for printing symbolic error names

 Documentation/core-api/printk-formats.rst |  12 ++
 include/linux/errname.h                   |  16 ++
 lib/Kconfig.debug                         |   9 +
 lib/Makefile                              |   1 +
 lib/errname.c                             | 222 ++++++++++++++++++++++
 lib/test_printf.c                         |  24 +++
 lib/vsprintf.c                            |  27 +++
 7 files changed, 311 insertions(+)
 create mode 100644 include/linux/errname.h
 create mode 100644 lib/errname.c

-- 
2.20.1

[PATCH v4 1/1] printf: add support for printing symbolic error names

[Rasmus Villemoes]

It has been suggested several times to extend vsnprintf() to be able
to convert the numeric value of ENOSPC to print "ENOSPC". This
implements that as a %p extension: With %pe, one can do

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %pe\n", foo);
    return PTR_ERR(foo);
  }

instead of what is seen in quite a few places in the kernel:

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %ld\n", PTR_ERR(foo));
    return PTR_ERR(foo);
  }

If the value passed to %pe is an ERR_PTR, but the library function
errname() added here doesn't know about the value, the value is simply
printed in decimal. If the value passed to %pe is not an ERR_PTR, we
treat it as an ordinary %p and thus print the hashed value (passing
non-ERR_PTR values to %pe indicates a bug in the caller, but we can't
do much about that).

With my embedded hat on, and because it's not very invasive to do,
I've made it possible to remove this. The errname() function and
associated lookup tables take up about 3K. For most, that's probably
quite acceptable and a price worth paying for more readable
dmesg (once this starts getting used), while for those that disable
printk() it's of very little use - I don't see a
procfs/sysfs/seq_printf() file reasonably making use of this - and
they clearly want to squeeze vmlinux as much as possible. Hence the
default y if PRINTK.

The symbols to include have been found by massaging the output of

  find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'

In the cases where some common aliasing exists
(e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
to the bottom so that one takes precedence.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 Documentation/core-api/printk-formats.rst |  12 ++
 include/linux/errname.h                   |  16 ++
 lib/Kconfig.debug                         |   9 +
 lib/Makefile                              |   1 +
 lib/errname.c                             | 222 ++++++++++++++++++++++
 lib/test_printf.c                         |  24 +++
 lib/vsprintf.c                            |  27 +++
 7 files changed, 311 insertions(+)
 create mode 100644 include/linux/errname.h
 create mode 100644 lib/errname.c

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index ecbebf4ca8e7..050f34f3a70f 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -79,6 +79,18 @@ has the added benefit of providing a unique identifier. On 64-bit machines
 the first 32 bits are zeroed. The kernel will print ``(ptrval)`` until it
 gathers enough entropy. If you *really* want the address see %px below.
 
+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.
+
 Symbols/Function Pointers
 -------------------------
 
diff --git a/include/linux/errname.h b/include/linux/errname.h
new file mode 100644
index 000000000000..e8576ad90cb7
--- /dev/null
+++ b/include/linux/errname.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ERRNAME_H
+#define _LINUX_ERRNAME_H
+
+#include <linux/stddef.h>
+
+#ifdef CONFIG_SYMBOLIC_ERRNAME
+const char *errname(int err);
+#else
+static inline const char *errname(int err)
+{
+	return NULL;
+}
+#endif
+
+#endif /* _LINUX_ERRNAME_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 93d97f9b0157..99a6cf677140 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -164,6 +164,15 @@ config DYNAMIC_DEBUG
 	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
 	  information.
 
+config SYMBOLIC_ERRNAME
+	bool "Support symbolic error names in printf"
+	default y if PRINTK
+	help
+	  If you say Y here, the kernel's printf implementation will
+	  be able to print symbolic error names such as ENOSPC instead
+	  of the number 28. It makes the kernel image slightly larger
+	  (about 3KB), but can make the kernel logs easier to read.
+
 endmenu # "printk and dmesg options"
 
 menu "Compile-time checks and compiler options"
diff --git a/lib/Makefile b/lib/Makefile
index c5892807e06f..d740534057ed 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -181,6 +181,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
 obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
 
 obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
+obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
 
 obj-$(CONFIG_NLATTR) += nlattr.o
 
diff --git a/lib/errname.c b/lib/errname.c
new file mode 100644
index 000000000000..30d3bab99477
--- /dev/null
+++ b/lib/errname.c
@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/build_bug.h>
+#include <linux/errno.h>
+#include <linux/errname.h>
+#include <linux/kernel.h>
+
+/*
+ * Ensure these tables do not accidentally become gigantic if some
+ * huge errno makes it in. On most architectures, the first table will
+ * only have about 140 entries, but mips and parisc have more sparsely
+ * 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.
+ */
+#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
+static const char *names_0[] = {
+	E(E2BIG),
+	E(EACCES),
+	E(EADDRINUSE),
+	E(EADDRNOTAVAIL),
+	E(EADV),
+	E(EAFNOSUPPORT),
+	E(EALREADY),
+	E(EBADE),
+	E(EBADF),
+	E(EBADFD),
+	E(EBADMSG),
+	E(EBADR),
+	E(EBADRQC),
+	E(EBADSLT),
+	E(EBFONT),
+	E(EBUSY),
+#ifdef ECANCELLED
+	E(ECANCELLED),
+#endif
+	E(ECHILD),
+	E(ECHRNG),
+	E(ECOMM),
+	E(ECONNABORTED),
+	E(ECONNRESET),
+	E(EDEADLOCK),
+	E(EDESTADDRREQ),
+	E(EDOM),
+	E(EDOTDOT),
+#ifndef CONFIG_MIPS
+	E(EDQUOT),
+#endif
+	E(EEXIST),
+	E(EFAULT),
+	E(EFBIG),
+	E(EHOSTDOWN),
+	E(EHOSTUNREACH),
+	E(EHWPOISON),
+	E(EIDRM),
+	E(EILSEQ),
+#ifdef EINIT
+	E(EINIT),
+#endif
+	E(EINPROGRESS),
+	E(EINTR),
+	E(EINVAL),
+	E(EIO),
+	E(EISCONN),
+	E(EISDIR),
+	E(EISNAM),
+	E(EKEYEXPIRED),
+	E(EKEYREJECTED),
+	E(EKEYREVOKED),
+	E(EL2HLT),
+	E(EL2NSYNC),
+	E(EL3HLT),
+	E(EL3RST),
+	E(ELIBACC),
+	E(ELIBBAD),
+	E(ELIBEXEC),
+	E(ELIBMAX),
+	E(ELIBSCN),
+	E(ELNRNG),
+	E(ELOOP),
+	E(EMEDIUMTYPE),
+	E(EMFILE),
+	E(EMLINK),
+	E(EMSGSIZE),
+	E(EMULTIHOP),
+	E(ENAMETOOLONG),
+	E(ENAVAIL),
+	E(ENETDOWN),
+	E(ENETRESET),
+	E(ENETUNREACH),
+	E(ENFILE),
+	E(ENOANO),
+	E(ENOBUFS),
+	E(ENOCSI),
+	E(ENODATA),
+	E(ENODEV),
+	E(ENOENT),
+	E(ENOEXEC),
+	E(ENOKEY),
+	E(ENOLCK),
+	E(ENOLINK),
+	E(ENOMEDIUM),
+	E(ENOMEM),
+	E(ENOMSG),
+	E(ENONET),
+	E(ENOPKG),
+	E(ENOPROTOOPT),
+	E(ENOSPC),
+	E(ENOSR),
+	E(ENOSTR),
+#ifdef ENOSYM
+	E(ENOSYM),
+#endif
+	E(ENOSYS),
+	E(ENOTBLK),
+	E(ENOTCONN),
+	E(ENOTDIR),
+	E(ENOTEMPTY),
+	E(ENOTNAM),
+	E(ENOTRECOVERABLE),
+	E(ENOTSOCK),
+	E(ENOTTY),
+	E(ENOTUNIQ),
+	E(ENXIO),
+	E(EOPNOTSUPP),
+	E(EOVERFLOW),
+	E(EOWNERDEAD),
+	E(EPERM),
+	E(EPFNOSUPPORT),
+	E(EPIPE),
+#ifdef EPROCLIM
+	E(EPROCLIM),
+#endif
+	E(EPROTO),
+	E(EPROTONOSUPPORT),
+	E(EPROTOTYPE),
+	E(ERANGE),
+	E(EREMCHG),
+#ifdef EREMDEV
+	E(EREMDEV),
+#endif
+	E(EREMOTE),
+	E(EREMOTEIO),
+#ifdef EREMOTERELEASE
+	E(EREMOTERELEASE),
+#endif
+	E(ERESTART),
+	E(ERFKILL),
+	E(EROFS),
+#ifdef ERREMOTE
+	E(ERREMOTE),
+#endif
+	E(ESHUTDOWN),
+	E(ESOCKTNOSUPPORT),
+	E(ESPIPE),
+	E(ESRCH),
+	E(ESRMNT),
+	E(ESTALE),
+	E(ESTRPIPE),
+	E(ETIME),
+	E(ETIMEDOUT),
+	E(ETOOMANYREFS),
+	E(ETXTBSY),
+	E(EUCLEAN),
+	E(EUNATCH),
+	E(EUSERS),
+	E(EXDEV),
+	E(EXFULL),
+
+	E(ECANCELED),
+	E(EAGAIN), /* EWOULDBLOCK */
+	E(ECONNREFUSED), /* EREFUSED */
+	E(EDEADLK), /* EDEADLK */
+};
+#undef E
+
+#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = "-" #err
+static const char *names_512[] = {
+	E(ERESTARTSYS),
+	E(ERESTARTNOINTR),
+	E(ERESTARTNOHAND),
+	E(ENOIOCTLCMD),
+	E(ERESTART_RESTARTBLOCK),
+	E(EPROBE_DEFER),
+	E(EOPENSTALE),
+	E(ENOPARAM),
+
+	E(EBADHANDLE),
+	E(ENOTSYNC),
+	E(EBADCOOKIE),
+	E(ENOTSUPP),
+	E(ETOOSMALL),
+	E(ESERVERFAULT),
+	E(EBADTYPE),
+	E(EJUKEBOX),
+	E(EIOCBQUEUED),
+	E(ERECALLCONFLICT),
+};
+#undef E
+
+static const char *__errname(unsigned err)
+{
+	if (err < ARRAY_SIZE(names_0))
+		return names_0[err];
+	if (err >= 512 && err - 512 < ARRAY_SIZE(names_512))
+		return names_512[err - 512];
+	/* But why? */
+	if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
+		return "EDQUOT";
+	return NULL;
+}
+
+/*
+ * errname(EIO) -> "EIO"
+ * errname(-EIO) -> "-EIO"
+ */
+const char *errname(int err)
+{
+	bool pos = err > 0;
+	const char *name = __errname(err > 0 ? err : -err);
+
+	return name ? name + pos : NULL;
+}
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 5d94cbff2120..4fa0ccf58420 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -593,6 +593,29 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+static void __init
+errptr(void)
+{
+	char buf[PLAIN_BUF_SIZE];
+
+	test("-1234", "%pe", ERR_PTR(-1234));
+
+	/* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */
+	BUILD_BUG_ON(IS_ERR(PTR));
+	snprintf(buf, sizeof(buf), "(%p)", PTR);
+	test(buf, "(%pe)", PTR);
+
+#ifdef CONFIG_SYMBOLIC_ERRNAME
+	test("(-ENOTSOCK)", "(%pe)", ERR_PTR(-ENOTSOCK));
+	test("(-EAGAIN)", "(%pe)", ERR_PTR(-EAGAIN));
+	BUILD_BUG_ON(EAGAIN != EWOULDBLOCK);
+	test("(-EAGAIN)", "(%pe)", ERR_PTR(-EWOULDBLOCK));
+	test("[-EIO    ]", "[%-8pe]", ERR_PTR(-EIO));
+	test("[    -EIO]", "[%8pe]", ERR_PTR(-EIO));
+	test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER));
+#endif
+}
+
 static void __init
 test_pointer(void)
 {
@@ -615,6 +638,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	errptr();
 }
 
 static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e78017a3e1bd..b54d252b398e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -21,6 +21,7 @@
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/errname.h>
 #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
 #include <linux/types.h>
 #include <linux/string.h>
@@ -613,6 +614,25 @@ static char *string_nocheck(char *buf, char *end, const char *s,
 	return widen_string(buf, len, end, spec);
 }
 
+static char *err_ptr(char *buf, char *end, void *ptr,
+		     struct printf_spec spec)
+{
+	int err = PTR_ERR(ptr);
+	const char *sym = errname(err);
+
+	if (sym)
+		return string_nocheck(buf, end, sym, spec);
+
+	/*
+	 * Somebody passed ERR_PTR(-1234) or some other non-existing
+	 * Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to
+	 * printing it as its decimal representation.
+	 */
+	spec.flags |= SIGN;
+	spec.base = 10;
+	return number(buf, end, err, spec);
+}
+
 /* Be careful: error messages must fit into the given buffer. */
 static char *error_string(char *buf, char *end, const char *s,
 			  struct printf_spec spec)
@@ -2187,6 +2207,11 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return kobject_string(buf, end, ptr, spec, fmt);
 	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))
+			break;
+		return err_ptr(buf, end, ptr, spec);
 	}
 
 	/* default is to _not_ leak addresses, hash before printing */
@@ -2823,6 +2848,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args)
 			case 'f':
 			case 'x':
 			case 'K':
+			case 'e':
 				save_arg(void *);
 				break;
 			default:
@@ -2999,6 +3025,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			case 'f':
 			case 'x':
 			case 'K':
+			case 'e':
 				process = true;
 				break;
 			default:
-- 
2.20.1

Re: [PATCH v4 1/1] printf: add support for printing symbolic error names

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

On Fri, Oct 11, 2019 at 03:36:17PM +0200, Rasmus Villemoes wrote:
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This
> implements that as a %p extension: With %pe, one can do
> 
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %pe\n", foo);
>     return PTR_ERR(foo);
>   }
> 
> instead of what is seen in quite a few places in the kernel:
> 
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %ld\n", PTR_ERR(foo));
>     return PTR_ERR(foo);
>   }
> 
> If the value passed to %pe is an ERR_PTR, but the library function
> errname() added here doesn't know about the value, the value is simply
> printed in decimal. If the value passed to %pe is not an ERR_PTR, we
> treat it as an ordinary %p and thus print the hashed value (passing
> non-ERR_PTR values to %pe indicates a bug in the caller, but we can't
> do much about that).
> 
> With my embedded hat on, and because it's not very invasive to do,
> I've made it possible to remove this. The errname() function and
> associated lookup tables take up about 3K. For most, that's probably
> quite acceptable and a price worth paying for more readable
> dmesg (once this starts getting used), while for those that disable
> printk() it's of very little use - I don't see a
> procfs/sysfs/seq_printf() file reasonably making use of this - and
> they clearly want to squeeze vmlinux as much as possible. Hence the
> default y if PRINTK.
> 
> The symbols to include have been found by massaging the output of
> 
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
> 
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

I like having an explicit code even better than relying on a plain %p to
emit the code. So please readd my

Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 1/1] printf: add support for printing symbolic error names

[Petr Mladek]

On Fri 2019-10-11 15:36:17, Rasmus Villemoes wrote:
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This
> implements that as a %p extension: With %pe, one can do

Reviewed-by: Petr Mladek <pmladek@suse.com>

I like the patch. There are only two rather cosmetic things.

> diff --git a/lib/errname.c b/lib/errname.c
> new file mode 100644
> index 000000000000..30d3bab99477
> --- /dev/null
> +++ b/lib/errname.c
> +const char *errname(int err)
> +{
> +	bool pos = err > 0;
> +	const char *name = __errname(err > 0 ? err : -err);
> +
> +	return name ? name + pos : NULL;

This made me to check C standard. It seems that "true" really has
to be "1".

But I think that I am not the only one who is not sure.
I would prefer to make it less tricky and use, for example:

	const char *name = __errname(err > 0 ? err : -err);
	if (!name)
		return NULL;

	return err > 0 ? name + 1 : name;

> +}
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 5d94cbff2120..4fa0ccf58420 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -593,6 +593,29 @@ flags(void)
>  	kfree(cmp_buffer);
>  }
>  
> +static void __init
> +errptr(void)
> +{
> +	char buf[PLAIN_BUF_SIZE];
> +
> +	test("-1234", "%pe", ERR_PTR(-1234));
> +
> +	/* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */
> +	BUILD_BUG_ON(IS_ERR(PTR));
> +	snprintf(buf, sizeof(buf), "(%p)", PTR);
> +	test(buf, "(%pe)", PTR);

There is a small race. "(____ptrval____)" is used for %p before
random numbers are initialized. The switch is done via workqueue
work, see enable_ptr_key_workfn(). It means that it can be done
in parallel.

I doubt that anyone would ever hit the race. But it could be very confusing
and hard to debug. I would replace it with:

	test_hashed("%pe", PTR);


If would like to have the two things fixed. I am not sure if you want
to send one more revision. Or I could also change it by follow
up patch when pushing. What is your preference, please?

Best Regards,
Petr

Re: [PATCH v4 1/1] printf: add support for printing symbolic error names

[Andy Shevchenko]

On Mon, Oct 14, 2019 at 4:02 PM Petr Mladek <pmladek@suse.com> wrote:
> On Fri 2019-10-11 15:36:17, Rasmus Villemoes wrote:
> > It has been suggested several times to extend vsnprintf() to be able
> > to convert the numeric value of ENOSPC to print "ENOSPC". This
> > implements that as a %p extension: With %pe, one can do

> > +const char *errname(int err)
> > +{
> > +     bool pos = err > 0;
> > +     const char *name = __errname(err > 0 ? err : -err);
> > +
> > +     return name ? name + pos : NULL;
>
> This made me to check C standard. It seems that "true" really has
> to be "1".

You may guarantee it by using !!.

     return name ? name + !!(err > 0) : NULL;


But to me it seems like forgotten use of pos in the other case above.

Anyway, I would rather do

abs(err) in the first place

and simple use name + 1 in the second as you suggested with maybe a
comment that we skip E letter.

> But I think that I am not the only one who is not sure.
> I would prefer to make it less tricky and use, for example:
>
>         const char *name = __errname(err > 0 ? err : -err);
>         if (!name)
>                 return NULL;
>
>         return err > 0 ? name + 1 : name;

> > +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/1] printf: add support for printing symbolic error names

[Rasmus Villemoes]

On 14/10/2019 15.02, Petr Mladek wrote:
> On Fri 2019-10-11 15:36:17, Rasmus Villemoes wrote:
>> It has been suggested several times to extend vsnprintf() to be able
>> to convert the numeric value of ENOSPC to print "ENOSPC". This
>> implements that as a %p extension: With %pe, one can do
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> I like the patch. There are only two rather cosmetic things.
> 
>> diff --git a/lib/errname.c b/lib/errname.c
>> new file mode 100644
>> index 000000000000..30d3bab99477
>> --- /dev/null
>> +++ b/lib/errname.c
>> +const char *errname(int err)
>> +{
>> +	bool pos = err > 0;
>> +	const char *name = __errname(err > 0 ? err : -err);
>> +
>> +	return name ? name + pos : NULL;
> 
> This made me to check C standard. It seems that "true" really has
> to be "1".
> 
> But I think that I am not the only one who is not sure.
> I would prefer to make it less tricky and use, for example:
> 
> 	const char *name = __errname(err > 0 ? err : -err);
> 	if (!name)
> 		return NULL;
> 
> 	return err > 0 ? name + 1 : name;

I didn't even stop to think that using the value of a comparison
operator/bool in arithmetic might be the littlest bit surprising for C
programmers. But your suggestion isn't terrible, so go ahead and write
it that way. And can I get you to fix the missing "-" in the MIPS
"EDQUOT" special case while you're at it?

>> +static void __init
>> +errptr(void)
>> +{
>> +	char buf[PLAIN_BUF_SIZE];
>> +
>> +	test("-1234", "%pe", ERR_PTR(-1234));
>> +
>> +	/* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */
>> +	BUILD_BUG_ON(IS_ERR(PTR));
>> +	snprintf(buf, sizeof(buf), "(%p)", PTR);
>> +	test(buf, "(%pe)", PTR);
> 
> There is a small race. "(____ptrval____)" is used for %p before
> random numbers are initialized. The switch is done via workqueue
> work, see enable_ptr_key_workfn(). It means that it can be done
> in parallel.

I know.

> I doubt that anyone would ever hit the race. But it could be very confusing
> and hard to debug.

I thought about it and decided not to care, as long as the errptr test
comes after the hashing test, because if the hashing is not initialized
then one gets a warning. I also considered setting a flag in that case
and then skipping the errptr hash test, but again, decided that the
warning would be enough.

> I would replace it with:

> 	test_hashed("%pe", PTR);

Sure, that will repeat the warning, but it doesn't seem to prevent a
false positive: Between plain_hash_to_buffer emitting the warning (and
returning 0) and the caller test_hashed() then performing the test()
against the buffer contents, the hash can become initialized and thus
change how %p[e] gets formatted. But ok, perhaps it is cleaner to reuse
test_hashed and avoid the local buffer in errptr. So yeah, I suppose
this on top is fine:

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 4fa0ccf58420..030daeb4fe21 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -596,14 +596,11 @@ flags(void)
 static void __init
 errptr(void)
 {
-       char buf[PLAIN_BUF_SIZE];
-
        test("-1234", "%pe", ERR_PTR(-1234));

        /* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */
        BUILD_BUG_ON(IS_ERR(PTR));
-       snprintf(buf, sizeof(buf), "(%p)", PTR);
-       test(buf, "(%pe)", PTR);
+       test_hashed("%pe", PTR);

 #ifdef CONFIG_SYMBOLIC_ERRNAME
        test("(-ENOTSOCK)", "(%pe)", ERR_PTR(-ENOTSOCK));



> If would like to have the two things fixed. I am not sure if you want
> to send one more revision. Or I could also change it by follow
> up patch when pushing.

I prefer you to fold in both changes instead of an extra patch, and if
you can't, I'll send a new revision.

Rasmus

RE: [PATCH v4 1/1] printf: add support for printing symbolic error names

[David Laight]

From: Petr Mladek
> Sent: 14 October 2019 14:03
> On Fri 2019-10-11 15:36:17, Rasmus Villemoes wrote:
> > It has been suggested several times to extend vsnprintf() to be able
> > to convert the numeric value of ENOSPC to print "ENOSPC". This
> > implements that as a %p extension: With %pe, one can do
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> I like the patch. There are only two rather cosmetic things.
> 
> > diff --git a/lib/errname.c b/lib/errname.c
> > new file mode 100644
> > index 000000000000..30d3bab99477
> > --- /dev/null
> > +++ b/lib/errname.c
> > +const char *errname(int err)
> > +{
> > +	bool pos = err > 0;
> > +	const char *name = __errname(err > 0 ? err : -err);
> > +
> > +	return name ? name + pos : NULL;
> 
> This made me to check C standard. It seems that "true" really has
> to be "1".

What is thus "true" symbol you are talking about?
Actually you could get rid of the "bool" as well.
	unsigned int pos = err > 0;
	const char *name = __errname(pos ? err : -err);
	return name ? &pos[name] : NULL;
then it is all well defined.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH v5] printf: add support for printing symbolic error names

[Rasmus Villemoes]

It has been suggested several times to extend vsnprintf() to be able
to convert the numeric value of ENOSPC to print "ENOSPC". This
implements that as a %p extension: With %pe, one can do

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %pe\n", foo);
    return PTR_ERR(foo);
  }

instead of what is seen in quite a few places in the kernel:

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %ld\n", PTR_ERR(foo));
    return PTR_ERR(foo);
  }

If the value passed to %pe is an ERR_PTR, but the library function
errname() added here doesn't know about the value, the value is simply
printed in decimal. If the value passed to %pe is not an ERR_PTR, we
treat it as an ordinary %p and thus print the hashed value (passing
non-ERR_PTR values to %pe indicates a bug in the caller, but we can't
do much about that).

With my embedded hat on, and because it's not very invasive to do,
I've made it possible to remove this. The errname() function and
associated lookup tables take up about 3K. For most, that's probably
quite acceptable and a price worth paying for more readable
dmesg (once this starts getting used), while for those that disable
printk() it's of very little use - I don't see a
procfs/sysfs/seq_printf() file reasonably making use of this - and
they clearly want to squeeze vmlinux as much as possible. Hence the
default y if PRINTK.

The symbols to include have been found by massaging the output of

  find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'

In the cases where some common aliasing exists
(e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
to the bottom so that one takes precedence.

Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Found a few more things, so sending new revision anyway.

v5: Fix cosmetic issues per Petr. Fix missing "-" in "EDQUOT". Fix a
few comments on at the end of the names_0 array. Add A-b Uwe, R-b Petr.

 Documentation/core-api/printk-formats.rst |  12 ++
 include/linux/errname.h                   |  16 ++
 lib/Kconfig.debug                         |   9 +
 lib/Makefile                              |   1 +
 lib/errname.c                             | 223 ++++++++++++++++++++++
 lib/test_printf.c                         |  21 ++
 lib/vsprintf.c                            |  27 +++
 7 files changed, 309 insertions(+)
 create mode 100644 include/linux/errname.h
 create mode 100644 lib/errname.c

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index ecbebf4ca8e7..050f34f3a70f 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -79,6 +79,18 @@ has the added benefit of providing a unique identifier. On 64-bit machines
 the first 32 bits are zeroed. The kernel will print ``(ptrval)`` until it
 gathers enough entropy. If you *really* want the address see %px below.
 
+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.
+
 Symbols/Function Pointers
 -------------------------
 
diff --git a/include/linux/errname.h b/include/linux/errname.h
new file mode 100644
index 000000000000..e8576ad90cb7
--- /dev/null
+++ b/include/linux/errname.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ERRNAME_H
+#define _LINUX_ERRNAME_H
+
+#include <linux/stddef.h>
+
+#ifdef CONFIG_SYMBOLIC_ERRNAME
+const char *errname(int err);
+#else
+static inline const char *errname(int err)
+{
+	return NULL;
+}
+#endif
+
+#endif /* _LINUX_ERRNAME_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 93d97f9b0157..99a6cf677140 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -164,6 +164,15 @@ config DYNAMIC_DEBUG
 	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
 	  information.
 
+config SYMBOLIC_ERRNAME
+	bool "Support symbolic error names in printf"
+	default y if PRINTK
+	help
+	  If you say Y here, the kernel's printf implementation will
+	  be able to print symbolic error names such as ENOSPC instead
+	  of the number 28. It makes the kernel image slightly larger
+	  (about 3KB), but can make the kernel logs easier to read.
+
 endmenu # "printk and dmesg options"
 
 menu "Compile-time checks and compiler options"
diff --git a/lib/Makefile b/lib/Makefile
index c5892807e06f..d740534057ed 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -181,6 +181,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
 obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
 
 obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
+obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
 
 obj-$(CONFIG_NLATTR) += nlattr.o
 
diff --git a/lib/errname.c b/lib/errname.c
new file mode 100644
index 000000000000..b67f58e29729
--- /dev/null
+++ b/lib/errname.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/build_bug.h>
+#include <linux/errno.h>
+#include <linux/errname.h>
+#include <linux/kernel.h>
+
+/*
+ * Ensure these tables do not accidentally become gigantic if some
+ * huge errno makes it in. On most architectures, the first table will
+ * only have about 140 entries, but mips and parisc have more sparsely
+ * 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.
+ */
+#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
+static const char *names_0[] = {
+	E(E2BIG),
+	E(EACCES),
+	E(EADDRINUSE),
+	E(EADDRNOTAVAIL),
+	E(EADV),
+	E(EAFNOSUPPORT),
+	E(EALREADY),
+	E(EBADE),
+	E(EBADF),
+	E(EBADFD),
+	E(EBADMSG),
+	E(EBADR),
+	E(EBADRQC),
+	E(EBADSLT),
+	E(EBFONT),
+	E(EBUSY),
+#ifdef ECANCELLED
+	E(ECANCELLED),
+#endif
+	E(ECHILD),
+	E(ECHRNG),
+	E(ECOMM),
+	E(ECONNABORTED),
+	E(ECONNRESET),
+	E(EDEADLOCK),
+	E(EDESTADDRREQ),
+	E(EDOM),
+	E(EDOTDOT),
+#ifndef CONFIG_MIPS
+	E(EDQUOT),
+#endif
+	E(EEXIST),
+	E(EFAULT),
+	E(EFBIG),
+	E(EHOSTDOWN),
+	E(EHOSTUNREACH),
+	E(EHWPOISON),
+	E(EIDRM),
+	E(EILSEQ),
+#ifdef EINIT
+	E(EINIT),
+#endif
+	E(EINPROGRESS),
+	E(EINTR),
+	E(EINVAL),
+	E(EIO),
+	E(EISCONN),
+	E(EISDIR),
+	E(EISNAM),
+	E(EKEYEXPIRED),
+	E(EKEYREJECTED),
+	E(EKEYREVOKED),
+	E(EL2HLT),
+	E(EL2NSYNC),
+	E(EL3HLT),
+	E(EL3RST),
+	E(ELIBACC),
+	E(ELIBBAD),
+	E(ELIBEXEC),
+	E(ELIBMAX),
+	E(ELIBSCN),
+	E(ELNRNG),
+	E(ELOOP),
+	E(EMEDIUMTYPE),
+	E(EMFILE),
+	E(EMLINK),
+	E(EMSGSIZE),
+	E(EMULTIHOP),
+	E(ENAMETOOLONG),
+	E(ENAVAIL),
+	E(ENETDOWN),
+	E(ENETRESET),
+	E(ENETUNREACH),
+	E(ENFILE),
+	E(ENOANO),
+	E(ENOBUFS),
+	E(ENOCSI),
+	E(ENODATA),
+	E(ENODEV),
+	E(ENOENT),
+	E(ENOEXEC),
+	E(ENOKEY),
+	E(ENOLCK),
+	E(ENOLINK),
+	E(ENOMEDIUM),
+	E(ENOMEM),
+	E(ENOMSG),
+	E(ENONET),
+	E(ENOPKG),
+	E(ENOPROTOOPT),
+	E(ENOSPC),
+	E(ENOSR),
+	E(ENOSTR),
+#ifdef ENOSYM
+	E(ENOSYM),
+#endif
+	E(ENOSYS),
+	E(ENOTBLK),
+	E(ENOTCONN),
+	E(ENOTDIR),
+	E(ENOTEMPTY),
+	E(ENOTNAM),
+	E(ENOTRECOVERABLE),
+	E(ENOTSOCK),
+	E(ENOTTY),
+	E(ENOTUNIQ),
+	E(ENXIO),
+	E(EOPNOTSUPP),
+	E(EOVERFLOW),
+	E(EOWNERDEAD),
+	E(EPERM),
+	E(EPFNOSUPPORT),
+	E(EPIPE),
+#ifdef EPROCLIM
+	E(EPROCLIM),
+#endif
+	E(EPROTO),
+	E(EPROTONOSUPPORT),
+	E(EPROTOTYPE),
+	E(ERANGE),
+	E(EREMCHG),
+#ifdef EREMDEV
+	E(EREMDEV),
+#endif
+	E(EREMOTE),
+	E(EREMOTEIO),
+#ifdef EREMOTERELEASE
+	E(EREMOTERELEASE),
+#endif
+	E(ERESTART),
+	E(ERFKILL),
+	E(EROFS),
+#ifdef ERREMOTE
+	E(ERREMOTE),
+#endif
+	E(ESHUTDOWN),
+	E(ESOCKTNOSUPPORT),
+	E(ESPIPE),
+	E(ESRCH),
+	E(ESRMNT),
+	E(ESTALE),
+	E(ESTRPIPE),
+	E(ETIME),
+	E(ETIMEDOUT),
+	E(ETOOMANYREFS),
+	E(ETXTBSY),
+	E(EUCLEAN),
+	E(EUNATCH),
+	E(EUSERS),
+	E(EXDEV),
+	E(EXFULL),
+
+	E(ECANCELED), /* ECANCELLED */
+	E(EAGAIN), /* EWOULDBLOCK */
+	E(ECONNREFUSED), /* EREFUSED */
+	E(EDEADLK), /* EDEADLOCK */
+};
+#undef E
+
+#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = "-" #err
+static const char *names_512[] = {
+	E(ERESTARTSYS),
+	E(ERESTARTNOINTR),
+	E(ERESTARTNOHAND),
+	E(ENOIOCTLCMD),
+	E(ERESTART_RESTARTBLOCK),
+	E(EPROBE_DEFER),
+	E(EOPENSTALE),
+	E(ENOPARAM),
+
+	E(EBADHANDLE),
+	E(ENOTSYNC),
+	E(EBADCOOKIE),
+	E(ENOTSUPP),
+	E(ETOOSMALL),
+	E(ESERVERFAULT),
+	E(EBADTYPE),
+	E(EJUKEBOX),
+	E(EIOCBQUEUED),
+	E(ERECALLCONFLICT),
+};
+#undef E
+
+static const char *__errname(unsigned err)
+{
+	if (err < ARRAY_SIZE(names_0))
+		return names_0[err];
+	if (err >= 512 && err - 512 < ARRAY_SIZE(names_512))
+		return names_512[err - 512];
+	/* But why? */
+	if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
+		return "-EDQUOT";
+	return NULL;
+}
+
+/*
+ * errname(EIO) -> "EIO"
+ * errname(-EIO) -> "-EIO"
+ */
+const char *errname(int err)
+{
+	const char *name = __errname(err > 0 ? err : -err);
+	if (!name)
+		return NULL;
+
+	return err > 0 ? name + 1 : name;
+}
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 5d94cbff2120..030daeb4fe21 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -593,6 +593,26 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+static void __init
+errptr(void)
+{
+	test("-1234", "%pe", ERR_PTR(-1234));
+
+	/* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */
+	BUILD_BUG_ON(IS_ERR(PTR));
+	test_hashed("%pe", PTR);
+
+#ifdef CONFIG_SYMBOLIC_ERRNAME
+	test("(-ENOTSOCK)", "(%pe)", ERR_PTR(-ENOTSOCK));
+	test("(-EAGAIN)", "(%pe)", ERR_PTR(-EAGAIN));
+	BUILD_BUG_ON(EAGAIN != EWOULDBLOCK);
+	test("(-EAGAIN)", "(%pe)", ERR_PTR(-EWOULDBLOCK));
+	test("[-EIO    ]", "[%-8pe]", ERR_PTR(-EIO));
+	test("[    -EIO]", "[%8pe]", ERR_PTR(-EIO));
+	test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER));
+#endif
+}
+
 static void __init
 test_pointer(void)
 {
@@ -615,6 +635,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	errptr();
 }
 
 static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e78017a3e1bd..b54d252b398e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -21,6 +21,7 @@
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/errname.h>
 #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
 #include <linux/types.h>
 #include <linux/string.h>
@@ -613,6 +614,25 @@ static char *string_nocheck(char *buf, char *end, const char *s,
 	return widen_string(buf, len, end, spec);
 }
 
+static char *err_ptr(char *buf, char *end, void *ptr,
+		     struct printf_spec spec)
+{
+	int err = PTR_ERR(ptr);
+	const char *sym = errname(err);
+
+	if (sym)
+		return string_nocheck(buf, end, sym, spec);
+
+	/*
+	 * Somebody passed ERR_PTR(-1234) or some other non-existing
+	 * Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to
+	 * printing it as its decimal representation.
+	 */
+	spec.flags |= SIGN;
+	spec.base = 10;
+	return number(buf, end, err, spec);
+}
+
 /* Be careful: error messages must fit into the given buffer. */
 static char *error_string(char *buf, char *end, const char *s,
 			  struct printf_spec spec)
@@ -2187,6 +2207,11 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return kobject_string(buf, end, ptr, spec, fmt);
 	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))
+			break;
+		return err_ptr(buf, end, ptr, spec);
 	}
 
 	/* default is to _not_ leak addresses, hash before printing */
@@ -2823,6 +2848,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args)
 			case 'f':
 			case 'x':
 			case 'K':
+			case 'e':
 				save_arg(void *);
 				break;
 			default:
@@ -2999,6 +3025,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			case 'f':
 			case 'x':
 			case 'K':
+			case 'e':
 				process = true;
 				break;
 			default:
-- 
2.20.1

Re: [PATCH v5] printf: add support for printing symbolic error names

[Andy Shevchenko]

On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:

> +const char *errname(int err)
> +{
> +       const char *name = __errname(err > 0 ? err : -err);

Looks like mine comment left unseen.
What about to simple use abs(err) here?

> +       if (!name)
> +               return NULL;
> +
> +       return err > 0 ? name + 1 : name;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5] printf: add support for printing symbolic error names

[Petr Mladek]

On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote:
> On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> 
> > +const char *errname(int err)
> > +{
> > +       const char *name = __errname(err > 0 ? err : -err);
> 
> Looks like mine comment left unseen.
> What about to simple use abs(err) here?

Andy, would you want to ack the patch with this change?
I could do it when pushing the patch.

Otherwise, it looks ready to go.

Thanks everybody involved for the patience.

Best Regards,
Petr

Re: [PATCH v5] printf: add support for printing symbolic error names

[Andy Shevchenko]

On Wed, Oct 16, 2019 at 5:52 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote:
> > On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> >
> > > +const char *errname(int err)
> > > +{
> > > +       const char *name = __errname(err > 0 ? err : -err);
> >
> > Looks like mine comment left unseen.
> > What about to simple use abs(err) here?
>
> Andy, would you want to ack the patch with this change?
> I could do it when pushing the patch.

Looks good to me.
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Otherwise, it looks ready to go.
>
> Thanks everybody involved for the patience.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5] printf: add support for printing symbolic error names

[Petr Mladek]

On Wed 2019-10-16 19:31:33, Andy Shevchenko wrote:
> On Wed, Oct 16, 2019 at 5:52 PM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote:
> > > On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes
> > > <linux@rasmusvillemoes.dk> wrote:
> > >
> > > > +const char *errname(int err)
> > > > +{
> > > > +       const char *name = __errname(err > 0 ? err : -err);
> > >
> > > Looks like mine comment left unseen.
> > > What about to simple use abs(err) here?
> >
> > Andy, would you want to ack the patch with this change?
> > I could do it when pushing the patch.
> 
> Looks good to me.
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

The patch has been committed into printk.git, branch for-5.5.

Best Regards,
Petr

Re: [PATCH v4 0/1] printf: add support for printing symbolic error names

[Geert Uytterhoeven]

Hi Rasmus,

Nice idea!

On Fri, Oct 11, 2019 at 3:38 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> This is a bit much for under the ---, so a separate cover letter for
> this single patch.
>
> v4: Dropped Uwe's ack since it's changed quite a bit. Change
> errcode->errname as suggested by Petr. Make it 'default y if PRINTK'
> so it's available in the common case, while those who have gone to
> great lengths to shave their kernel to the bare minimum are not
> affected.
>
> Also require the caller to use %pe instead of printing all ERR_PTRs
> symbolically. I can see some value in having the call site explicitly
> indicate that they're printing an ERR_PTR (i.e., having the %pe), but
> I also still believe it would make sense to print ordinary %p,
> ERR_PTR() symbolically instead of as a random hash value that's not
> stable across reboots. But in the interest of getting this in, I'll
> leave that for now. It's easy enough to do later by just changing the
> "case 'e'" to do a break (with an updated comment), then do an
> IS_ERR() check after the switch.
>
> Something I've glossed over in previous versions, and nobody has
> commented on, is that I produced "ENOSPC" while the 'fallback' would
> print "-28" (i.e., there's no minus in the symbolic case). I don't
> care much either way, but here I've tried to show how I'd do it if we
> want the minus also in the symbolic case. At first, I tried just using
> the standard idiom
>
>   if (buf < end)
>     *buf = '-';
>   buf++;
>
> followed by string(sym, ...). However, that doesn't work very well if
> one wants to honour field width - for that to work, the whole string
> including - must come from the errname() lookup and be handled by
> string(). The simplest seemed to be to just unconditionally prefix all
> strings with "-" when building the tables, and then change errname()
> back to supporting both positive and negative error numbers.

Still, it looks a bit wasteful to me to include the dash in each and every
string value.

Do you think you can code the +/- logic in string_nocheck() in less than
the gain achieved by dropping the dashes from the tables?
(e.g. by using the SIGN spec.flags? ;-)
Or, do we need it? IS_ERR() doesn't consider positive values errors.

Oh, what about the leading "E"? That one looks harder to get rid of,
though ;-)

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 0/1] printf: add support for printing symbolic error names

[Petr Mladek]

On Tue 2019-11-26 15:04:06, Geert Uytterhoeven wrote:
> Hi Rasmus,
> 
> Nice idea!
> 
> On Fri, Oct 11, 2019 at 3:38 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> > This is a bit much for under the ---, so a separate cover letter for
> > this single patch.
> >
> > v4: Dropped Uwe's ack since it's changed quite a bit. Change
> > errcode->errname as suggested by Petr. Make it 'default y if PRINTK'
> > so it's available in the common case, while those who have gone to
> > great lengths to shave their kernel to the bare minimum are not
> > affected.
> >
> > Also require the caller to use %pe instead of printing all ERR_PTRs
> > symbolically. I can see some value in having the call site explicitly
> > indicate that they're printing an ERR_PTR (i.e., having the %pe), but
> > I also still believe it would make sense to print ordinary %p,
> > ERR_PTR() symbolically instead of as a random hash value that's not
> > stable across reboots. But in the interest of getting this in, I'll
> > leave that for now. It's easy enough to do later by just changing the
> > "case 'e'" to do a break (with an updated comment), then do an
> > IS_ERR() check after the switch.
> >
> > Something I've glossed over in previous versions, and nobody has
> > commented on, is that I produced "ENOSPC" while the 'fallback' would
> > print "-28" (i.e., there's no minus in the symbolic case). I don't
> > care much either way, but here I've tried to show how I'd do it if we
> > want the minus also in the symbolic case. At first, I tried just using
> > the standard idiom
> >
> >   if (buf < end)
> >     *buf = '-';
> >   buf++;
> >
> > followed by string(sym, ...). However, that doesn't work very well if
> > one wants to honour field width - for that to work, the whole string
> > including - must come from the errname() lookup and be handled by
> > string(). The simplest seemed to be to just unconditionally prefix all
> > strings with "-" when building the tables, and then change errname()
> > back to supporting both positive and negative error numbers.
> 
> Still, it looks a bit wasteful to me to include the dash in each and every
> string value.
> 
> Do you think you can code the +/- logic in string_nocheck() in less than
> the gain achieved by dropping the dashes from the tables?
> (e.g. by using the SIGN spec.flags? ;-)
> Or, do we need it? IS_ERR() doesn't consider positive values errors.
> 
> Oh, what about the leading "E"? That one looks harder to get rid of,
> though ;-)

It would be nice. But too big hack is not worth it. Anybody who cares
about saving 0.2kB would likely disable this feature completely.

Feel to provide a patch so that we could see how good/bad it is.

Best Regards,
Petr