Received-SPF: pass (google.com: domain of linux-kernel+bounces-7420-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <15cf089f-be9a-4762-ae6b-4791efca6b44@gmx.com>
Date: Thu, 21 Dec 2023 07:08:08 +1030
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
 Qu Wenruo <wqu@suse.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>,
 Andrew Morton <akpm@linux-foundation.org>, linux-btrfs@vger.kernel.org,
 Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
 linux-kernel@vger.kernel.org, David Sterba <dsterba@suse.cz>
References: <ZYL5CI4aEyVnabhe@smile.fi.intel.com>
Content-Language: en-US
From: Qu Wenruo <quwenruo.btrfs@gmx.com>
Autocrypt: addr=quwenruo.btrfs@gmx.com; keydata=
 xsBNBFnVga8BCACyhFP3ExcTIuB73jDIBA/vSoYcTyysFQzPvez64TUSCv1SgXEByR7fju3o
 8RfaWuHCnkkea5luuTZMqfgTXrun2dqNVYDNOV6RIVrc4YuG20yhC1epnV55fJCThqij0MRL
 1NxPKXIlEdHvN0Kov3CtWA+R1iNN0RCeVun7rmOrrjBK573aWC5sgP7YsBOLK79H3tmUtz6b
 9Imuj0ZyEsa76Xg9PX9Hn2myKj1hfWGS+5og9Va4hrwQC8ipjXik6NKR5GDV+hOZkktU81G5
 gkQtGB9jOAYRs86QG/b7PtIlbd3+pppT0gaS+wvwMs8cuNG+Pu6KO1oC4jgdseFLu7NpABEB
 AAHNIlF1IFdlbnJ1byA8cXV3ZW5ydW8uYnRyZnNAZ214LmNvbT7CwJQEEwEIAD4CGwMFCwkI
 BwIGFQgJCgsCBBYCAwECHgECF4AWIQQt33LlpaVbqJ2qQuHCPZHzoSX+qAUCY00iVQUJDToH
 pgAKCRDCPZHzoSX+qNKACACkjDLzCvcFuDlgqCiS4ajHAo6twGra3uGgY2klo3S4JespWifr
 BLPPak74oOShqNZ8yWzB1Bkz1u93Ifx3c3H0r2vLWrImoP5eQdymVqMWmDAq+sV1Koyt8gXQ
 XPD2jQCrfR9nUuV1F3Z4Lgo+6I5LjuXBVEayFdz/VYK63+YLEAlSowCF72Lkz06TmaI0XMyj
 jgRNGM2MRgfxbprCcsgUypaDfmhY2nrhIzPUICURfp9t/65+/PLlV4nYs+DtSwPyNjkPX72+
 LdyIdY+BqS8cZbPG5spCyJIlZonADojLDYQq4QnufARU51zyVjzTXMg5gAttDZwTH+8LbNI4
 mm2YzsBNBFnVga8BCACqU+th4Esy/c8BnvliFAjAfpzhI1wH76FD1MJPmAhA3DnX5JDORcga
 CbPEwhLj1xlwTgpeT+QfDmGJ5B5BlrrQFZVE1fChEjiJvyiSAO4yQPkrPVYTI7Xj34FnscPj
 /IrRUUka68MlHxPtFnAHr25VIuOS41lmYKYNwPNLRz9Ik6DmeTG3WJO2BQRNvXA0pXrJH1fN
 GSsRb+pKEKHKtL1803x71zQxCwLh+zLP1iXHVM5j8gX9zqupigQR/Cel2XPS44zWcDW8r7B0
 q1eW4Jrv0x19p4P923voqn+joIAostyNTUjCeSrUdKth9jcdlam9X2DziA/DHDFfS5eq4fEv
 ABEBAAHCwHwEGAEIACYCGwwWIQQt33LlpaVbqJ2qQuHCPZHzoSX+qAUCY00ibgUJDToHvwAK
 CRDCPZHzoSX+qK6vB/9yyZlsS+ijtsvwYDjGA2WhVhN07Xa5SBBvGCAycyGGzSMkOJcOtUUf
 tD+ADyrLbLuVSfRN1ke738UojphwkSFj4t9scG5A+U8GgOZtrlYOsY2+cG3R5vjoXUgXMP37
 INfWh0KbJodf0G48xouesn08cbfUdlphSMXujCA8y5TcNyRuNv2q5Nizl8sKhUZzh4BascoK
 DChBuznBsucCTAGrwPgG4/ul6HnWE8DipMKvkV9ob1xJS2W4WJRPp6QdVrBWJ9cCdtpR6GbL
 iQi22uZXoSPv/0oUrGU+U5X4IvdnvT+8viPzszL5wXswJZfqfy8tmHM85yjObVdIG6AlnrrD
In-Reply-To: <ZYL5CI4aEyVnabhe@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:FP4uEyVe//8=;/Tfap43Q9bLW+FPMus/5VVqiWdn
 tuTmSz0jkFzq8eEREhv4SUX5VYep0zzBg8LC+hBuJGhvMlc2fe8fqKzseLicFVmblaw6hXVgY
 0lz5xrvyJXOTYwyjZL1yy9c1QIE90Yll1+Ws5W68xWKbZAjTKH4fJeku8NsEdKWPDasg3IqV0
 9Frwllp3cgfQDIAtXRStdWj93cPQMHZNGeHOLpwmeFxqdb8iopxfMeTAw4Xwaz75nJYDhiUUI
 QdNlt5SHNJHgWKdIJQZFcFyne04dsfEpuc7QiKyYD4xgmul6LF9G4UG5765sArsu+juqcrxUx
 0UzdIE91btSvwT7fx9b/egdRM2XyigI4k4qw6gKaCRnAYHE5x6AX4HlAGvzBjLrT5NJvph+fz
 Jx9FonzFqklmukaWHxyxEvglQ/VkKPGz81NHQJ+4IbR2jKPXH300Q/V41tVQw/BT0OgfYQjt3
 6txaWrBfd5+lnQgiwMoM22N70VSYyNj0Z/zOlxPLuBt1rNsvI4gxYRrgRefe1+xRgvG2WW6t/
 xmEzcz4f0dFCPyKRgQPlDcyLDOBIzfgIhJVn+OdbKla8Ob20A2Kvei0OIsyuITwHRulwDUg46
 6VANCpVVSowTkyedqnCwBjsF8cLCLL8OOd+EV2emX5vDSIibg6aec4veMKjY7CWI1YsyJx4Qx
 gkERVCl6sC8JKlJoPL2w2jd9eTdGXWM36ZyikheP9rBMA7P29AcHcMK1NYNTGTky33CVBMSiY
 es4nshyisKNvIFQdvEIPvaEFCXrfw7GEyRigBMhTkGPWndSt3IK8xo7HrWxiSOzPe2PBOq54D
 SKOIMOhYBOzolg7QFyUXg7ceE9MTtEuJUYBDV55nMZOLuZhzqHDmloVEZtz9mnHkU7Umg36Sw
 7SqmHt54Kd151T5WbJ+d13wIlv/6peHGGYgLR9Pu0qpd12O/d4HgfAcyn2M7iwKk4TzTbuAHd
 m3d2xWzOhvezn/Ze7hvCCj1uZO0=


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 th=
e
>>> _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 suff=
ix 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 c=
all
>> is to keep the suffix behavior...
>>
>> And indeed using _parse_integer() with _parse_interger_fixup_radix() wo=
uld
>> 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'l=
l try
> to summarize my point here.
>
> First of all, I do not like the naming, it's too vague. What kind of suf=
fix?
> 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.

To me, we should mark memparse() as deprecated as soon as possible, not
spreading the damn pandemic to any newer code.

The "convenience" is not an excuse to use incorrect code.

>
> 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 solutio=
n.

>
> Third, as Alexey said, we have metric and byte suffixes and they are dif=
ferent.
> Supporting one without the other is just adding to the existing confusio=
n.
>
> 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 fo=
rm).

Then why there is the hell of memparse() in the first place?
It doesn't have test case (we have cmdline_kunit, but it doesn't test
memparse() at all), nor the proper error detection.

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.

Thanks,
Qu

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