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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> ---
> 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.

>
> ...
>