Re: [PATCH v2 08/11] lib/vsprintf: allow range of prefix for %pl[From[To]]

[Rasmus Villemoes]

On Thu, Jan 14 2016, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> The user may supply the range of desired prefixes in a format %pl[From[To]],
> where 'From' is letter of minimum allowed prefix, and 'To' is a maximum
> correspondently.
>

Sorry, but yuck. I really don't think a %p extension is a good way to
achieve this pretty-printing (with or without the min/max unit thing
added). Also, this makes it too easy to lose information (since %plX
would round down to X), so we'd neither get something representing the
passed-in value nor something human readable (as your test case { .v =
1097364144125ULL, .r = "1071644671 KiB" } shows).

>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  Documentation/printk-formats.txt |  4 ++++
>  lib/test_printf.c                | 35 ++++++++++++++++++++++++++++++++---
>  lib/vsprintf.c                   | 18 +++++++++++++++++-
>  3 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 8e8b4c0..3b41899 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -289,9 +289,13 @@ struct clk:
>  
>  unsigned long long value in human-readable form (with IEC binary prefix):
>  
> +	%pl[From[To]]
> +
>  	%pl		1023 KiB	(1047552)
>  	%pl		1048575 B	(1048575)
> +	%plKM		1023 KiB	(1048575)
>  	%pl		1 MiB		(1048576)
> +	%plG		2 TiB		(2199023255552)
>  
>  	For printing given unsigned long long value in human-readable form with
>  	automatically choosen IEC binary prefix.
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 1e078c2..ae35885 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -420,10 +420,39 @@ human_readable(void)
>  		{ .v = 1048575ULL,		.r = "1048575 B" },
>  		{ .v = 1047552ULL,		.r = "1023 KiB" },
>  	};
> -	unsigned int i;
> +	struct hr_test_data htdplM[] = {
> +		{ .v = 1097364144128ULL,	.r = "1022 GiB" },
> +		{ .v = 1097364144125ULL,	.r = "1046527 MiB" },
> +		{ .v = 1048576ULL,		.r = "1 MiB" },
> +		{ .v = 1048575ULL,		.r = "0 MiB" },
> +		{ .v = 1047552ULL,		.r = "0 MiB" },
> +	};
> +	struct hr_test_data htdplKM[] = {
> +		{ .v = 1097364144128ULL,	.r = "1046528 MiB" },
> +		{ .v = 1097364144125ULL,	.r = "1071644671 KiB" },
> +		{ .v = 1048578ULL,		.r = "1024 KiB" },
> +		{ .v = 1048576ULL,		.r = "1 MiB" },
> +		{ .v = 1048575ULL,		.r = "1023 KiB" },
> +		{ .v = 1047552ULL,		.r = "1023 KiB" },
> +	};
> +	struct hr_test_array {
> +		const char *fmt;
> +		struct hr_test_data *data;
> +		size_t sz;
> +	};
> +	struct hr_test_array hta[] = {
> +		{ "%pl",	htdpl,		ARRAY_SIZE(htdpl) },
> +		{ "%plM",	htdplM,		ARRAY_SIZE(htdplM) },
> +		{ "%plKM",	htdplKM,	ARRAY_SIZE(htdplKM) },
> +		/* No reversed %pl[To[From]] order */
> +		{ "%plMK",	htdplM,		ARRAY_SIZE(htdplM) },
> +	};
> +	unsigned int i, j;
>  
> -	for (i = 0; i < ARRAY_SIZE(htdpl); i++)
> -		test(htdpl[i].r, "%pl", &htdpl[i].v);
> +	for (j = 0; j < ARRAY_SIZE(hta); j++) {
> +		for (i = 0; i < hta[j].sz; i++)
> +			test(hta[j].data[i].r, hta[j].fmt, &hta[j].data[i].v);
> +	}
>  }
>  
>  static void __init
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 8288470..fbcb221 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1310,12 +1310,28 @@ char *human_readable(char *buf, char *end, const void *addr,
>  		     struct printf_spec spec, const char *fmt)
>  {
>  	unsigned long long value = *(unsigned long long *)addr;
> +	unsigned short from = 0, to = STRING_UNITS_2_NUM - 1;
>  	unsigned long index;
> +	unsigned int i;
> +

Is there any reason you try to use every integer rank? from, to, index
and i never take values other than [0..9], right? So unsigned int
(or u8 if you somehow think that would generate better code) should be
good for them all? 

> +	/* Parse %pl[From[To]] */
> +	for (i = 0; i < STRING_UNITS_2_NUM; i++)
> +		if (fmt[1] == string_units_2[i][0])
> +			break;
> +	if (i < STRING_UNITS_2_NUM) {
> +		from = i;
> +		for (i = 0; i < STRING_UNITS_2_NUM; i++)
> +			if (fmt[2] == string_units_2[i][0])
> +				break;
> +		if (i < STRING_UNITS_2_NUM && i >= from)
> +			to = i;
> +	}
>  
>  	if (value)
>  		index = __ffs64(value) / 10;
>  	else
>  		index = 0;
> +	index = clamp_t(unsigned long, index, from, to);

And then you wouldn't need to use the _t version here.

>  	/* Print numeric part */
>  	buf = number(buf, end, value >> (index * 10), default_dec_spec);
> @@ -1459,7 +1475,7 @@ int kptr_restrict __read_mostly;
>   *       Do not use this feature without some mechanism to verify the
>   *       correctness of the format string and va_list arguments.
>   * - 'K' For a kernel pointer that should be hidden from unprivileged users
> - * - 'l' For a value in human-readable form (with IEC binary prefix)
> + * - 'l[from[to]]' For a value in human-readable form (with IEC binary prefix)
>   * - 'NF' For a netdev_features_t
>   * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
>   *            a certain separator (' ' by default):