Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe()

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

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 = &tests[i];
>> +		unsigned long long tmp;
>> +		char *retptr;
>> +		int ret;
>> +
>> +		ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
>> +		if (ret != 0) {
>> +			WARN(1, "str '%s', expected ret 0 got %d\n", t->str, ret);
>> +			continue;
>> +		}
>> +		if (tmp != t->expected_value)
>> +			WARN(1, "str '%s' incorrect result, expected %llu got %llu",
>> +			     t->str, t->expected_value, tmp);
>> +		if (retptr != 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) = {
>> @@ -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>
>