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 <[email protected]>
---
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
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?
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
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?
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
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 <[email protected]>
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
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)
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 <[email protected]>
Apart from the above minor issues:
Acked-by: Uwe Kleine-König <[email protected]>
Best regards
Uwe
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 <[email protected]>
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
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 <[email protected]>
>
> 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
On Sat, Aug 31, 2019 at 12:48 AM Rasmus Villemoes
<[email protected]> 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
On 9/4/19 6:19 PM, Andy Shevchenko wrote:
> On Sat, Aug 31, 2019 at 12:48 AM Rasmus Villemoes
> <[email protected]> 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
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
>> <[email protected]> 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
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 <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
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
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
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 <[email protected]>
> Signed-off-by: Rasmus Villemoes <[email protected]>
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
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 <[email protected]>
>>> Signed-off-by: Rasmus Villemoes <[email protected]>
>>
>> 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
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 <[email protected]>
>> Signed-off-by: Rasmus Villemoes <[email protected]>
>
> 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
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 <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
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
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
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
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/[email protected]
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
On Tue, 17 Sep 2019 08:59:59 +0200 Rasmus Villemoes <[email protected]> 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?
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
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 <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
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
On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes
<[email protected]> 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
On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote:
> On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes
> <[email protected]> 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
On Wed, Oct 16, 2019 at 5:52 PM Petr Mladek <[email protected]> wrote:
>
> On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote:
> > On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes
> > <[email protected]> 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 <[email protected]>
>
> Otherwise, it looks ready to go.
>
> Thanks everybody involved for the patience.
--
With Best Regards,
Andy Shevchenko
On Wed 2019-10-16 19:31:33, Andy Shevchenko wrote:
> On Wed, Oct 16, 2019 at 5:52 PM Petr Mladek <[email protected]> wrote:
> >
> > On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote:
> > > On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes
> > > <[email protected]> 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 <[email protected]>
The patch has been committed into printk.git, branch for-5.5.
Best Regards,
Petr
Hi Rasmus,
Nice idea!
On Fri, Oct 11, 2019 at 3:38 PM Rasmus Villemoes
<[email protected]> 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 -- [email protected]
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
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
> <[email protected]> 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