Received-SPF: pass (google.com: domain of linux-kernel+bounces-15334-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1704168510.git.wqu@suse.com> <a56def269d7885840a19a57aca0169891f5f0f32.1704168510.git.wqu@suse.com>
 <CAMuHMdWXRid2hyvMLcQ6f+M1fxBZUPdeSN=3e=-xXNSce4gsJg@mail.gmail.com> <756ac3e8-3d68-40fe-a7d4-1cf6ac77185e@gmx.com>
In-Reply-To: <756ac3e8-3d68-40fe-a7d4-1cf6ac77185e@gmx.com>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Wed, 3 Jan 2024 10:27:30 +0100
Message-ID: <CAMuHMdVNZPm0jdqD0EdahiTc8PJYQ+OVvBxKagQx_je-GTmJ2w@mail.gmail.com>
Subject: Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe()
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: Qu Wenruo <wqu@suse.com>, 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, ddiss@suse.de
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Qu,

On Tue, Jan 2, 2024 at 9:56=E2=80=AFPM Qu Wenruo <quwenruo.btrfs@gmx.com> w=
rote:
> On 2024/1/2 23:53, Geert Uytterhoeven wrote:
> > On Tue, Jan 2, 2024 at 5:13=E2=80=AFAM 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 er=
ror cases.
> >> + */
> >> +#define ULL_PATTERN            (0xefefefef7a7a7a7aULL)
> >> +#if BITS_PER_LONG =3D=3D 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_M=
AX))

[1] https://lore.kernel.org/oe-kbuild-all/202312181649.u6k6hLIm-lkp@intel.c=
om/
[2] https://elixir.bootlin.com/linux/latest/source/include/linux/kernel.h#L=
82

Gr{oetje,eeting}s,

                        Geert

--=20
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
.org

In personal conversations with technical people, I call myself a hacker. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- Linus Torvalds