[PATCH 0/3] argparse: improve handling of multi-instance args

[PATCH 0/3] argparse: improve handling of multi-instance args

[Bruce Richardson]

Coverity (correctly) identified an issue[1] where, after the recent
rework[2], the internal flag, used by argparse to track what arguments
were previously encountered or not, was out of range for the type and no
longer having any effect. Fixing this flag to be back into range then,
somewhat surprisingly, caused a number of unit test failures to occur.

The reason for these failures is that the tracking of args encountered
is done via setting an internal flag on the user-passed arguments
object. In the unit tests, this caused issues where the flags field was
not getting properly reset between calls to the parse operation. [This
is only an issue after the rework, because previously information like
param type and optionality was encoded in the flags, so they were more
often reset during testing].

Rather than fixing the tests directly to always reset the flags, which
is simply working around the issue IMHO, this patchset instead fixes the
issue in a more user-friendly way by changing the library to never
modify the user-passed structure - making it completely safe to reuse
across multiple calls. This is done in the first two patches.

The final, third patch, adds an additional unit test to check that the
tracking of flags being seen or not, and the handling of the
"RTE_ARGPARSE_FLAG_SUPPORT_MULTI" flag is correct. This closes a gap in
testing, since the original issue of the flag being out-of-range should
have been caught in testing, rather than having to rely on coverity.

[1] Coverity Issue: 470190
[2] https://github.com/DPDK/dpdk/commit/04acc21beeeb78477b15a3f497d3628fd70a6a9f

Bruce Richardson (3):
  argparse: track parsed arguments internally
  argparse: mark parameter struct as const
  test/argparse: add test for repeated arguments

 app/test/test_argparse.c    | 33 +++++++++++++++++
 lib/argparse/rte_argparse.c | 73 ++++++++++++++++++++++---------------
 lib/argparse/rte_argparse.h |  2 +-
 3 files changed, 77 insertions(+), 31 deletions(-)

--
2.48.1

[PATCH 1/3] argparse: track parsed arguments internally

[Bruce Richardson]

Change how the argparse library records what cmdline arguments have been
seen or not; instead of setting an extra hidden flag in the user-passed
argument structure, set a flag on an internally allocated and managed
array. This means that the user parameters are not modified during the
parse operation, so they can be reused multiple times if necessary, e.g.
in testing.

This change also fixes a coverity issue, where the flag used to indicate
a seen argument was not adjusted to take account of the change of type
of the "flags" setting from 64-bit to 32-bit.

Coverity issue: 470190
Fixes: 04acc21beeeb ("argparse: use enums to remove max-value defines in lists")

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/argparse/rte_argparse.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index b5b8467dea..431d0f9b84 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -16,8 +16,6 @@ RTE_LOG_REGISTER_DEFAULT(rte_argparse_logtype, INFO);
 #define ARGPARSE_LOG(level, ...) \
 	RTE_LOG_LINE(level, ARGPARSE, "" __VA_ARGS__)
 
-#define ARG_ATTR_FLAG_PARSED_MASK	RTE_BIT64(63)
-
 static inline bool
 is_arg_optional(const struct rte_argparse_arg *arg)
 {
@@ -273,9 +271,9 @@ verify_argparse_arg(const struct rte_argparse *obj, uint32_t index)
 }
 
 static int
-verify_argparse(const struct rte_argparse *obj)
+verify_argparse(const struct rte_argparse *obj, size_t *nb_args)
 {
-	uint32_t idx;
+	size_t idx;
 	int ret;
 
 	if (obj->prog_name == NULL) {
@@ -302,6 +300,7 @@ verify_argparse(const struct rte_argparse *obj)
 			return ret;
 		idx++;
 	}
+	*nb_args = idx;
 
 	return 0;
 }
@@ -361,8 +360,8 @@ is_arg_match(struct rte_argparse_arg *arg, const char *curr_argv, uint32_t len)
 }
 
 static struct rte_argparse_arg *
