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

[Andy Shevchenko]

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?

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

-- 
With Best Regards,
Andy Shevchenko