[PATCH v1 1/2] vsprintf: introduce %dE for error constants

[PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

	pr_info("probing failed (%dE)\n", ret);

expands to

	probing failed (EIO)

if ret holds -EIO (or EIO). This introduces an array of error codes. If
the error code is missing, %dE falls back to %d and so prints the plain
number.

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

there are many code sites that benefit from this. Just grep for
"(%d)" ...

As an example the follow up patch converts a printk to use this new
format escape.

Best regards
Uwe

 Documentation/core-api/printk-formats.rst |   3 +
 lib/vsprintf.c                            | 193 +++++++++++++++++++++-
 2 files changed, 195 insertions(+), 1 deletion(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcb..81002414f956 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -35,6 +35,9 @@ Integer types
 		u64			%llu or %llx
 
 
+To print the name that corresponds to an integer error constant, use %dE and
+pass the int.
+
 If <type> is dependent on a config option for its size (e.g., sector_t,
 blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
 format specifier of its largest possible type and explicitly cast to it.
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..672eab8dab84 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
 	return buf;
 }
 
+#define ERRORCODE(x) { .str = #x, .err = x }
+
+static const struct {
+	const char *str;
+	int err;
+} errorcodes[] = {
+	ERRORCODE(EPERM),
+	ERRORCODE(ENOENT),
+	ERRORCODE(ESRCH),
+	ERRORCODE(EINTR),
+	ERRORCODE(EIO),
+	ERRORCODE(ENXIO),
+	ERRORCODE(E2BIG),
+	ERRORCODE(ENOEXEC),
+	ERRORCODE(EBADF),
+	ERRORCODE(ECHILD),
+	ERRORCODE(EAGAIN),
+	ERRORCODE(ENOMEM),
+	ERRORCODE(EACCES),
+	ERRORCODE(EFAULT),
+	ERRORCODE(ENOTBLK),
+	ERRORCODE(EBUSY),
+	ERRORCODE(EEXIST),
+	ERRORCODE(EXDEV),
+	ERRORCODE(ENODEV),
+	ERRORCODE(ENOTDIR),
+	ERRORCODE(EISDIR),
+	ERRORCODE(EINVAL),
+	ERRORCODE(ENFILE),
+	ERRORCODE(EMFILE),
+	ERRORCODE(ENOTTY),
+	ERRORCODE(ETXTBSY),
+	ERRORCODE(EFBIG),
+	ERRORCODE(ENOSPC),
+	ERRORCODE(ESPIPE),
+	ERRORCODE(EROFS),
+	ERRORCODE(EMLINK),
+	ERRORCODE(EPIPE),
+	ERRORCODE(EDOM),
+	ERRORCODE(ERANGE),
+	ERRORCODE(EDEADLK),
+	ERRORCODE(ENAMETOOLONG),
+	ERRORCODE(ENOLCK),
+	ERRORCODE(ENOSYS),
+	ERRORCODE(ENOTEMPTY),
+	ERRORCODE(ELOOP),
+	ERRORCODE(EWOULDBLOCK),
+	ERRORCODE(ENOMSG),
+	ERRORCODE(EIDRM),
+	ERRORCODE(ECHRNG),
+	ERRORCODE(EL2NSYNC),
+	ERRORCODE(EL3HLT),
+	ERRORCODE(EL3RST),
+	ERRORCODE(ELNRNG),
+	ERRORCODE(EUNATCH),
+	ERRORCODE(ENOCSI),
+	ERRORCODE(EL2HLT),
+	ERRORCODE(EBADE),
+	ERRORCODE(EBADR),
+	ERRORCODE(EXFULL),
+	ERRORCODE(ENOANO),
+	ERRORCODE(EBADRQC),
+	ERRORCODE(EBADSLT),
+	ERRORCODE(EBFONT),
+	ERRORCODE(ENOSTR),
+	ERRORCODE(ENODATA),
+	ERRORCODE(ETIME),
+	ERRORCODE(ENOSR),
+	ERRORCODE(ENONET),
+	ERRORCODE(ENOPKG),
+	ERRORCODE(EREMOTE),
+	ERRORCODE(ENOLINK),
+	ERRORCODE(EADV),
+	ERRORCODE(ESRMNT),
+	ERRORCODE(ECOMM),
+	ERRORCODE(EPROTO),
+	ERRORCODE(EMULTIHOP),
+	ERRORCODE(EDOTDOT),
+	ERRORCODE(EBADMSG),
+	ERRORCODE(EOVERFLOW),
+	ERRORCODE(ENOTUNIQ),
+	ERRORCODE(EBADFD),
+	ERRORCODE(EREMCHG),
+	ERRORCODE(ELIBACC),
+	ERRORCODE(ELIBBAD),
+	ERRORCODE(ELIBSCN),
+	ERRORCODE(ELIBMAX),
+	ERRORCODE(ELIBEXEC),
+	ERRORCODE(EILSEQ),
+	ERRORCODE(ERESTART),
+	ERRORCODE(ESTRPIPE),
+	ERRORCODE(EUSERS),
+	ERRORCODE(ENOTSOCK),
+	ERRORCODE(EDESTADDRREQ),
+	ERRORCODE(EMSGSIZE),
+	ERRORCODE(EPROTOTYPE),
+	ERRORCODE(ENOPROTOOPT),
+	ERRORCODE(EPROTONOSUPPORT),
+	ERRORCODE(ESOCKTNOSUPPORT),
+	ERRORCODE(EOPNOTSUPP),
+	ERRORCODE(EPFNOSUPPORT),
+	ERRORCODE(EAFNOSUPPORT),
+	ERRORCODE(EADDRINUSE),
+	ERRORCODE(EADDRNOTAVAIL),
+	ERRORCODE(ENETDOWN),
+	ERRORCODE(ENETUNREACH),
+	ERRORCODE(ENETRESET),
+	ERRORCODE(ECONNABORTED),
+	ERRORCODE(ECONNRESET),
+	ERRORCODE(ENOBUFS),
+	ERRORCODE(EISCONN),
+	ERRORCODE(ENOTCONN),
+	ERRORCODE(ESHUTDOWN),
+	ERRORCODE(ETOOMANYREFS),
+	ERRORCODE(ETIMEDOUT),
+	ERRORCODE(ECONNREFUSED),
+	ERRORCODE(EHOSTDOWN),
+	ERRORCODE(EHOSTUNREACH),
+	ERRORCODE(EALREADY),
+	ERRORCODE(EINPROGRESS),
+	ERRORCODE(ESTALE),
+	ERRORCODE(EUCLEAN),
+	ERRORCODE(ENOTNAM),
+	ERRORCODE(ENAVAIL),
+	ERRORCODE(EISNAM),
+	ERRORCODE(EREMOTEIO),
+	ERRORCODE(EDQUOT),
+	ERRORCODE(ENOMEDIUM),
+	ERRORCODE(EMEDIUMTYPE),
+	ERRORCODE(ECANCELED),
+	ERRORCODE(ENOKEY),
+	ERRORCODE(EKEYEXPIRED),
+	ERRORCODE(EKEYREVOKED),
+	ERRORCODE(EKEYREJECTED),
+	ERRORCODE(EOWNERDEAD),
+	ERRORCODE(ENOTRECOVERABLE),
+	ERRORCODE(ERFKILL),
+	ERRORCODE(EHWPOISON),
+	ERRORCODE(ERESTARTSYS),
+	ERRORCODE(ERESTARTNOINTR),
+	ERRORCODE(ERESTARTNOHAND),
+	ERRORCODE(ENOIOCTLCMD),
+	ERRORCODE(ERESTART_RESTARTBLOCK),
+	ERRORCODE(EPROBE_DEFER),
+	ERRORCODE(EOPENSTALE),
+	ERRORCODE(ENOPARAM),
+	ERRORCODE(EBADHANDLE),
+	ERRORCODE(ENOTSYNC),
+	ERRORCODE(EBADCOOKIE),
+	ERRORCODE(ENOTSUPP),
+	ERRORCODE(ETOOSMALL),
+	ERRORCODE(ESERVERFAULT),
+	ERRORCODE(EBADTYPE),
+	ERRORCODE(EJUKEBOX),
+	ERRORCODE(EIOCBQUEUED),
+	ERRORCODE(ERECALLCONFLICT),
+};
+
+static noinline_for_stack
+char *errstr(char *buf, char *end, unsigned long long num,
+	     struct printf_spec spec)
+{
+	char *errname = NULL;
+	size_t errnamelen, copy;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
+		if (num == errorcodes[i].err || num == -errorcodes[i].err) {
+			errname = errorcodes[i].str;
+			break;
+		}
+	}
+
+	if (!errname) {
+		/* fall back to ordinary number */
+		return number(buf, end, num, spec);
+	}
+
+	copy = errnamelen = strlen(errname);
+	if (copy > end - buf)
+		copy = end - buf;
+	buf = memcpy(buf, errname, copy);
+
+	return buf + errnamelen;
+}
+
 static noinline_for_stack
 char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
 {
@@ -2566,7 +2752,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				num = va_arg(args, unsigned int);
 			}
 
-			str = number(str, end, num, spec);
+			if (spec.type == FORMAT_TYPE_INT && *fmt == 'E') {
+				fmt++;
+				str = errstr(str, end, num, spec);
+			} else {
+				str = number(str, end, num, spec);
+			}
 		}
 	}
 
