Re: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/12/21 22:30, Andy Shevchenko wrote:
> On Thu, Dec 21, 2023 at 07:08:08AM +1030, Qu Wenruo wrote:
>> On 2023/12/21 00:54, Andy Shevchenko wrote:
>>> On Wed, Dec 20, 2023 at 08:31:09PM +1030, Qu Wenruo wrote:
>>>> On 2023/12/20 20:24, Alexey Dobriyan wrote:
>>>>>> Just as mentioned in the comment of memparse(), the simple_stroull()
>>>>>> usage can lead to overflow all by itself.
>>>>>
>>>>> which is the root cause...
>>>>>
>>>>> I don't like one char suffixes. They are easy to integrate but then the
>>>>> _real_ suffixes are "MiB", "GiB", etc.
>>>>>
>>>>> If you care only about memparse(), then using _parse_integer() can be
>>>>> arranged. I don't see why not.
>>>>
>>>> Well, personally speaking I don't think we should even support the suffix at
>>>> all, at least for the only two usage inside btrfs.
>>>>
>>>> But unfortunately I'm not the one to do the final call, and the final call
>>>> is to keep the suffix behavior...
>>>>
>>>> And indeed using _parse_integer() with _parse_interger_fixup_radix() would
>>>> be better, as we don't need to extend the _kstrtoull() code base.
>>>
>>> My comment on the first patch got vanished due to my MTA issues, but I'll try
>>> to summarize my point here.
>>>
>>> First of all, I do not like the naming, it's too vague. What kind of suffix?
>>> Do we suppose to have suffix in the input? What will be the behaviour w/o
>>> suffix?  And so on...
>>
>> I really like David Sterb to hear this though.
>
> Me too, I like to hear opinions. But I will fight for the best we can do here.
>
>> To me, we should mark memparse() as deprecated as soon as possible, not
>> spreading the damn pandemic to any newer code.
>
> Send a patch!
>
>> The "convenience" is not an excuse to use incorrect code.
>
> I do not object this.
>
>>> Second, if it's a problem in memparse(), just fix it and that's all.
>>
>> Nope, the memparse() itself doesn't have any way to indicate errors.
>>
>> It's not fixable in the first place, as long as you want a drop-in solution.
>>
>>> Third, as Alexey said, we have metric and byte suffixes and they are different.
>>> Supporting one without the other is just adding to the existing confusion.
>>>
>>> Last, but not least, we do NOT accept new code in the lib/ without test cases.
>>>
>>> So, that said here is my formal NAK for this series (at least in this form).
>>
>> Then why there is the hell of memparse() in the first place?
>
> You have all means to investigate.
> It used to be setup_mem() till 9b0f5889b12b ("Linux 2.2.18pre9"),
> which in turn was split from setup_arch() in 716454f016a9 ("Import
> 2.1.121pre1")... Looking deeper seems it comes as a parser at hand
> for the mem= command line parameter very long time ago.
>
>> It doesn't have test case (we have cmdline_kunit, but it doesn't test
>> memparse() at all), nor the proper error detection.
>
> Exactly! Someone's job to add this. And the best is the one who touches
> the code. See how cmdline_kunit appears.
>
>> I'm fine to get my patch rejected, but why the hell of memparse() is
>> here in the first place?
>> It doesn't fit any of the standard you mentioned.
>
> So, what standard did we have in above mentioned (prehistorical) time?

Fine, there is no standard in the ancient days.

Then what about going the following path for the whole memparse() rabbit
hole?

- Mark the old memparse() deprecated
- Add a new function memparse_safe() (or rename the older one to
   __memparse, and let the new one to be named memparse()?)
- Add unit test for the new memparse_safe() or whatever the name is
- Try my best to migrate as many call sites as possible
   Only the two btrfs ones I'm 100% confident for now

Would that be a sounding plan?

Thanks,
Qu
>
>>> P.S> The Subject should start with either kstrtox: or lib/kstrtox.c.
>