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

[Qu Wenruo]

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@xxxxxxx>