-- 
2.20.1

[PATCH v1 2/2] gpiolib: print an error name instead of a plain number in error string

[Uwe Kleine-König]

This is an example that makes use of the just introduced printk format
%dE that prints (e.g.) "EIO" when the matching integer is -EIO (or EIO).

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f497003f119c..b50ea24f087f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1247,7 +1247,7 @@ static void gpiochip_setup_devs(void)
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		err = gpiochip_setup_dev(gdev);
 		if (err)
-			pr_err("%s: Failed to initialize gpio device (%d)\n",
+			pr_err("%s: Failed to initialize gpio device (%dE)\n",
 			       dev_name(&gdev->dev), err);
 	}
 }
-- 
2.20.1

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Andrew Morton]

(cc printk maintainers).

On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> wrote:

> 	pr_info("probing failed (%dE)\n", ret);
> 
> expands to
> 
> 	probing failed (EIO)
> 
> if ret holds -EIO (or EIO). This introduces an array of error codes. If
> the error code is missing, %dE falls back to %d and so prints the plain
> number.

Huh.  I'm surprised we don't already have this.  Seems that this will
be applicable in a lot of places?  Although we shouldn't go blindly
converting everything in sight - that would risk breaking userspace
which parses kernel strings.

Is it really necessary to handle the positive errnos?  Does much kernel
code actually do that (apart from kernel code which is buggy)?

> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello
> 
> there are many code sites that benefit from this. Just grep for
> "(%d)" ...

Yup.  This observation shouldn't be below the "^---$" ;) An approximate
grep|wc would be interesting.

> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
>  	return buf;
>  }
>  
> +#define ERRORCODE(x) { .str = #x, .err = x }
> +
> +static const struct {
> +	const char *str;
> +	int err;
> +} errorcodes[] = {

It's a bit of a hack, but an array of char*'s and a separate array of
ushorts would save a bit of space.

> +	ERRORCODE(EPERM),
> +	ERRORCODE(ENOENT),
> +	ERRORCODE(ESRCH),
>
> ...
>
> +static noinline_for_stack

Why this?  I'm suspecting this will actually increase stack use?

> +char *errstr(char *buf, char *end, unsigned long long num,
> +	     struct printf_spec spec)
> +{
> +	char *errname = NULL;
> +	size_t errnamelen, copy;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> +		if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> +			errname = errorcodes[i].str;
> +			break;
> +		}
> +	}
> +
> +	if (!errname) {
> +		/* fall back to ordinary number */
> +		return number(buf, end, num, spec);
> +	}
> +
> +	copy = errnamelen = strlen(errname);
> +	if (copy > end - buf)
> +		copy = end - buf;
> +	buf = memcpy(buf, errname, copy);
> +
> +	return buf + errnamelen;
> +}

OK, I guess `errstr' is an OK name for a static function and we can use
this to add a new strerror() should the need arise.

>
> ...
>

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

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

Hello Andrew,

On Sat, Aug 24, 2019 at 04:58:29PM -0700, Andrew Morton wrote:
> (cc printk maintainers).

Ah, I wasn't aware there is something like them. Thanks

> On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> 
> > 	pr_info("probing failed (%dE)\n", ret);
> > 
> > expands to
> > 
> > 	probing failed (EIO)
> > 
> > if ret holds -EIO (or EIO). This introduces an array of error codes. If
> > the error code is missing, %dE falls back to %d and so prints the plain
> > number.
> 
> Huh.  I'm surprised we don't already have this.  Seems that this will
> be applicable in a lot of places?  Although we shouldn't go blindly
> converting everything in sight - that would risk breaking userspace
> which parses kernel strings.

Uah, even the kernel log is API? But I agree so far that this is merge
window material and shouldn't make it into v5.3 :-)

> Is it really necessary to handle the positive errnos?  Does much kernel
> code actually do that (apart from kernel code which is buggy)?

I didn't check; probably not. But the whole positive range seems so
unused and interpreting EIO (and not only -EIO) as "EIO" seems straight
forward. But I don't feel strong either way.

> > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > ---
> > Hello
> > 
> > there are many code sites that benefit from this. Just grep for
> > "(%d)" ...
> 
> Yup.  This observation shouldn't be below the "^---$" ;) An approximate
> grep|wc would be interesting.

I didn't check how many false positives there are using "(%d)", but I'd
use an a bit more elaborate expression for the commit log:

	$ git grep '(%d)' | wc -l
	7336
	$ git grep -E '^\s*(printk|(kv?as|sn?|vs(c?n)?)printf|(kvm|dev|pr)_(emerg|alert|crit|err|warn(ing)?|notice|info|cont|devel|debug|dbg))\(.*(\(%d\)|: %d)\\n' | wc -l
	9140

The latter matches several printk-variants emitting a string ending in
one of

	'(%d)\n' (1839 matches)
	': %d\n' (7301 matches)

. I would expect that many of the 7336 matches for '(%d)' that are not
matched by the longer expression are false negatives because the
function name is in the previous line. So I would estimate around 10k
strings that could benefit from %dE.

> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
> >  	return buf;
> >  }
> >  
> > +#define ERRORCODE(x) { .str = #x, .err = x }
> > +
> > +static const struct {
> > +	const char *str;
> > +	int err;
> > +} errorcodes[] = {
> 
> It's a bit of a hack, but an array of char*'s and a separate array of
> ushorts would save a bit of space.

Hmm, true. Currently we have 150 entries taking 150 * (sizeof(void *) *
2). Is it worth to think about the hacky solution to go down to 150 *
(sizeof(void *) + 2)?

For an ARM build bloat-o-meter reports (comparing linus/master to linus/master
+ my patch):

	add/remove: 2/0 grow/shrink: 4/2 up/down: 1488/-8 (1480)
	Function                                     old     new   delta
	errorcodes                                     -    1200   +1200
	errstr                                         -     200    +200
	vsnprintf                                    884     960     +76
	set_precision                                148     152      +4
	resource_string                             1380    1384      +4
	flags_string                                 400     404      +4
	num_to_str                                   288     284      -4
	format_decode                               1024    1020      -4
	Total: Before=21686, After=23166, chg +6.82%

But that doesn't seem to include the size increase for all the added
strings which seems to be around another 1300 bytes.

> > +	ERRORCODE(EPERM),
> > +	ERRORCODE(ENOENT),
> > +	ERRORCODE(ESRCH),
> >
> > ...
> >
> > +static noinline_for_stack
> 
> Why this?  I'm suspecting this will actually increase stack use?

I don't know what it does, just copied it from number() which is used
similarly.
 
> > +char *errstr(char *buf, char *end, unsigned long long num,
> > +	     struct printf_spec spec)
> > +{
> > +	char *errname = NULL;

I missed a warning during my tests, there is a const missing in this
line.

> > +	size_t errnamelen, copy;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> > +		if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> > +			errname = errorcodes[i].str;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!errname) {
> > +		/* fall back to ordinary number */
> > +		return number(buf, end, num, spec);
> > +	}
> > +
> > +	copy = errnamelen = strlen(errname);
> > +	if (copy > end - buf)
> > +		copy = end - buf;
> > +	buf = memcpy(buf, errname, copy);
> > +
> > +	return buf + errnamelen;
> > +}
> 
> OK, I guess `errstr' is an OK name for a static function

