Re: [PATCH v3 1/4] lib/string_helpers: export string_units_{2,10} for others

[James Bottomley]

On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> There is one user coming which would like to use those string arrays.
> It might
> be useful for any other user in the future.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  include/linux/string_helpers.h |  3 +++
>  lib/string_helpers.c           | 21 ++++++++++++---------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/string_helpers.h
> b/include/linux/string_helpers.h
> index dabe643..1d16240 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -10,6 +10,9 @@ enum string_size_units {
>  	STRING_UNITS_2,		/* use binary powers of 2^10
> */
>  };
>  
> +extern const char *const string_units_10[];
> +extern const char *const string_units_2[];
> +
>  void string_get_size(u64 size, u64 blk_size, enum string_size_units
> units,
>  		     char *buf, int len);
>  
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 5939f63..86124c9 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -13,6 +13,15 @@
>  #include <linux/string.h>
>  #include <linux/string_helpers.h>
>  
> +const char *const string_units_10[] = {
> +	"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB",
> +};
> +EXPORT_SYMBOL(string_units_10);
> +const char *const string_units_2[] = {
> +	"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB",
> +};
> +EXPORT_SYMBOL(string_units_2);
> +
>  /**
>   * string_get_size - get the size in the specified units
>   * @size:	The size to be converted in blocks
> @@ -29,15 +38,9 @@
>  void string_get_size(u64 size, u64 blk_size, const enum
> string_size_units units,
>  		     char *buf, int len)
>  {
> -	static const char *const units_10[] = {
> -		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
> -	};
> -	static const char *const units_2[] = {
> -		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB",
> "ZiB", "YiB"
> -	};
>  	static const char *const *const units_str[] = {
> -		[STRING_UNITS_10] = units_10,
> -		[STRING_UNITS_2] = units_2,
> +		[STRING_UNITS_10] = string_units_10,
> +		[STRING_UNITS_2] = string_units_2,
>  	};
>  	static const unsigned int divisor[] = {
>  		[STRING_UNITS_10] = 1000,
> @@ -92,7 +95,7 @@ void string_get_size(u64 size, u64 blk_size, const
> enum string_size_units units,
>  	}
>  
>   out:
> -	if (i >= ARRAY_SIZE(units_2))
> +	if (i >= ARRAY_SIZE(string_units_2))

so now, no-one other than string_helpers.c can tell the size of the
array ... I don't think that's an improvement.  Also for a trivial
patch I'm starting to think there should be a three strikes rule: we
get a large number of bugs from allegedly trivial reworks which
wouldn't have happened if we'd retained the original working code in
the first place.

After two attempts, doesn't it perhaps strike you that a helper
function rather than a direct export would get over this difficulty? 
 It might also address the precision problem you introduced.

James