linux-kernel - Re: [PATCH v2 01/39] lib/string_helpers: Add flags param to string_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231024194653.c24qbnk6bx3hep6y@moria.home.lan>
Date:   Tue, 24 Oct 2023 15:46:53 -0400
From:   Kent Overstreet <kent.overstreet@...ux.dev>
To:     Andy Shevchenko <andy@...nel.org>
Cc:     Suren Baghdasaryan <surenb@...gle.com>, akpm@...ux-foundation.org,
        keescook@...omium.org, rostedt@...dmis.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 01/39] lib/string_helpers: Add flags param to
 string_get_size()

On Tue, Oct 24, 2023 at 05:26:18PM +0300, Andy Shevchenko wrote:
> (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@...ux.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.

It's now a flags parameter: passing an empty set of flags is not
"relying on an implementation detail".

> > -/* 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.

Octal human readable numbers? No, no one's wanted that so far and I
very much doubt anyone will want that in the future.

> > +	STRING_SIZE_BASE2	= (1 << 0),
> > +	STRING_SIZE_NOSPACE	= (1 << 1),
> > +	STRING_SIZE_NOBYTES	= (1 << 2),
> >  };
> 
> Please, add necessary comments.

That I can do.

> > +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?

Minimizing the size of the diff and making it more reviewable. It's fine
as an internal implementation thing.

> 
> 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;

No, those would not be atomic changes. In particular changing the
parameter to a flags without changing the callers - that's not how we do
things.

We're currently working towards _better_ type safety for enums, fyi.

The new flags _could_ be a separate patch, but since it would be
touching much the same code as the previous patch I don't see the point
in splitting it.

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

Not sure I understand your complaint? Were you attached to the redundant
Bs?

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

No, I would not be in favor of another %p extension: in particular,
since this takes integer inputs the lack of type safety for %p
extensions coupled with C's very relaxed approach to integer type
conversion would be a really nasty footgun.