IMHO the name is very generic (which is bad), but it is in good company,
as there is also pointer() and number().

> and we can use this to add a new strerror() should the need arise.

In userspace the purpose of strerror is different. It would yield "I/O
error" not "EIO". So strerror using this array would only be a "strerror
light".

In my first prototype I even used %m instead of %dE, but as %m (in
glibc) doesn't consume an argument and produces the more verbose
variant, I changed my mind and went for %dE. (Also my patch has the
undocumented side effect that you can use %ldE if the error is held by a
long int. I didn't test that though.) 

Best regards
Uwe

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

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Sergey Senozhatsky]

On (08/24/19 16:58), Andrew Morton wrote:
> On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> 
> > 	pr_info("probing failed (%dE)\n", ret);
> > 
> > expands to
> > 
> > 	probing failed (EIO)
> > 
> > if ret holds -EIO (or EIO). This introduces an array of error codes. If
> > the error code is missing, %dE falls back to %d and so prints the plain
> > number.
> 
> Huh.  I'm surprised we don't already have this.  Seems that this will
> be applicable in a lot of places?  Although we shouldn't go blindly
> converting everything in sight - that would risk breaking userspace
> which parses kernel strings.
> 
> Is it really necessary to handle the positive errnos?  Does much kernel
> code actually do that (apart from kernel code which is buggy)?

Good point.
POSIX functions on error usually return -1 (negative value) and set errno
(positive value). Positive errno value can be passed to strerror() or
strerror_r() that decode that value and return a human readable
representation. E.g. strerr(9) returns "Bad file descriptor".

We don't have errno. Instead, and I may be wrong on this, kernel functions
are expected to return negative error codes. A very quick grep shows that
there are, however, patterns like "return positive errno".
E.g. drivers/xen/xenbus/xenbus_xs.c: get_error()

		return EINVAL;

But this EINVAL eventually becomes negative

	err = get_error(ret);
	return ERR_PTR(-err);
	
or net/bluetooth/lib.c: bt_to_errno(). But, once again, bt_to_errno()
return value eventually becomes negative:

	err = -bt_to_errno(hdev->req_result);

So errstr() probably can handle only negative values. And, may be,
I'd rename errstr() to strerror(); just because there is a well known
function, which "translates" errnos.

Unlike strerror(), errstr() just returns a macro name. Example:
	"Request failed: EJUKEBOX"

EJUKEBOX does not tell me anything. A quick way to find out what does
EJUKEBOX stand for is to grep include/linux/errno.h

#define EJUKEBOX  528  /* Request initiated, but will not complete before timeout */

One still has to grep; either for 528 or for EJUKEBOX. I think that it
might be simpler, however, to grep for EJUKEBOX, because one can grep
the source code immediately, while in case of 528 one has to map 528
to the corresponding macro first and then grep the source code for EJUKEBOX.
Overall %dE looks interesting.

	-ss

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Jani Nikula]

