Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Tue, 24 Oct 2023 17:26:18 +0300
From:   Andy Shevchenko <andy@kernel.org>
To:     Suren Baghdasaryan <surenb@google.com>
Cc:     akpm@linux-foundation.org, kent.overstreet@linux.dev,
        keescook@chromium.org, rostedt@goodmis.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 01/39] lib/string_helpers: Add flags param to
 string_get_size()
Message-ID: <ZTfUCiFP3hVJ+EXh@smile.fi.intel.com>
References: <20231024134637.3120277-1-surenb@google.com>
 <20231024134637.3120277-2-surenb@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231024134637.3120277-2-surenb@google.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

(Minimized the list of people for my review / comments)

On Tue, Oct 24, 2023 at 06:45:58AM -0700, Suren Baghdasaryan wrote:
> From: Kent Overstreet <kent.overstreet@linux.dev>
> 
> The new flags parameter allows controlling
>  - Whether or not the units suffix is separated by a space, for
>    compatibility with sort -h
>  - Whether or not to append a B suffix - we're not always printing
>    bytes.

...

>  	string_get_size(nblocks, queue_logical_block_size(q),
> -			STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
> +			0, cap_str_10, sizeof(cap_str_10));

This doesn't seem right (even if it works). We shouldn't rely on the
implementation details.

...

> -	string_get_size(sdkp->capacity, sector_size,
> -			STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));

> +	string_get_size(sdkp->capacity, sector_size, 0,
> +			cap_str_10, sizeof(cap_str_10));

Neither this.

...

> -/* Descriptions of the types of units to
> - * print in */
> -enum string_size_units {
> -	STRING_UNITS_10,	/* use powers of 10^3 (standard SI) */
> -	STRING_UNITS_2,		/* use binary powers of 2^10 */
> +enum string_size_flags {

So, please add UNITS_10 as it is now. It will help if anybody in the future
wants to add, e.g., 8-base numbers.

> +	STRING_SIZE_BASE2	= (1 << 0),
> +	STRING_SIZE_NOSPACE	= (1 << 1),
> +	STRING_SIZE_NOBYTES	= (1 << 2),
>  };

Please, add necessary comments.

...

> +enum string_size_units {
> +	STRING_UNITS_10,	/* use powers of 10^3 (standard SI) */
> +	STRING_UNITS_2,		/* use binary powers of 2^10 */
> +};

And what a point now in having these?

I assume you need to split this to a few patches:

1) rename parameter to be a flags without renaming the definitions (this will
   touch only string_helpers part);
2) do the end job by renaming it all over the drivers;
3) add the other flags one-by-one (each in a separate change);
4) use new flags where it's needed;

Also see below.

...

>  	static const char *const units_10[] = {
> -		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
> +		"", "k", "M", "G", "T", "P", "E", "Z", "Y"
>  	};
>  	static const char *const units_2[] = {
> -		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB"
> +		"", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei", "Zi", "Yi"
>  	};

Ouch, instead of leaving this and actually "cutting the letter" with NO* flags,
you did something different.

...

Now the main part. Since in 50+% cases (I briefly estimated, it may be more)
this is used in printf() why not introducing a new pointer extension for that?

Yes, it may be done separately, but it will look like a double effort to me.
Instead it might give us a possibility to scale w/o touching users each time
we want to do something and at the same time hide this complete API under
printf() implementation.

-- 
With Best Regards,
Andy Shevchenko