linux-kernel - Re: [PATCH v2 1/2] lib/strtox: introduce kstrtoull

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: <97b85612-16ab-4099-9a8e-426df510d7db@gmx.com>
Date: Sat, 23 Dec 2023 20:27:30 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: David Disseldorp <ddiss@...e.de>, Qu Wenruo <wqu@...e.com>
Cc: linux-btrfs@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 David Laight <David.Laight@...LAB.COM>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] lib/strtox: introduce kstrtoull_suffix() helper



On 2023/12/20 16:08, David Disseldorp wrote:
> On Wed, 20 Dec 2023 10:40:00 +1030, Qu Wenruo wrote:
>
[...]
>>
>> ----
>> Changelog:
>> v2:
>> - Use enum bitmap to describe the suffixes
>>    This gets rid of the upper/lower case problem, and enum makes it
>>    a little more readable.
>>
>> - Fix the suffix overflow detection
>>
>> - Move the left shift out of the switch block
>>
>> - Remove the "done" tag
>>    Since no tailing character can already be handled properly.
>
> nit: git am puts this changelog in the commit message when applied.
> Please use `git send-email --annotate` and put it next to the diffstat,
> so that it gets discarded.

Got it.

[...]
>> +};
>> +
>> +/*
>> + * The default suffix list would not include "E" since it's too easy to overflow
>> + * and not much real world usage.
>> + */
>
> ^ this comment is a duplicate.
>
>> +#define KSTRTOULL_SUFFIX_DEFAULT (SUFFIX_K | SUFFIX_M | SUFFIX_G | SUFFIX_T | SUFFIX_P)
>
> I think it'd be clearer if you dropped this default and had callers
> explicitly provide the desired suffix mask.

Well, that would be long, and would be even longer as the newer naming
would be MEMPARSE_SUFFIX_*, to be more explicit on what the suffix is for...

And I really want callers to choose a saner default suffix, thus here
comes the default one.

In fact, in my next version, I also found that there are some memparse()
call sites benefits from the newer suffixes (although won't for the "E"
one).
The example is the call site setup_elfcorehdr(). Where the comment only
mentions KMG, but since memparse() silently added "PE" suffixes, maybe
on some mainframes we saved some time for one or two lucky admins.

[...]
>
>
> With the above changes made, feel free to add
> Reviewed-by: David Disseldorp <ddiss@...e.de>

Thanks for the review, but I'm afraid the newer version would be another
beast.

All the ommitted comments would be addressed a in new series.
>
> I'll leave the review of patch 2/2 up to others, as I'm still a little
> worried about sysfs trailing whitespace regressions.

That won't be a problem anymore, the new series would keep the old
@retptr behavior, thus for btrfs part it won't be changed at all.

Thanks,
Qu
>