[PATCH v2 1/5] lib/cmdline_kunit: add a new test case for get_options()

[PATCH v2 1/5] lib/cmdline_kunit: add a new test case for get_options()

[Andy Shevchenko]

Add a test case for get_options() which is provided by cmdline.c.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/cmdline_kunit.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
index 550e7a47fd24..74da9ed61779 100644
--- a/lib/cmdline_kunit.c
+++ b/lib/cmdline_kunit.c
@@ -18,6 +18,26 @@ static const int cmdline_test_values[] = {
 	1, 3, 2, 1, 1, 1, 3, 1,
 };
 
+static_assert(ARRAY_SIZE(cmdline_test_strings) == ARRAY_SIZE(cmdline_test_values));
+
+static const char *cmdline_test_range_strings[] = {
+	"-7" , "--7"  , "-1-2"    , "7--9",
+	"7-" , "-7--9", "7-9,"    , "9-7" ,
+	"5-a", "a-5"  , "5-8"     , ",8-5",
+	"+,1", "-,4"  , "-3,0-1,6", "4,-" ,
+	" +2", " -9"  , "0-1,-3,6", "- 9" ,
+};
+
+static const int cmdline_test_range_values[][16] = {
+	{ 1, -7, }, { 0, -0, }, { 4, -1, 0, +1, 2, }, { 0, 7, },
+	{ 0, +7, }, { 0, -7, }, { 3, +7, 8, +9, 0, }, { 0, 9, },
+	{ 0, +5, }, { 0, -0, }, { 4, +5, 6, +7, 8, }, { 0, 0, },
+	{ 0, +0, }, { 0, -0, }, { 4, -3, 0, +1, 6, }, { 1, 4, },
+	{ 0, +0, }, { 0, -0, }, { 4, +0, 1, -3, 6, }, { 0, 0, },
+};
+
+static_assert(ARRAY_SIZE(cmdline_test_range_strings) == ARRAY_SIZE(cmdline_test_range_values));
+
 static void cmdline_do_one_test(struct kunit *test, const char *in, int rc, int offset)
 {
 	const char *fmt = "Pattern: %s";
@@ -84,10 +104,42 @@ static void cmdline_test_tail_int(struct kunit *test)
 	} while (++i < ARRAY_SIZE(cmdline_test_strings));
 }
 
+static void cmdline_do_one_range_test(struct kunit *test, const char *in,
+				      unsigned int n, const int *e)
+{
+	unsigned int i;
+	int r[16];
+
+#define FMT	"in test %u"
+#define FMT2	"expected %d numbers, got %d"
+#define FMT3	"at %d"
+	memset(r, 0, sizeof(r));
+	get_options(in, ARRAY_SIZE(r), r);
+	KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], FMT " " FMT2, n, e[0], r[0]);
+	for (i = 1; i < ARRAY_SIZE(r); i++)
+		KUNIT_EXPECT_EQ_MSG(test, r[i], e[i], FMT " " FMT3, n, i);
+#undef FMT3
+#undef FMT2
+#undef FMT
+}
+
+static void cmdline_test_range(struct kunit *test)
+{
+	unsigned int i = 0;
+
+	do {
+		const char *str = cmdline_test_range_strings[i];
+		const int *e = cmdline_test_range_values[i];
+
+		cmdline_do_one_range_test(test, str, i, e);
+	} while (++i < ARRAY_SIZE(cmdline_test_range_strings));
+}
+
 static struct kunit_case cmdline_test_cases[] = {
 	KUNIT_CASE(cmdline_test_noint),
 	KUNIT_CASE(cmdline_test_lead_int),
 	KUNIT_CASE(cmdline_test_tail_int),
+	KUNIT_CASE(cmdline_test_range),
 	{}
 };
 
-- 
2.29.2

[PATCH v2 2/5] lib/cmdline: Update documentation to reflect behaviour

[Andy Shevchenko]

get_options() API has some tricks to optimize that may be not so obvious
to the caller. Update documentation to reflect current behaviour.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/cmdline.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index b390dd03363b..2a9ae2143e42 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -83,7 +83,7 @@ EXPORT_SYMBOL(get_option);
  *	get_options - Parse a string into a list of integers
  *	@str: String to be parsed
  *	@nints: size of integer array
- *	@ints: integer array
+ *	@ints: integer array (must have a room for at least one element)
  *
  *	This function parses a string containing a comma-separated
  *	list of integers, a hyphen-separated range of _positive_ integers,
