Received-SPF: pass (google.com: domain of linux-kernel+bounces-16092-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <a5a65a27-07bb-4399-bf28-1c27f9af6fbf@gmx.com>
Date: Thu, 4 Jan 2024 09:15:33 +1030
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe()
Content-Language: en-US
To: David Disseldorp <ddiss@suse.de>, Qu Wenruo <wqu@suse.com>
Cc: linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org,
 akpm@linux-foundation.org, christophe.jaillet@wanadoo.fr,
 andriy.shevchenko@linux.intel.com, David.Laight@ACULAB.COM
References: <cover.1704168510.git.wqu@suse.com>
 <a56def269d7885840a19a57aca0169891f5f0f32.1704168510.git.wqu@suse.com>
 <20240103011700.222b2b5c@echidna>
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: <20240103011700.222b2b5c@echidna>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:1+0LOf5gIEk=;Jfk+oH4h12B7dYPmQgQGQM8/RSY
 KPW06GvwGTULDn8OiwtdOgrR9bXIF3Lx5aJMXUotUZVkucxU7ASw2IJSG2TgpRDOWGRSI0Vwe
 KBBRvsnBgHySvENES5iHBF/E8ojFufSxPy9gqP2faKh+9Z0Vaadyg/O9Rh9riDV0uDNGLux4I
 hvGwDpNoEwLxUqiXwEA1M+KpgJBv9frnDaHB9qBamZYCG0MsYuOM6xQlFPwpKkgbnZ5UiwXyt
 7SBO8Ac8FNtM1Na9q4uA6vR3Z5hJcDvWBl3t+6qA2ktI5mFCREtT6FgqTW4mpDVtb/4TP7nOx
 UWhr0KLbjssNFlasks3VGu7fgKNGLYXQxMAF0LIIkAur/Bwp1nQn+SSARbyaGOpHB+nmsuYDP
 QsR7pe0OTFWSxDsU+hYyEBH3ntwfOcOExVRNs8oHA9rw+M89pFqaaZFoynpmqwLMFaNq68xxA
 51uW9+C4cIcIOVRyCpCgeJpGTzYt7snwX30l2lDyyGSP5BNZoIFU6hiFYEX21HJBAAbP8G/NE
 sD6X17IqVDLhkMQGSirw1g1Ltmg+LuaHIMqpsEYiWPqCgp2+2mppA+SLfsH2oUQg7GFvCieeN
 u43xSppXkqkYHxC7FqVHK1lq5geU05z5l+t8WvFrskebYrMQfsDFSu4nF1oAIOGtIkNfdQURP
 MuKqrs2MBsAVV2xp+gCxcpu9kvWt7HHdYNfQfwXEB5lQtzoND6kdxUG5g5WJuFNu1ndLpn1oY
 skEDBoJ8u+bqF5fxajKgfvKEZL4i54r/vMCpGG9xrTAFPcRq6OqRekvZ09cbWkDRQ+C9BB9Xs
 tTxNYkwFWsOB1BPgx0pH/IEFawPVliDvDQKxpriSHeG3hQ8wxpp1pDY8/DTguGKtfSnVIhp/E
 lAi7RCrpnUAEun9KpJCrsqEoWlBd6NuelTX5P9oGj/DYb6p+j1DrmkZ+mvQGNyMvGsCV2wIln
 AtQPWH0qJwiXtQ9PA9EVcmlZndw=


On 2024/1/3 00:47, David Disseldorp wrote:
> On Tue,  2 Jan 2024 14:42:13 +1030, Qu Wenruo wrote:
>
[...]
>> +		{"P", -EINVAL},
>> +
>> +		/* Overflow in the string itself*/
>> +		{"18446744073709551616", -ERANGE},
>> +		{"02000000000000000000000", -ERANGE},
>> +		{"0x10000000000000000",	-ERANGE},
> nit:                                   ^ whitespace damage

Sorry, I didn't get the point here.

I checked the patch it's a single space.
Or I missed/screwed up something?

>> +
>> +		/*
[...]
>> +
>> +		/*
>> +		 * Finally one suffix then tailing chars, to test the @retptr
>> +		 * behavior.
>> +		 */
>> +		{"68k ",		69632,			3},
>> +		{"8MS",			8388608,		2},
>> +		{"0xaeGis",		0x2b80000000,		5},
>> +		{"0xaTx",		0xa0000000000,		4},
>> +		{"3E8",			0x3000000000000000,	2},
>
> In future it'd be good to get some coverage for non-MEMPARSE_TEST_SUFFIX
> use cases, e.g.:
>    /* supported suffix, but not provided with @suffixes */
>    {"7K", (MEMPARSE_SUFFIX_M |\
>            MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
>            MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E), 7, 1},

That's a great idea, since I'm still prepare a v3, it's not hard to add
it into v3.

Thanks,
Qu
>
>> +	};
>> +	unsigned int i;
>> +
>> +	for_each_test(i, tests) {
>> +		const struct memparse_test_ok *t =3D &tests[i];
>> +		unsigned long long tmp;
>> +		char *retptr;
>> +		int ret;
>> +
>> +		ret =3D memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
>> +		if (ret !=3D 0) {
>> +			WARN(1, "str '%s', expected ret 0 got %d\n", t->str, ret);
>> +			continue;
>> +		}
>> +		if (tmp !=3D t->expected_value)
>> +			WARN(1, "str '%s' incorrect result, expected %llu got %llu",
>> +			     t->str, t->expected_value, tmp);
>> +		if (retptr !=3D t->str + t->retptr_off)
>> +			WARN(1, "str '%s' incorrect endptr, expected %u got %zu",
>> +			     t->str, t->retptr_off, retptr - t->str);
>> +	}
>> +}
>>   static void __init test_kstrtoll_fail(void)
>>   {
>>   	static DEFINE_TEST_FAIL(test_ll_fail) =3D {
>> @@ -710,6 +941,10 @@ static int __init test_kstrtox_init(void)
>>   	test_kstrtoll_ok();
>>   	test_kstrtoll_fail();
>>
>> +	test_memparse_safe_ok();
>> +	test_memparse_safe_fail();
>> +
>> +
> nit: whitespace ^
>
>>   	test_kstrtou64_ok();
>>   	test_kstrtou64_fail();
>>   	test_kstrtos64_ok();
>
> With Geert's comments addressed:
> Reviewed-by: David Disseldorp <ddiss@suse.de>
>