Messages in this thread |  | | From | Geert Uytterhoeven <> | | Date | Wed, 3 Jan 2024 10:27:30 +0100 | | Subject | Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe() |
| |
Hi Qu,
On Tue, Jan 2, 2024 at 9:56 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > On 2024/1/2 23:53, Geert Uytterhoeven wrote: > > On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <wqu@suse.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@suse.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...
So did that fix the sparse warning? ;-)
> >> +#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)
Ah yes, sparse can be annoying. I'm still looking for a clean and concise way to shut up [1].
> 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?
Masking off the extra bits, like lower_32_bits()[2] does?
#define POINTER_PATTERN ((void *)(uintptr_t)((ULL_PATTERN) & UINTPTR_MAX))
[1] https://lore.kernel.org/oe-kbuild-all/202312181649.u6k6hLIm-lkp@intel.com/ [2] https://elixir.bootlin.com/linux/latest/source/include/linux/kernel.h#L82
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
|  |