@@ -91,6 +91,11 @@ EXPORT_SYMBOL(get_option);
  *	full, or when no more numbers can be retrieved from the
  *	string.
  *
+ *	Returns:
+ *
+ *	The first element is filled by the amount of the collected numbers
+ *	in the range. The rest is what was parsed from the @str.
+ *
  *	Return value is the character in the string which caused
  *	the parse to end (typically a null terminator, if @str is
  *	completely parseable).
-- 
2.29.2

[PATCH v2 3/5] lib/cmdline: Allow get_options() to take 0 to validate the input

[Andy Shevchenko]

Allow get_options() to take 0 as a number of integers parameter to validate
the input.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/cmdline.c       | 14 +++++++++++---
 lib/cmdline_kunit.c | 10 +++++++++-
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 2a9ae2143e42..1106a8bcd63e 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -91,6 +91,9 @@ EXPORT_SYMBOL(get_option);
  *	full, or when no more numbers can be retrieved from the
  *	string.
  *
+ *	When @nints is 0, the function just validates the given @str and
+ *	returns amount of parseable integers as described below.
+ *
  *	Returns:
  *
  *	The first element is filled by the amount of the collected numbers
@@ -103,15 +106,20 @@ EXPORT_SYMBOL(get_option);
 
 char *get_options(const char *str, int nints, int *ints)
 {
+	bool validate = nints == 0;
 	int res, i = 1;
 
-	while (i < nints) {
-		res = get_option((char **)&str, ints + i);
+	while (i < nints || validate) {
+		int *pint = validate ? ints : ints + i;
+
+		res = get_option((char **)&str, pint);
 		if (res == 0)
 			break;
 		if (res == 3) {
+			int n = validate ? 0 : nints - i;
 			int range_nums;
-			range_nums = get_range((char **)&str, ints + i, nints - i);
+
+			range_nums = get_range((char **)&str, pint, n);
 			if (range_nums < 0)
 				break;
 			/*
diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
index 74da9ed61779..a6119c164b48 100644
--- a/lib/cmdline_kunit.c
+++ b/lib/cmdline_kunit.c
@@ -109,15 +109,23 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in,
 {
 	unsigned int i;
 	int r[16];
+	int *p;
 
 #define FMT	"in test %u"
 #define FMT2	"expected %d numbers, got %d"
 #define FMT3	"at %d"
 	memset(r, 0, sizeof(r));
 	get_options(in, ARRAY_SIZE(r), r);
-	KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], FMT " " FMT2, n, e[0], r[0]);
+	KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], FMT " (parsed) " FMT2, n, e[0], r[0]);
 	for (i = 1; i < ARRAY_SIZE(r); i++)
 		KUNIT_EXPECT_EQ_MSG(test, r[i], e[i], FMT " " FMT3, n, i);
+
+	memset(r, 0, sizeof(r));
+	get_options(in, 0, r);
+	KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], FMT " (validated) " FMT2, n, e[0], r[0]);
+
+	p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0]));
+	KUNIT_EXPECT_PTR_EQ_MSG(test, p, (int *)0, FMT " out of bound " FMT3, n, p - r);
 #undef FMT3
 #undef FMT2
 #undef FMT
-- 
2.29.2

[PATCH v2 4/5] gpio: aggregator: Replace isrange() by using get_options()

[Andy Shevchenko]

We already have a nice helper called get_options() which can be used
to validate the input format. Replace isrange() by using it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 33 +++------------------------------
 1 file changed, 3 insertions(+), 30 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index dfd8a4876a27..40a081b095fb 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -62,34 +62,6 @@ static char *get_arg(char **args)
 	return start;
 }
 
-static bool isrange(const char *s)
-{
-	size_t n;
-
-	if (IS_ERR_OR_NULL(s))
-		return false;
-
-	while (1) {
-		n = strspn(s, "0123456789");
-		if (!n)
-			return false;
-
-		s += n;
-
-		switch (*s++) {
-		case '\0':
-			return true;
-
-		case '-':
-		case ',':
-			break;
-
-		default:
-			return false;
-		}
-	}
-}
-
 static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
 			 int hwnum, unsigned int *n)
 {
@@ -112,10 +84,10 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
 
 static int aggr_parse(struct gpio_aggregator *aggr)
 {
+	char *name, *offsets, *p;
 	char *args = aggr->args;
 	unsigned long *bitmap;
 	unsigned int i, n = 0;
-	char *name, *offsets;
 	int error = 0;
 
 	bitmap = bitmap_alloc(ARCH_NR_GPIOS, GFP_KERNEL);
@@ -130,7 +102,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 			goto free_bitmap;
 		}
 
-		if (!isrange(offsets)) {
+		p = get_options(offsets, 0, &error);
+		if (error == 0 || *p) {
 			/* Named GPIO line */
 			error = aggr_add_gpio(aggr, name, U16_MAX, &n);
 			if (error)
-- 
2.29.2

[PATCH v2 5/5] gpio: aggregator: Remove trailing comma in terminator entries

[Andy Shevchenko]

Remove trailing comma in terminator entries to avoid potential
expanding an array behind it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 40a081b095fb..0cab833fbd81 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -244,7 +244,7 @@ static DRIVER_ATTR_WO(delete_device);
 static struct attribute *gpio_aggregator_attrs[] = {
 	&driver_attr_new_device.attr,
 	&driver_attr_delete_device.attr,
-	NULL,
+	NULL
 };
 ATTRIBUTE_GROUPS(gpio_aggregator);
 
@@ -518,7 +518,7 @@ static const struct of_device_id gpio_aggregator_dt_ids[] = {
 	 * Add GPIO-operated devices controlled from userspace below,
 	 * or use "driver_override" in sysfs
 	 */
-	{},
+	{}
 };
 MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
 #endif
-- 
2.29.2

Re: [PATCH v2 4/5] gpio: aggregator: Replace isrange() by using get_options()

[Linus Walleij]

On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> We already have a nice helper called get_options() which can be used
> to validate the input format. Replace isrange() by using it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This is really neat.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 5/5] gpio: aggregator: Remove trailing comma in terminator entries

[Linus Walleij]

On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Remove trailing comma in terminator entries to avoid potential
> expanding an array behind it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 5/5] gpio: aggregator: Remove trailing comma in terminator entries

[Andy Shevchenko]

On Thu, Jan 21, 2021 at 02:17:47PM +0100, Linus Walleij wrote:
> On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Remove trailing comma in terminator entries to avoid potential
> > expanding an array behind it.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

Bart, I would like to send this series as a part of my Intel GPIO PR or tell me
if you wish another approach.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] gpio: aggregator: Remove trailing comma in terminator entries

[Bartosz Golaszewski]

On Thu, Jan 21, 2021 at 6:08 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jan 21, 2021 at 02:17:47PM +0100, Linus Walleij wrote:
> > On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > Remove trailing comma in terminator entries to avoid potential
> > > expanding an array behind it.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Thanks!
>
> Bart, I would like to send this series as a part of my Intel GPIO PR or tell me
> if you wish another approach.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

No that's fine.

Bart

Re: [PATCH v2 5/5] gpio: aggregator: Remove trailing comma in terminator entries

[Bartosz Golaszewski]

On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Remove trailing comma in terminator entries to avoid potential
> expanding an array behind it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-aggregator.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index 40a081b095fb..0cab833fbd81 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -244,7 +244,7 @@ static DRIVER_ATTR_WO(delete_device);
>  static struct attribute *gpio_aggregator_attrs[] = {
>         &driver_attr_new_device.attr,
>         &driver_attr_delete_device.attr,
> -       NULL,
> +       NULL
>  };
>  ATTRIBUTE_GROUPS(gpio_aggregator);
>
> @@ -518,7 +518,7 @@ static const struct of_device_id gpio_aggregator_dt_ids[] = {
>          * Add GPIO-operated devices controlled from userspace below,
>          * or use "driver_override" in sysfs
>          */
> -       {},
> +       {}
>  };
>  MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
>  #endif
> --
> 2.29.2
>

Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Re: [PATCH v2 4/5] gpio: aggregator: Replace isrange() by using get_options()

[Bartosz Golaszewski]

On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> We already have a nice helper called get_options() which can be used
> to validate the input format. Replace isrange() by using it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Re: [PATCH v2 5/5] gpio: aggregator: Remove trailing comma in terminator entries

[Geert Uytterhoeven]

On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Remove trailing comma in terminator entries to avoid potential
> expanding an array behind it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v2 1/5] lib/cmdline_kunit: add a new test case for get_options()

