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

DPDK patches and discussions
 help / color / mirror / Atom feed

* [PATCH 0/3] argparse: improve handling of multi-instance args
@ 2025-06-16 10:49 Bruce Richardson
  2025-06-16 10:49 ` [PATCH 1/3] argparse: track parsed arguments internally Bruce Richardson
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Bruce Richardson @ 2025-06-16 10:49 UTC (permalink / raw)
  To: dev; +Cc: 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/3] argparse: track parsed arguments internally
  2025-06-16 10:49 [PATCH 0/3] argparse: improve handling of multi-instance args Bruce Richardson
@ 2025-06-16 10:49 ` Bruce Richardson
  2025-06-16 10:49 ` [PATCH 2/3] argparse: mark parameter struct as const Bruce Richardson
  2025-06-16 10:49 ` [PATCH 3/3] test/argparse: add test for repeated arguments Bruce Richardson
  2 siblings, 0 replies; 4+ messages in thread
From: Bruce Richardson @ 2025-06-16 10:49 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, Chengwen Feng

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/3] argparse: mark parameter struct as const
  2025-06-16 10:49 [PATCH 0/3] argparse: improve handling of multi-instance args Bruce Richardson
  2025-06-16 10:49 ` [PATCH 1/3] argparse: track parsed arguments internally Bruce Richardson
@ 2025-06-16 10:49 ` Bruce Richardson
  2025-06-16 10:49 ` [PATCH 3/3] test/argparse: add test for repeated arguments Bruce Richardson
  2 siblings, 0 replies; 4+ messages in thread
From: Bruce Richardson @ 2025-06-16 10:49 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, Chengwen Feng

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 3/3] test/argparse: add test for repeated arguments
  2025-06-16 10:49 [PATCH 0/3] argparse: improve handling of multi-instance args Bruce Richardson
  2025-06-16 10:49 ` [PATCH 1/3] argparse: track parsed arguments internally Bruce Richardson
  2025-06-16 10:49 ` [PATCH 2/3] argparse: mark parameter struct as const Bruce Richardson
@ 2025-06-16 10:49 ` Bruce Richardson
  2 siblings, 0 replies; 4+ messages in thread
From: Bruce Richardson @ 2025-06-16 10:49 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, Chengwen Feng

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-06-16 10:50 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-16 10:49 [PATCH 0/3] argparse: improve handling of multi-instance args Bruce Richardson
2025-06-16 10:49 ` [PATCH 1/3] argparse: track parsed arguments internally Bruce Richardson
2025-06-16 10:49 ` [PATCH 2/3] argparse: mark parameter struct as const Bruce Richardson
2025-06-16 10:49 ` [PATCH 3/3] test/argparse: add test for repeated arguments Bruce Richardson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).