Messages in this thread | | | Date | Thu, 21 Dec 2023 14:00:40 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper |
| |
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
| |