On Sat, 24 Aug 2019, Andrew Morton <akpm@linux-foundation.org> wrote:
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
>>  	return buf;
>>  }
>>  
>> +#define ERRORCODE(x) { .str = #x, .err = x }
>> +
>> +static const struct {
>> +	const char *str;
>> +	int err;
>> +} errorcodes[] = {
>
> It's a bit of a hack, but an array of char*'s and a separate array of
> ushorts would save a bit of space.

Or just

#define ERRORCODE(x) [x] = #x

static const char * const errorcodes[] = {
	ERRORCODE(EPERM),
        ERRORCODE(ENOENT),
        ...
};

Saves space, faster lookup, discovers at build time why EWOULDBLOCK
would always show up as EAGAIN in the logs. We don't have holes to speak
of in the error codes.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Jani Nikula]

On Mon, 26 Aug 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Sat, 24 Aug 2019, Andrew Morton <akpm@linux-foundation.org> wrote:
>>> --- a/lib/vsprintf.c
>>> +++ b/lib/vsprintf.c
>>> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
>>>  	return buf;
>>>  }
>>>  
>>> +#define ERRORCODE(x) { .str = #x, .err = x }
>>> +
>>> +static const struct {
>>> +	const char *str;
>>> +	int err;
>>> +} errorcodes[] = {
>>
>> It's a bit of a hack, but an array of char*'s and a separate array of
>> ushorts would save a bit of space.
>
> Or just
>
> #define ERRORCODE(x) [x] = #x
>
> static const char * const errorcodes[] = {
> 	ERRORCODE(EPERM),
>         ERRORCODE(ENOENT),
>         ...
> };
>
> Saves space, faster lookup, discovers at build time why EWOULDBLOCK
> would always show up as EAGAIN in the logs. We don't have holes to speak
> of in the error codes.

Meh, failed to notice the range ERESTARTSYS..ERECALLCONFLICT. Other than
that, it's nicer. ;)

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Rasmus Villemoes]

On 25/08/2019 01.37, Uwe Kleine-König wrote:
> 	pr_info("probing failed (%dE)\n", ret);
> 
> expands to
> 
> 	probing failed (EIO)
> 
> if ret holds -EIO (or EIO). This introduces an array of error codes. If
> the error code is missing, %dE falls back to %d and so prints the plain
> number.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello
> 
> there are many code sites that benefit from this. Just grep for
> "(%d)" ...
> 
> As an example the follow up patch converts a printk to use this new
> format escape.
> 
> Best regards
> Uwe
> 
>  Documentation/core-api/printk-formats.rst |   3 +
>  lib/vsprintf.c                            | 193 +++++++++++++++++++++-
>  2 files changed, 195 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index c6224d039bcb..81002414f956 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -35,6 +35,9 @@ Integer types
>  		u64			%llu or %llx
>  
>  
> +To print the name that corresponds to an integer error constant, use %dE and
> +pass the int.
> +
>  If <type> is dependent on a config option for its size (e.g., sector_t,
>  blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
>  format specifier of its largest possible type and explicitly cast to it.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137..672eab8dab84 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
>  	return buf;
>  }
>  
> +#define ERRORCODE(x) { .str = #x, .err = x }
> +
> +static const struct {
> +	const char *str;
> +	int err;
> +} errorcodes[] = {
> +	ERRORCODE(EPERM),
...
> +	ERRORCODE(ERECALLCONFLICT),
> +};
> +
> +static noinline_for_stack
> +char *errstr(char *buf, char *end, unsigned long long num,
> +	     struct printf_spec spec)
> +{
> +	char *errname = NULL;
> +	size_t errnamelen, copy;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> +		if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> +			errname = errorcodes[i].str;
> +			break;
> +		}
> +	}

Not sure I'm a fan of iterating this array. Yes, the errno values are a
bit sparse, but maybe one could use a dense array with O(1) lookup for
those < 128 and then have an exceptional table like yours for the rest.
But if you do keep this whole array thing, please do as Andrew suggested
and compact it somewhat (4 bytes per entry plus the storage for the
strings themselves is enough, see e.g. e1f0bce3), and put EINVAL and
other common things near the start (at least EINVAL is a lot more common
than ENOEXEC).

> +	if (!errname) {
> +		/* fall back to ordinary number */
> +		return number(buf, end, num, spec);
> +	}
> +
> +	copy = errnamelen = strlen(errname);
> +	if (copy > end - buf)
> +		copy = end - buf;
> +	buf = memcpy(buf, errname, copy);

