[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <756ac3e8-3d68-40fe-a7d4-1cf6ac77185e@gmx.com>
Date: Wed, 3 Jan 2024 07:25:33 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>, Qu Wenruo <wqu@...e.com>
Cc: linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
akpm@...ux-foundation.org, christophe.jaillet@...adoo.fr,
andriy.shevchenko@...ux.intel.com, David.Laight@...lab.com, ddiss@...e.de
Subject: Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe()
On 2024/1/2 23:53, Geert Uytterhoeven wrote:
> Hi Qu,
>
> On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <wqu@...e.com> wrote:
>> The new tests cases for memparse_safe() include:
>>
>> - The existing test cases for kstrtoull()
>> Including all the 3 bases (8, 10, 16), and all the ok and failure
>> cases.
>> Although there are something we need to verify specific for
>> memparse_safe():
>>
>> * @retptr and @value are not modified for failure cases
>>
>> * return value are correct for failure cases
>>
>> * @retptr is correct for the good cases
>>
>> - New test cases
>> Not only testing the result value, but also the @retptr, including:
>>
>> * good cases with extra tailing chars, but without valid prefix
>> The @retptr should point to the first char after a valid string.
>> 3 cases for all the 3 bases.
>>
>> * good cases with extra tailing chars, with valid prefix
>> 5 cases for all the suffixes.
>>
>> * bad cases without any number but stray suffix
>> Should be rejected with -EINVAL
>>
>> Signed-off-by: Qu Wenruo <wqu@...e.com>
>
> Thanks for your patch!
>
>> --- a/lib/test-kstrtox.c
>> +++ b/lib/test-kstrtox.c
>> @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void)
>> TEST_OK(kstrtoll, long long, "%lld", test_ll_ok);
>> }
>>
>> +/*
>> + * The special pattern to make sure the result is not modified for error cases.
>> + */
>> +#define ULL_PATTERN (0xefefefef7a7a7a7aULL)
>> +#if BITS_PER_LONG == 32
>> +#define POINTER_PATTERN (0xefef7a7a7aUL)
>
> This pattern needs 40 bits to fit, so it doesn't fit in a 32-bit
> unsigned long or pointer. Probably you wanted to use 0xef7a7a7aUL
> instead?
My bad, one extra byte...
>
>> +#else
>> +#define POINTER_PATTERN (ULL_PATTERN)
>> +#endif
>
> Shouldn't a simple cast to uintptr_t work fine for both 32-bit and
> 64-bit systems:
>
> #define POINTER_PATTERN ((uintptr_t)ULL_PATTERN)
>
> Or even better, incorporate the cast to a pointer:
>
> #define POINTER_PATTERN ((void *)(uintptr_t)ULL_PATTERN)
The problem is reported by sparse, which warns about that ULL_PATTERN
converted to a pointer would lose its width:
lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from
constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
I'm not sure if using uiintptr_t would solve it, thus I go the macro to
switch the value to avoid the static checker's warning.
I tried to check how other locations handles patterned pointer value,
like CONFIG_INIT_STACK_ALL_PATTERN, but they're either relying on the
compiler or just memset().
Any better idea to solve the problem in a better way?
Thanks,
Qu
>
> so you can drop the extra cast when assigning/comparing retptr below.
>
>> +
>> +/* Want to include "E" suffix for full coverage. */
>> +#define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
>> + MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
>> + MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E)
>> +
>> +static void __init test_memparse_safe_fail(void)
>> +{
>
> [...]
>
>> + for_each_test(i, tests) {
>> + const struct memparse_test_fail *t = &tests[i];
>> + unsigned long long tmp = ULL_PATTERN;
>> + char *retptr = (char *)POINTER_PATTERN;
>> + int ret;
>
> [...]
>
> + if (retptr != (char *)POINTER_PATTERN)
>
> Gr{oetje,eeting}s,
>
> Geert
>
Powered by blists - more mailing lists