[Bartosz Golaszewski]

On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Add a test case for get_options() which is provided by cmdline.c.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/cmdline_kunit.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
> index 550e7a47fd24..74da9ed61779 100644
> --- a/lib/cmdline_kunit.c
> +++ b/lib/cmdline_kunit.c
> @@ -18,6 +18,26 @@ static const int cmdline_test_values[] = {
>         1, 3, 2, 1, 1, 1, 3, 1,
>  };
>
> +static_assert(ARRAY_SIZE(cmdline_test_strings) == ARRAY_SIZE(cmdline_test_values));
> +
> +static const char *cmdline_test_range_strings[] = {
> +       "-7" , "--7"  , "-1-2"    , "7--9",
> +       "7-" , "-7--9", "7-9,"    , "9-7" ,
> +       "5-a", "a-5"  , "5-8"     , ",8-5",
> +       "+,1", "-,4"  , "-3,0-1,6", "4,-" ,
> +       " +2", " -9"  , "0-1,-3,6", "- 9" ,
> +};
> +
> +static const int cmdline_test_range_values[][16] = {
> +       { 1, -7, }, { 0, -0, }, { 4, -1, 0, +1, 2, }, { 0, 7, },
> +       { 0, +7, }, { 0, -7, }, { 3, +7, 8, +9, 0, }, { 0, 9, },
> +       { 0, +5, }, { 0, -0, }, { 4, +5, 6, +7, 8, }, { 0, 0, },
> +       { 0, +0, }, { 0, -0, }, { 4, -3, 0, +1, 6, }, { 1, 4, },
> +       { 0, +0, }, { 0, -0, }, { 4, +0, 1, -3, 6, }, { 0, 0, },
> +};
> +
> +static_assert(ARRAY_SIZE(cmdline_test_range_strings) == ARRAY_SIZE(cmdline_test_range_values));
> +
>  static void cmdline_do_one_test(struct kunit *test, const char *in, int rc, int offset)
>  {
>         const char *fmt = "Pattern: %s";
> @@ -84,10 +104,42 @@ static void cmdline_test_tail_int(struct kunit *test)
>         } while (++i < ARRAY_SIZE(cmdline_test_strings));
>  }
>
> +static void cmdline_do_one_range_test(struct kunit *test, const char *in,
> +                                     unsigned int n, const int *e)
> +{
> +       unsigned int i;
> +       int r[16];
> +
> +#define FMT    "in test %u"
> +#define FMT2   "expected %d numbers, got %d"
> +#define FMT3   "at %d"
> +       memset(r, 0, sizeof(r));
> +       get_options(in, ARRAY_SIZE(r), r);
> +       KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], FMT " " FMT2, n, e[0], r[0]);
> +       for (i = 1; i < ARRAY_SIZE(r); i++)
> +               KUNIT_EXPECT_EQ_MSG(test, r[i], e[i], FMT " " FMT3, n, i);
> +#undef FMT3
> +#undef FMT2
> +#undef FMT