This is buggy, AFAICT. buf may be beyond end (that's the convention), so
end-buf (which is a ptrdiff_t, which is probably a signed type, but it
gets converted to a size_t when compared to copy) can be a huge number,
so copy>end-buf is false.

Please just use the string() helper that gets used for printing other
fixed strings (as well as %s input).

And add tests to lib/test_printf.c, that would catch this sort of thing
immediately.

> +	return buf + errnamelen;
> +}
> +
>  static noinline_for_stack
>  char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
>  {
> @@ -2566,7 +2752,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  				num = va_arg(args, unsigned int);
>  			}
>  
> -			str = number(str, end, num, spec);
> +			if (spec.type == FORMAT_TYPE_INT && *fmt == 'E') {
> +				fmt++;
> +				str = errstr(str, end, num, spec);

drivers/staging/speakup/speakup_bns.c has a %dE, have you checked
whether you're breaking that one? It's hard to grep for all the
variations, %-0*.5dE is also legal and would be caught here.

This has previously been suggested as a %p extension, and I think users
would just as often have an ERR_PTR() as a plain int or long. Since we
already have %p[alphanumeric] as a special kernel thing, why not do
that? It's more convenient to convert from ints/longs to error pointers

  pr_err("Uh-oh: %pE", ERR_PTR(ret));

than the other way around

  pr_err("Uh-oh: %dE", PTR_ERR(p)); // oops, must be %ldE

Kernel size impact? Have you considered making it possible to opt-out
and have %dE just mean %d?

Rasmus

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Petr Mladek]

On Sun 2019-08-25 11:14:42, Uwe Kleine-König  wrote:
> Hello Andrew,
> 
> On Sat, Aug 24, 2019 at 04:58:29PM -0700, Andrew Morton wrote:
> > (cc printk maintainers).
> 
> Ah, I wasn't aware there is something like them. Thanks
> 
> > On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > 
> > > 	pr_info("probing failed (%dE)\n", ret);
> > > 
> > > expands to
> > > 
> > > 	probing failed (EIO)
> > > 
> > > if ret holds -EIO (or EIO). This introduces an array of error codes. If
> > > the error code is missing, %dE falls back to %d and so prints the plain
> > > number.

What was the motivation for this patch, please?

Did it look like a good idea?
Did anyone got tired by searching for the error codes many
times a day?
Did the idea came from a developer, support, or user, please?

> 	add/remove: 2/0 grow/shrink: 4/2 up/down: 1488/-8 (1480)
> 	Function                                     old     new   delta
> 	errorcodes                                     -    1200   +1200
> 	errstr                                         -     200    +200
> 	vsnprintf                                    884     960     +76
> 	set_precision                                148     152      +4
> 	resource_string                             1380    1384      +4
> 	flags_string                                 400     404      +4
> 	num_to_str                                   288     284      -4
> 	format_decode                               1024    1020      -4
> 	Total: Before=21686, After=23166, chg +6.82%
> 
> But that doesn't seem to include the size increase for all the added
> strings which seems to be around another 1300 bytes.

This non-trivial increase of the size and the table still
includes only part of the error codes.

The array is long, created by cpu&paste, the index of each code
is not obvious.

There are ideas to make the code even more tricky to reduce
the size, keep it fast.

Both, %dE modifier and the output format (ECODE) is non-standard.

Upper letters gain a lot of attention. But the error code is
only helper information. Also many error codes are misleading because
they are used either wrongly or there was no better available.

There is no proof that this approach would be widely acceptable for
subsystem maintainers. Some might not like mass and "blind" code
changes. Some might not like the output at all.

I am not persuaded that all this is worth it. Also I do not like
the non-standard solution.

Best Regards,
Petr

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Enrico Weigelt, metux IT consult]

On 25.08.19 01:37, Uwe Kleine-König wrote:

Hi,

> +static noinline_for_stack > +char *errstr(char *buf, char *end, unsigned long long num,> +	 
struct printf_spec spec)> +{
#1: why not putting that into some separate strerror() lib function ?
     This is something I've been looking for quite some time (actually
     already hacked it up somewhere, sometime, but forgotten ...)

#2: why not just having a big case statement and leave the actual lookup
     logic to the compiler ? IMHO, could be written in a very compact way
     by some macro magic