-find_option_arg(struct rte_argparse *obj, const char *curr_argv, const char *has_equal,
-		const char **arg_name)
+find_option_arg(struct rte_argparse *obj, uint32_t *idx,
+		const char *curr_argv, const char *has_equal, const char **arg_name)
 {
 	uint32_t len = strlen(curr_argv) - (has_equal != NULL ? strlen(has_equal) : 0);
 	struct rte_argparse_arg *arg;
@@ -377,6 +376,7 @@ find_option_arg(struct rte_argparse *obj, const char *curr_argv, const char *has
 		if (match) {
 			/* Obtains the exact matching name (long or short). */
 			*arg_name = len > 2 ? arg->name_long : arg->name_short;
+			*idx = i;
 			return arg;
 		}
 	}
@@ -606,12 +606,14 @@ is_help(const char *curr_argv)
 }
 
 static int
-parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
+parse_args(struct rte_argparse *obj, bool *arg_parsed,
+		int argc, char **argv, bool *show_help)
 {
 	uint32_t position_count = calc_position_count(obj);
 	struct rte_argparse_arg *arg;
 	uint32_t position_index = 0;
 	const char *arg_name;
+	uint32_t arg_idx;
 	char *curr_argv;
 	char *has_equal;
 	char *value;
@@ -648,13 +650,13 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 
 		has_equal = strchr(curr_argv, '=');
 		arg_name = NULL;
-		arg = find_option_arg(obj, curr_argv, has_equal, &arg_name);
+		arg = find_option_arg(obj, &arg_idx, curr_argv, has_equal, &arg_name);
 		if (arg == NULL || arg_name == NULL) {
 			ARGPARSE_LOG(ERR, "unknown argument %s!", curr_argv);
 			return -EINVAL;
 		}
 
-		if ((arg->flags & ARG_ATTR_FLAG_PARSED_MASK) && !arg_attr_flag_multi(arg)) {
+		if (arg_parsed[arg_idx] && !arg_attr_flag_multi(arg)) {
 			ARGPARSE_LOG(ERR, "argument %s should not occur multiple times!", arg_name);
 			return -EINVAL;
 		}
@@ -684,7 +686,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 			return ret;
 
 		/* This argument parsed success! then mark it parsed. */
-		arg->flags |= ARG_ATTR_FLAG_PARSED_MASK;
+		arg_parsed[arg_idx] = true;
 	}
 
 	return i;
@@ -795,14 +797,25 @@ RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_argparse_parse, 24.03)
 int
 rte_argparse_parse(struct rte_argparse *obj, int argc, char **argv)
 {
+	bool *arg_parsed = NULL;
 	bool show_help = false;
+	size_t nb_args = 0;
 	int ret;
 
-	ret = verify_argparse(obj);
+	ret = verify_argparse(obj, &nb_args);
 	if (ret != 0)
 		goto error;
 
-	ret = parse_args(obj, argc, argv, &show_help);
+	/* allocate the flags array to indicate what arguments are parsed or not */
+	arg_parsed = calloc(nb_args, sizeof(*arg_parsed));
+	if (arg_parsed == NULL) {
+		ARGPARSE_LOG(ERR, "failed to allocate memory for argument flags!");
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	ret = parse_args(obj, arg_parsed, argc, argv, &show_help);
+	free(arg_parsed);
 	if (ret < 0)
 		goto error;
 
-- 
2.48.1

[PATCH 2/3] argparse: mark parameter struct as const

[Bruce Richardson]

Now that argparse no longer modifies the parameters passed in by the
user, mark them as explicitly "const" to provide that guarantee to the
user.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/argparse/rte_argparse.c | 40 ++++++++++++++++++-------------------
 lib/argparse/rte_argparse.h |  2 +-
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 431d0f9b84..331f05f01d 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -323,10 +323,10 @@ calc_position_count(const struct rte_argparse *obj)
 	return count;
 }
 
-static struct rte_argparse_arg *
-find_position_arg(struct rte_argparse *obj, uint32_t index)
+static const struct rte_argparse_arg *
+find_position_arg(const struct rte_argparse *obj, uint32_t index)
 {
-	struct rte_argparse_arg *arg;
+	const struct rte_argparse_arg *arg;
 	uint32_t count = 0;
 	uint32_t i;
 
@@ -345,7 +345,7 @@ find_position_arg(struct rte_argparse *obj, uint32_t index)
 }
 
 static bool
-is_arg_match(struct rte_argparse_arg *arg, const char *curr_argv, uint32_t len)
+is_arg_match(const struct rte_argparse_arg *arg, const char *curr_argv, uint32_t len)
 {
 	if (strlen(arg->name_long) == len && strncmp(arg->name_long, curr_argv, len) == 0)
 		return true;
@@ -359,12 +359,12 @@ is_arg_match(struct rte_argparse_arg *arg, const char *curr_argv, uint32_t len)
 	return false;
 }
 
-static struct rte_argparse_arg *
-find_option_arg(struct rte_argparse *obj, uint32_t *idx,
+static const struct rte_argparse_arg *
+find_option_arg(const struct rte_argparse *obj, uint32_t *idx,
 		const char *curr_argv, const char *has_equal, const char **arg_name)
 {
 	uint32_t len = strlen(curr_argv) - (has_equal != NULL ? strlen(has_equal) : 0);
-	struct rte_argparse_arg *arg;
+	const struct rte_argparse_arg *arg;
 	uint32_t i;
 	bool match;
 
@@ -385,7 +385,7 @@ find_option_arg(struct rte_argparse *obj, uint32_t *idx,
 }
 
 static int
-parse_arg_int(struct rte_argparse_arg *arg, const char *value)
+parse_arg_int(const struct rte_argparse_arg *arg, const char *value)
 {
 	char *s = NULL;
 
@@ -410,7 +410,7 @@ parse_arg_int(struct rte_argparse_arg *arg, const char *value)
 }
 
 static int
-parse_arg_u8(struct rte_argparse_arg *arg, const char *value)
+parse_arg_u8(const struct rte_argparse_arg *arg, const char *value)
 {
 	unsigned long val;
 	char *s = NULL;
@@ -438,7 +438,7 @@ parse_arg_u8(struct rte_argparse_arg *arg, const char *value)
 }
 
 static int
-parse_arg_u16(struct rte_argparse_arg *arg, const char *value)
+parse_arg_u16(const struct rte_argparse_arg *arg, const char *value)
 {
 	unsigned long val;
 	char *s = NULL;
@@ -466,7 +466,7 @@ parse_arg_u16(struct rte_argparse_arg *arg, const char *value)
 }
 
 static int
-parse_arg_u32(struct rte_argparse_arg *arg, const char *value)
+parse_arg_u32(const struct rte_argparse_arg *arg, const char *value)
 {
 	unsigned long val;
 	char *s = NULL;
@@ -494,7 +494,7 @@ parse_arg_u32(struct rte_argparse_arg *arg, const char *value)
 }
 
 static int
-parse_arg_u64(struct rte_argparse_arg *arg, const char *value)
+parse_arg_u64(const struct rte_argparse_arg *arg, const char *value)
 {
 	unsigned long val;
 	char *s = NULL;
@@ -522,7 +522,7 @@ parse_arg_u64(struct rte_argparse_arg *arg, const char *value)
 }
 
 static int
-parse_arg_str(struct rte_argparse_arg *arg, const char *value)
+parse_arg_str(const struct rte_argparse_arg *arg, const char *value)
 {
 	if (value == NULL) {
 		*(char **)arg->val_saver = arg->val_set;
@@ -534,7 +534,7 @@ parse_arg_str(struct rte_argparse_arg *arg, const char *value)
 }
 
 static int
-parse_arg_bool(struct rte_argparse_arg *arg, const char *value)
+parse_arg_bool(const struct rte_argparse_arg *arg, const char *value)
 {
 	if (value == NULL) {
 		*(bool *)arg->val_saver = (arg->val_set != NULL);
@@ -555,7 +555,7 @@ parse_arg_bool(struct rte_argparse_arg *arg, const char *value)
 }
 
 static int
-parse_arg_autosave(struct rte_argparse_arg *arg, const char *value)
+parse_arg_autosave(const struct rte_argparse_arg *arg, const char *value)
 {
 	switch (arg->value_type) {
 	case RTE_ARGPARSE_VALUE_TYPE_NONE:
@@ -582,8 +582,8 @@ parse_arg_autosave(struct rte_argparse_arg *arg, const char *value)
 
 /* arg_parse indicates the name entered by the user, which can be long-name or short-name. */
 static int
-parse_arg_val(struct rte_argparse *obj, const char *arg_name,
-	      struct rte_argparse_arg *arg, char *value)
+parse_arg_val(const struct rte_argparse *obj, const char *arg_name,
+	      const struct rte_argparse_arg *arg, char *value)
 {
 	int ret;
 
@@ -606,11 +606,11 @@ is_help(const char *curr_argv)
 }
 
 static int
-parse_args(struct rte_argparse *obj, bool *arg_parsed,
+parse_args(const struct rte_argparse *obj, bool *arg_parsed,
 		int argc, char **argv, bool *show_help)
 {
 	uint32_t position_count = calc_position_count(obj);
-	struct rte_argparse_arg *arg;
+	const struct rte_argparse_arg *arg;
 	uint32_t position_index = 0;
 	const char *arg_name;
 	uint32_t arg_idx;
@@ -795,7 +795,7 @@ show_args_help(const struct rte_argparse *obj)
 
 RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_argparse_parse, 24.03)
 int
-rte_argparse_parse(struct rte_argparse *obj, int argc, char **argv)
+rte_argparse_parse(const struct rte_argparse *obj, int argc, char **argv)
 {
 	bool *arg_parsed = NULL;
 	bool show_help = false;
diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index 3d04d47771..52bef34363 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -187,7 +187,7 @@ struct rte_argparse {
  *   Otherwise negative error code is returned.
  */
 __rte_experimental
-int rte_argparse_parse(struct rte_argparse *obj, int argc, char **argv);
+int rte_argparse_parse(const struct rte_argparse *obj, int argc, char **argv);
 
 /**
  * @warning
-- 
2.48.1

[PATCH 3/3] test/argparse: add test for repeated arguments

[Bruce Richardson]

Add a test case to check that argparse throws an error when an argument
is repeated on the commandline without the SUPPORT_MULTI flag. Also test
that multiple arguments are correctly handled with the flag.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_argparse.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index 57b85c4bf0..0a229752fa 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -329,6 +329,38 @@ test_argparse_invalid_option(void)
 	return 0;
 }
 
+static int
+test_argparse_invalid_repeated_option(void)
+{
+	/* test that we allow repeated args only with the MULTI flag */
+	struct rte_argparse *obj;
+	char *argv[3];
+	int ret;
+
+	/* check that we error out with two "-a" flags */
+	obj = test_argparse_init_obj();
+	obj->args[0].val_saver = NULL;
+	obj->args[1].val_saver = NULL;
+	argv[0] = test_strdup(obj->prog_name);
+	argv[1] = test_strdup("-a");
+	argv[2] = test_strdup("-a");
+	ret = rte_argparse_parse(obj, 3, argv);
+	TEST_ASSERT(ret == -EINVAL, "Argparse did not error out with two '-a' flags!");
+
+	obj = test_argparse_init_obj();
+	obj->args[0].val_saver = NULL;
+	obj->args[1].val_saver = NULL;
+	obj->args[0].flags |= RTE_ARGPARSE_FLAG_SUPPORT_MULTI;
+	/* check that we allow two "-a" flags with MULTI flag set */
+	argv[0] = test_strdup(obj->prog_name);
+	argv[1] = test_strdup("-a");
+	argv[2] = test_strdup("-a");
+	ret = rte_argparse_parse(obj, 3, argv);
+	TEST_ASSERT(ret == 3, "Argparse failed to handle duplicate '-a' flags!");
+
+	return 0;
+}
+
 static int
 test_argparse_opt_autosave_parse_int_of_no_val(void)
 {
@@ -864,6 +896,7 @@ static struct unit_test_suite argparse_test_suite = {
 		TEST_CASE(test_argparse_invalid_arg_flags),
 		TEST_CASE(test_argparse_invalid_arg_repeat),
 		TEST_CASE(test_argparse_invalid_option),
+		TEST_CASE(test_argparse_invalid_repeated_option),
 		TEST_CASE(test_argparse_opt_autosave_parse_int_of_no_val),
 		TEST_CASE(test_argparse_opt_autosave_parse_int_of_required_val),
 		TEST_CASE(test_argparse_opt_autosave_parse_int_of_optional_val),
-- 
2.48.1