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

[Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>]

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