> +	for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) { > +		if (num == errorcodes[i].err || num == -errorcodes[i].err) {

why not taking the abs value only once, instead of duplicate comp on
each iteration ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[Andy Shevchenko]

On Sun, Aug 25, 2019 at 2:40 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
>         pr_info("probing failed (%dE)\n", ret);
>
> expands to
>
>         probing failed (EIO)
>
> if ret holds -EIO (or EIO). This introduces an array of error codes. If
> the error code is missing, %dE falls back to %d and so prints the plain
> number.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello
>
> there are many code sites that benefit from this. Just grep for
> "(%d)" ...
>
> As an example the follow up patch converts a printk to use this new
> format escape.
>

Let's not do this!

We have already a lot of pain with pointer extension, but here is just
a misleading stuff.
Besides above, how you print (float) number out of kernel in sysfs in
very well standard format?
Please, use %p<SMTH> instead.

> Best regards
> Uwe
>
>  Documentation/core-api/printk-formats.rst |   3 +
>  lib/vsprintf.c                            | 193 +++++++++++++++++++++-
>  2 files changed, 195 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index c6224d039bcb..81002414f956 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -35,6 +35,9 @@ Integer types
>                 u64                     %llu or %llx
>
>
> +To print the name that corresponds to an integer error constant, use %dE and
> +pass the int.
> +
>  If <type> is dependent on a config option for its size (e.g., sector_t,
>  blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
>  format specifier of its largest possible type and explicitly cast to it.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137..672eab8dab84 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
>         return buf;
>  }
>
> +#define ERRORCODE(x) { .str = #x, .err = x }
> +
> +static const struct {
> +       const char *str;
> +       int err;
> +} errorcodes[] = {
> +       ERRORCODE(EPERM),
> +       ERRORCODE(ENOENT),
> +       ERRORCODE(ESRCH),
> +       ERRORCODE(EINTR),
> +       ERRORCODE(EIO),
> +       ERRORCODE(ENXIO),
> +       ERRORCODE(E2BIG),
> +       ERRORCODE(ENOEXEC),
> +       ERRORCODE(EBADF),
> +       ERRORCODE(ECHILD),
> +       ERRORCODE(EAGAIN),
> +       ERRORCODE(ENOMEM),
> +       ERRORCODE(EACCES),
> +       ERRORCODE(EFAULT),
> +       ERRORCODE(ENOTBLK),
> +       ERRORCODE(EBUSY),
> +       ERRORCODE(EEXIST),
> +       ERRORCODE(EXDEV),
> +       ERRORCODE(ENODEV),
> +       ERRORCODE(ENOTDIR),
> +       ERRORCODE(EISDIR),
> +       ERRORCODE(EINVAL),
> +       ERRORCODE(ENFILE),
> +       ERRORCODE(EMFILE),
> +       ERRORCODE(ENOTTY),
> +       ERRORCODE(ETXTBSY),
> +       ERRORCODE(EFBIG),
> +       ERRORCODE(ENOSPC),
> +       ERRORCODE(ESPIPE),
> +       ERRORCODE(EROFS),
> +       ERRORCODE(EMLINK),
> +       ERRORCODE(EPIPE),
> +       ERRORCODE(EDOM),
> +       ERRORCODE(ERANGE),
> +       ERRORCODE(EDEADLK),
> +       ERRORCODE(ENAMETOOLONG),
> +       ERRORCODE(ENOLCK),
> +       ERRORCODE(ENOSYS),
> +       ERRORCODE(ENOTEMPTY),
> +       ERRORCODE(ELOOP),
> +       ERRORCODE(EWOULDBLOCK),
> +       ERRORCODE(ENOMSG),
> +       ERRORCODE(EIDRM),
> +       ERRORCODE(ECHRNG),
> +       ERRORCODE(EL2NSYNC),
> +       ERRORCODE(EL3HLT),
> +       ERRORCODE(EL3RST),
> +       ERRORCODE(ELNRNG),
> +       ERRORCODE(EUNATCH),
> +       ERRORCODE(ENOCSI),
> +       ERRORCODE(EL2HLT),
> +       ERRORCODE(EBADE),
> +       ERRORCODE(EBADR),
> +       ERRORCODE(EXFULL),
> +       ERRORCODE(ENOANO),
> +       ERRORCODE(EBADRQC),
> +       ERRORCODE(EBADSLT),
> +       ERRORCODE(EBFONT),
> +       ERRORCODE(ENOSTR),
> +       ERRORCODE(ENODATA),
> +       ERRORCODE(ETIME),
> +       ERRORCODE(ENOSR),
> +       ERRORCODE(ENONET),
> +       ERRORCODE(ENOPKG),
> +       ERRORCODE(EREMOTE),
> +       ERRORCODE(ENOLINK),
> +       ERRORCODE(EADV),
> +       ERRORCODE(ESRMNT),
> +       ERRORCODE(ECOMM),
> +       ERRORCODE(EPROTO),
> +       ERRORCODE(EMULTIHOP),
> +       ERRORCODE(EDOTDOT),
> +       ERRORCODE(EBADMSG),
> +       ERRORCODE(EOVERFLOW),
> +       ERRORCODE(ENOTUNIQ),
> +       ERRORCODE(EBADFD),
> +       ERRORCODE(EREMCHG),
> +       ERRORCODE(ELIBACC),
> +       ERRORCODE(ELIBBAD),
> +       ERRORCODE(ELIBSCN),
> +       ERRORCODE(ELIBMAX),
> +       ERRORCODE(ELIBEXEC),
> +       ERRORCODE(EILSEQ),
> +       ERRORCODE(ERESTART),
> +       ERRORCODE(ESTRPIPE),
> +       ERRORCODE(EUSERS),
> +       ERRORCODE(ENOTSOCK),
> +       ERRORCODE(EDESTADDRREQ),
> +       ERRORCODE(EMSGSIZE),
> +       ERRORCODE(EPROTOTYPE),
> +       ERRORCODE(ENOPROTOOPT),
> +       ERRORCODE(EPROTONOSUPPORT),
> +       ERRORCODE(ESOCKTNOSUPPORT),
> +       ERRORCODE(EOPNOTSUPP),
> +       ERRORCODE(EPFNOSUPPORT),
> +       ERRORCODE(EAFNOSUPPORT),
> +       ERRORCODE(EADDRINUSE),
> +       ERRORCODE(EADDRNOTAVAIL),
> +       ERRORCODE(ENETDOWN),
> +       ERRORCODE(ENETUNREACH),
> +       ERRORCODE(ENETRESET),
> +       ERRORCODE(ECONNABORTED),
> +       ERRORCODE(ECONNRESET),
> +       ERRORCODE(ENOBUFS),
> +       ERRORCODE(EISCONN),
> +       ERRORCODE(ENOTCONN),
> +       ERRORCODE(ESHUTDOWN),
> +       ERRORCODE(ETOOMANYREFS),
> +       ERRORCODE(ETIMEDOUT),
> +       ERRORCODE(ECONNREFUSED),
> +       ERRORCODE(EHOSTDOWN),
> +       ERRORCODE(EHOSTUNREACH),
> +       ERRORCODE(EALREADY),
> +       ERRORCODE(EINPROGRESS),
> +       ERRORCODE(ESTALE),
> +       ERRORCODE(EUCLEAN),
> +       ERRORCODE(ENOTNAM),
> +       ERRORCODE(ENAVAIL),
> +       ERRORCODE(EISNAM),
> +       ERRORCODE(EREMOTEIO),
> +       ERRORCODE(EDQUOT),
> +       ERRORCODE(ENOMEDIUM),
> +       ERRORCODE(EMEDIUMTYPE),
> +       ERRORCODE(ECANCELED),
> +       ERRORCODE(ENOKEY),
> +       ERRORCODE(EKEYEXPIRED),
> +       ERRORCODE(EKEYREVOKED),
> +       ERRORCODE(EKEYREJECTED),
> +       ERRORCODE(EOWNERDEAD),
> +       ERRORCODE(ENOTRECOVERABLE),
> +       ERRORCODE(ERFKILL),
> +       ERRORCODE(EHWPOISON),
> +       ERRORCODE(ERESTARTSYS),
> +       ERRORCODE(ERESTARTNOINTR),
> +       ERRORCODE(ERESTARTNOHAND),
> +       ERRORCODE(ENOIOCTLCMD),
> +       ERRORCODE(ERESTART_RESTARTBLOCK),
> +       ERRORCODE(EPROBE_DEFER),
> +       ERRORCODE(EOPENSTALE),
> +       ERRORCODE(ENOPARAM),
> +       ERRORCODE(EBADHANDLE),
> +       ERRORCODE(ENOTSYNC),
> +       ERRORCODE(EBADCOOKIE),
> +       ERRORCODE(ENOTSUPP),
> +       ERRORCODE(ETOOSMALL),
> +       ERRORCODE(ESERVERFAULT),
> +       ERRORCODE(EBADTYPE),
> +       ERRORCODE(EJUKEBOX),
> +       ERRORCODE(EIOCBQUEUED),
> +       ERRORCODE(ERECALLCONFLICT),
> +};
> +
> +static noinline_for_stack
> +char *errstr(char *buf, char *end, unsigned long long num,
> +            struct printf_spec spec)
> +{
> +       char *errname = NULL;
> +       size_t errnamelen, copy;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> +               if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> +                       errname = errorcodes[i].str;
> +                       break;
> +               }
> +       }
> +
> +       if (!errname) {
> +               /* fall back to ordinary number */
> +               return number(buf, end, num, spec);
> +       }
> +
> +       copy = errnamelen = strlen(errname);
> +       if (copy > end - buf)
> +               copy = end - buf;
> +       buf = memcpy(buf, errname, copy);
> +
> +       return buf + errnamelen;
> +}
> +
>  static noinline_for_stack
>  char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
>  {
> @@ -2566,7 +2752,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>                                 num = va_arg(args, unsigned int);
>                         }
>
> -                       str = number(str, end, num, spec);
> +                       if (spec.type == FORMAT_TYPE_INT && *fmt == 'E') {
> +                               fmt++;
> +                               str = errstr(str, end, num, spec);
> +                       } else {
> +                               str = number(str, end, num, spec);
> +                       }
>                 }
>         }
>
> --
> 2.20.1
>


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

[David Laight]

From: Enrico Weigelt, metux IT consult
> Sent: 26 August 2019 14:29
> On 25.08.19 01:37, Uwe Kleine-König wrote:
> 
> Hi,
> 
> > +static noinline_for_stack > +char *errstr(char *buf, char *end, unsigned long long num,> +
> struct printf_spec spec)> +{
> #1: why not putting that into some separate strerror() lib function ?
>      This is something I've been looking for quite some time (actually
>      already hacked it up somewhere, sometime, but forgotten ...)
> 
> #2: why not just having a big case statement and leave the actual lookup
>      logic to the compiler ? IMHO, could be written in a very compact way
>      by some macro magic

And generate an enormous amount of code and long chains of mispredicted branches.

Is it also worth looking at how long the strings are.
If they can be truncated to 16 bytes then char[][16] will generate
much better code than the array of pointers.

OTOH I'm not really sure it is all a good idea.

	David

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