IMO these FMT macros are super confusing, please just use hardcoded
strings because you're not saving any code anyway.

Bart

> +}
> +
> +static void cmdline_test_range(struct kunit *test)
> +{
> +       unsigned int i = 0;
> +
> +       do {
> +               const char *str = cmdline_test_range_strings[i];
> +               const int *e = cmdline_test_range_values[i];
> +
> +               cmdline_do_one_range_test(test, str, i, e);
> +       } while (++i < ARRAY_SIZE(cmdline_test_range_strings));
> +}
> +
>  static struct kunit_case cmdline_test_cases[] = {
>         KUNIT_CASE(cmdline_test_noint),
>         KUNIT_CASE(cmdline_test_lead_int),
>         KUNIT_CASE(cmdline_test_tail_int),
> +       KUNIT_CASE(cmdline_test_range),
>         {}
>  };
>
> --
> 2.29.2
>

Re: [PATCH v2 2/5] lib/cmdline: Update documentation to reflect behaviour

[Bartosz Golaszewski]

On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> get_options() API has some tricks to optimize that may be not so obvious
> to the caller. Update documentation to reflect current behaviour.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/cmdline.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index b390dd03363b..2a9ae2143e42 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -83,7 +83,7 @@ EXPORT_SYMBOL(get_option);
>   *     get_options - Parse a string into a list of integers
>   *     @str: String to be parsed
>   *     @nints: size of integer array
> - *     @ints: integer array
> + *     @ints: integer array (must have a room for at least one element)
>   *
>   *     This function parses a string containing a comma-separated
>   *     list of integers, a hyphen-separated range of _positive_ integers,
> @@ -91,6 +91,11 @@ EXPORT_SYMBOL(get_option);
>   *     full, or when no more numbers can be retrieved from the
>   *     string.
>   *
> + *     Returns:
> + *
> + *     The first element is filled by the amount of the collected numbers
> + *     in the range. The rest is what was parsed from the @str.
> + *
>   *     Return value is the character in the string which caused
>   *     the parse to end (typically a null terminator, if @str is
>   *     completely parseable).
> --
> 2.29.2
>

Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Re: [PATCH v2 3/5] lib/cmdline: Allow get_options() to take 0 to validate the input

