This block of tests was meant to find/flag incorrect use of the ":"
and "/" separators (syntax errors) and invalid (zero) group len.
However they were specified with an 8 bit width and 32 bit operations,
so they really contained two errors (EINVAL and ERANGE).
Promote them to 32 bit so it is clear what they are meant to target,
and then add tests specific for ERANGE (no syntax errors, just doing
32bit op on 8 bit width, plus a typical 9-on-8 fencepost error).
Note that the remaining "10-1" on 8 is left as-is, since that captures
the fact that we check for (r->start > r->end) ---> EINVAL before we
check for (r->end >= nbits) ---> ERANGE. If the code is ever re-ordered
then this test will pick up the mismatched errno value.
Cc: Yury Norov <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
lib/test_bitmap.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 4425a1dd4ef1..3d2cd3b1de84 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -337,13 +337,15 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
{-EINVAL, "-1", NULL, 8, 0},
{-EINVAL, "-0", NULL, 8, 0},
- {-EINVAL, "10-1", NULL, 8, 0},
- {-EINVAL, "0-31:", NULL, 8, 0},
- {-EINVAL, "0-31:0", NULL, 8, 0},
- {-EINVAL, "0-31:0/", NULL, 8, 0},
- {-EINVAL, "0-31:0/0", NULL, 8, 0},
- {-EINVAL, "0-31:1/0", NULL, 8, 0},
- {-EINVAL, "0-31:10/1", NULL, 8, 0},
+ {-EINVAL, "10-1", NULL, 8, 0}, /* (start > end) ; also ERANGE */
+ {-ERANGE, "8-8", NULL, 8, 0},
+ {-ERANGE, "0-31", NULL, 8, 0},
+ {-EINVAL, "0-31:", NULL, 32, 0},
+ {-EINVAL, "0-31:0", NULL, 32, 0},
+ {-EINVAL, "0-31:0/", NULL, 32, 0},
+ {-EINVAL, "0-31:0/0", NULL, 32, 0},
+ {-EINVAL, "0-31:1/0", NULL, 32, 0},
+ {-EINVAL, "0-31:10/1", NULL, 32, 0},
{-EOVERFLOW, "0-98765432123456789:10/1", NULL, 8, 0},
{-EINVAL, "a-31", NULL, 8, 0},
--
2.17.1
On Tue, Jan 26, 2021 at 12:11:34PM -0500, Paul Gortmaker wrote:
> This block of tests was meant to find/flag incorrect use of the ":"
> and "/" separators (syntax errors) and invalid (zero) group len.
>
> However they were specified with an 8 bit width and 32 bit operations,
> so they really contained two errors (EINVAL and ERANGE).
>
> Promote them to 32 bit so it is clear what they are meant to target,
> and then add tests specific for ERANGE (no syntax errors, just doing
> 32bit op on 8 bit width, plus a typical 9-on-8 fencepost error).
>
> Note that the remaining "10-1" on 8 is left as-is, since that captures
> the fact that we check for (r->start > r->end) ---> EINVAL before we
> check for (r->end >= nbits) ---> ERANGE. If the code is ever re-ordered
> then this test will pick up the mismatched errno value.
I didn't get the last statement. You meant code in the bitmap library itself,
and not in the test cases? Please, clarify this somehow.
I don't really much care, since it's not a tricky commit, but it might be split
to two or three separated ones. Anyway, feel free to add
Reviewed-by: Andy Shevchenko <[email protected]>
> Cc: Yury Norov <[email protected]>
> Cc: Rasmus Villemoes <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Signed-off-by: Paul Gortmaker <[email protected]>
> ---
> lib/test_bitmap.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index 4425a1dd4ef1..3d2cd3b1de84 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -337,13 +337,15 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
>
> {-EINVAL, "-1", NULL, 8, 0},
> {-EINVAL, "-0", NULL, 8, 0},
> - {-EINVAL, "10-1", NULL, 8, 0},
> - {-EINVAL, "0-31:", NULL, 8, 0},
> - {-EINVAL, "0-31:0", NULL, 8, 0},
> - {-EINVAL, "0-31:0/", NULL, 8, 0},
> - {-EINVAL, "0-31:0/0", NULL, 8, 0},
> - {-EINVAL, "0-31:1/0", NULL, 8, 0},
> - {-EINVAL, "0-31:10/1", NULL, 8, 0},
> + {-EINVAL, "10-1", NULL, 8, 0}, /* (start > end) ; also ERANGE */
> + {-ERANGE, "8-8", NULL, 8, 0},
> + {-ERANGE, "0-31", NULL, 8, 0},
> + {-EINVAL, "0-31:", NULL, 32, 0},
> + {-EINVAL, "0-31:0", NULL, 32, 0},
> + {-EINVAL, "0-31:0/", NULL, 32, 0},
> + {-EINVAL, "0-31:0/0", NULL, 32, 0},
> + {-EINVAL, "0-31:1/0", NULL, 32, 0},
> + {-EINVAL, "0-31:10/1", NULL, 32, 0},
> {-EOVERFLOW, "0-98765432123456789:10/1", NULL, 8, 0},
>
> {-EINVAL, "a-31", NULL, 8, 0},
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
[Re: [PATCH 1/8] lib: test_bitmap: clearly separate ERANGE from EINVAL tests.] On 26/01/2021 (Tue 23:04) Andy Shevchenko wrote:
> On Tue, Jan 26, 2021 at 12:11:34PM -0500, Paul Gortmaker wrote:
> > This block of tests was meant to find/flag incorrect use of the ":"
> > and "/" separators (syntax errors) and invalid (zero) group len.
> >
> > However they were specified with an 8 bit width and 32 bit operations,
> > so they really contained two errors (EINVAL and ERANGE).
> >
> > Promote them to 32 bit so it is clear what they are meant to target,
> > and then add tests specific for ERANGE (no syntax errors, just doing
> > 32bit op on 8 bit width, plus a typical 9-on-8 fencepost error).
> >
> > Note that the remaining "10-1" on 8 is left as-is, since that captures
> > the fact that we check for (r->start > r->end) ---> EINVAL before we
> > check for (r->end >= nbits) ---> ERANGE. If the code is ever re-ordered
> > then this test will pick up the mismatched errno value.
>
> I didn't get the last statement. You meant code in the bitmap library itself,
> and not in the test cases? Please, clarify this somehow.
It probably wasn't worth a mention at all, as that test in question was
left unchanged; but yes - it was a reference to the ordering of the
sanity checks in the bitmap code itself and not the test order. I'll
simply delete the confusing "10-1" paragraph/comment.
> I don't really much care, since it's not a tricky commit, but it might be split
> to two or three separated ones. Anyway, feel free to add
> Reviewed-by: Andy Shevchenko <[email protected]>
Since you mentioned it, I assume you would prefer it. So I will make
the 8 --> 32 change in one commit, and add the two new ERANGE tests in
another subsequent commit.
Thanks,
Paul.
--
>
> > Cc: Yury Norov <[email protected]>
> > Cc: Rasmus Villemoes <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Signed-off-by: Paul Gortmaker <[email protected]>
> > ---
> > lib/test_bitmap.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> > index 4425a1dd4ef1..3d2cd3b1de84 100644
> > --- a/lib/test_bitmap.c
> > +++ b/lib/test_bitmap.c
> > @@ -337,13 +337,15 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
> >
> > {-EINVAL, "-1", NULL, 8, 0},
> > {-EINVAL, "-0", NULL, 8, 0},
> > - {-EINVAL, "10-1", NULL, 8, 0},
> > - {-EINVAL, "0-31:", NULL, 8, 0},
> > - {-EINVAL, "0-31:0", NULL, 8, 0},
> > - {-EINVAL, "0-31:0/", NULL, 8, 0},
> > - {-EINVAL, "0-31:0/0", NULL, 8, 0},
> > - {-EINVAL, "0-31:1/0", NULL, 8, 0},
> > - {-EINVAL, "0-31:10/1", NULL, 8, 0},
> > + {-EINVAL, "10-1", NULL, 8, 0}, /* (start > end) ; also ERANGE */
> > + {-ERANGE, "8-8", NULL, 8, 0},
> > + {-ERANGE, "0-31", NULL, 8, 0},
> > + {-EINVAL, "0-31:", NULL, 32, 0},
> > + {-EINVAL, "0-31:0", NULL, 32, 0},
> > + {-EINVAL, "0-31:0/", NULL, 32, 0},
> > + {-EINVAL, "0-31:0/0", NULL, 32, 0},
> > + {-EINVAL, "0-31:1/0", NULL, 32, 0},
> > + {-EINVAL, "0-31:10/1", NULL, 32, 0},
> > {-EOVERFLOW, "0-98765432123456789:10/1", NULL, 8, 0},
> >
> > {-EINVAL, "a-31", NULL, 8, 0},
> > --
> > 2.17.1
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>