[Bartosz Golaszewski]

On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Allow get_options() to take 0 as a number of integers parameter to validate
> the input.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/cmdline.c       | 14 +++++++++++---
>  lib/cmdline_kunit.c | 10 +++++++++-
>  2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index 2a9ae2143e42..1106a8bcd63e 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -91,6 +91,9 @@ EXPORT_SYMBOL(get_option);
>   *     full, or when no more numbers can be retrieved from the
>   *     string.
>   *
> + *     When @nints is 0, the function just validates the given @str and
> + *     returns amount of parseable integers as described below.

I'm not a native English speaker but it sounds like this should be
"returns the amount".

> + *
>   *     Returns:
>   *
>   *     The first element is filled by the amount of the collected numbers
> @@ -103,15 +106,20 @@ EXPORT_SYMBOL(get_option);
>
>  char *get_options(const char *str, int nints, int *ints)
>  {
> +       bool validate = nints == 0;

bool validate = (nints == 0) would be clearer IMO.

>         int res, i = 1;
>
> -       while (i < nints) {
> -               res = get_option((char **)&str, ints + i);
> +       while (i < nints || validate) {
> +               int *pint = validate ? ints : ints + i;
> +
> +               res = get_option((char **)&str, pint);
>                 if (res == 0)
>                         break;
>                 if (res == 3) {
> +                       int n = validate ? 0 : nints - i;
>                         int range_nums;
> -                       range_nums = get_range((char **)&str, ints + i, nints - i);
> +
> +                       range_nums = get_range((char **)&str, pint, n);
>                         if (range_nums < 0)
>                                 break;
>                         /*
> diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
> index 74da9ed61779..a6119c164b48 100644
> --- a/lib/cmdline_kunit.c
> +++ b/lib/cmdline_kunit.c
> @@ -109,15 +109,23 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in,
>  {
>         unsigned int i;
>         int r[16];
> +       int *p;
>
>  #define FMT    "in test %u"
>  #define FMT2   "expected %d numbers, got %d"
>  #define FMT3   "at %d"
>         memset(r, 0, sizeof(r));
>         get_options(in, ARRAY_SIZE(r), r);
> -       KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], FMT " " FMT2, n, e[0], r[0]);
> +       KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], FMT " (parsed) " FMT2, n, e[0], r[0]);
>         for (i = 1; i < ARRAY_SIZE(r); i++)
>                 KUNIT_EXPECT_EQ_MSG(test, r[i], e[i], FMT " " FMT3, n, i);
> +
> +       memset(r, 0, sizeof(r));
> +       get_options(in, 0, r);
> +       KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], FMT " (validated) " FMT2, n, e[0], r[0]);
> +
> +       p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0]));
> +       KUNIT_EXPECT_PTR_EQ_MSG(test, p, (int *)0, FMT " out of bound " FMT3, n, p - r);
>  #undef FMT3
>  #undef FMT2
>  #undef FMT

Same as the other patch - just put the formatting strings into the messages.

Bart

> --
> 2.29.2
>

Re: [PATCH v2 3/5] lib/cmdline: Allow get_options() to take 0 to validate the input

[Andy Shevchenko]

On Fri, Jan 22, 2021 at 12:15:20PM +0100, Bartosz Golaszewski wrote:
> On Wed, Jan 20, 2021 at 10:45 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > + *     When @nints is 0, the function just validates the given @str and
> > + *     returns amount of parseable integers as described below.
> 
> I'm not a native English speaker but it sounds like this should be
> "returns the amount".

Sounds reasonable. Fixed for v3.

...

> > +       bool validate = nints == 0;
> 
> bool validate = (nints == 0) would be clearer IMO.

I don't see the benefit, but I have changed.

...

> Same as the other patch - just put the formatting strings into the messages.

Okay, I changed.


Thanks for review!

-- 
With Best Regards,
Andy Shevchenko