[PATCH 0/4] refine argparse library

[PATCH 0/4] refine argparse library

[Chengwen Feng]

I found a couple of issues when I revisited the argparse_autotest
output, so got this patchset.

Chengwen Feng (4):
  argparse: refine error message
  argparse: remove dead code
  argparse: fix argument flags operate as uint32 type
  test/argparse: refine testcases

 app/test/test_argparse.c    | 63 +++++++++++++++++++------------------
 lib/argparse/rte_argparse.c | 41 +++++++++++-------------
 2 files changed, 52 insertions(+), 52 deletions(-)

-- 
2.17.1

[PATCH 1/4] argparse: refine error message

[Chengwen Feng]

This patch refines the error message.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/argparse/rte_argparse.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 2d953f1694..48738cd07b 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -67,7 +67,7 @@ verify_arg_name(const struct rte_argparse_arg *arg)
 			return -EINVAL;
 		}
 		if (arg->name_long[1] != '-') {
-			ARGPARSE_LOG(ERR, "optional long name %s must only start with '--'",
+			ARGPARSE_LOG(ERR, "optional long name %s doesn't start with '--'",
 				     arg->name_long);
 			return -EINVAL;
 		}
@@ -101,7 +101,7 @@ static int
 verify_arg_help(const struct rte_argparse_arg *arg)
 {
 	if (arg->help == NULL) {
-		ARGPARSE_LOG(ERR, "argument %s must have help info!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s doesn't have help info!", arg->name_long);
 		return -EINVAL;
 	}
 
@@ -116,13 +116,13 @@ verify_arg_has_val(const struct rte_argparse_arg *arg)
 	if (is_arg_positional(arg)) {
 		if (has_val == RTE_ARGPARSE_ARG_REQUIRED_VALUE)
 			return 0;
-		ARGPARSE_LOG(ERR, "argument %s is positional, should has zero or required-val!",
+		ARGPARSE_LOG(ERR, "argument %s is positional, must config required-val!",
 			     arg->name_long);
 		return -EINVAL;
 	}
 
 	if (has_val == 0) {
-		ARGPARSE_LOG(ERR, "argument %s is optional, has-val config wrong!",
+		ARGPARSE_LOG(ERR, "argument %s is optional, has-value config wrong!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -140,13 +140,13 @@ verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 
 	if (arg->val_saver == NULL) {
 		if (val_type != 0) {
-			ARGPARSE_LOG(ERR, "argument %s parse by callback, val-type must be zero!",
+			ARGPARSE_LOG(ERR, "argument %s parsed by callback, value-type should not be set!",
 				     arg->name_long);
 			return -EINVAL;
 		}
 
 		if (obj->callback == NULL) {
-			ARGPARSE_LOG(ERR, "argument %s parse by callback, but callback is NULL!",
+			ARGPARSE_LOG(ERR, "argument %s parsed by callback, but callback is NULL!",
 				     arg->name_long);
 			return -EINVAL;
 		}
@@ -155,12 +155,12 @@ verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 	}
 
 	if (val_type == 0 || val_type >= cmp_max) {
-		ARGPARSE_LOG(ERR, "argument %s val-type config wrong!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s value-type config wrong!", arg->name_long);
 		return -EINVAL;
 	}
 
 	if (has_val == RTE_ARGPARSE_ARG_REQUIRED_VALUE && arg->val_set != NULL) {
-		ARGPARSE_LOG(ERR, "argument %s has required value, val-set should be NULL!",
+		ARGPARSE_LOG(ERR, "argument %s has required value, value-set should be NULL!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -175,7 +175,8 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 	uint32_t unused_bits = arg_attr_unused_bits(arg);
 
 	if (unused_bits != 0) {
-		ARGPARSE_LOG(ERR, "argument %s flags set wrong!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s flags unused bits should not be set!",
+			     arg->name_long);
 		return -EINVAL;
 	}
 
@@ -189,7 +190,7 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 	}
 
 	if (arg->val_saver != NULL) {
-		ARGPARSE_LOG(ERR, "argument %s could occur multiple times, should use callback to parse!",
+		ARGPARSE_LOG(ERR, "argument %s supports multiple times, should use callback to parse!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -536,8 +537,10 @@ parse_arg_autosave(struct rte_argparse_arg *arg, const char *value)
 	return ret;
 }
 
+/* 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, struct rte_argparse_arg *arg, char *value)
+parse_arg_val(struct rte_argparse *obj, const char *arg_name,
+	      struct rte_argparse_arg *arg, char *value)
 {
 	int ret;
 
@@ -546,7 +549,7 @@ parse_arg_val(struct rte_argparse *obj, struct rte_argparse_arg *arg, char *valu
 	else
 		ret = parse_arg_autosave(arg, value);
 	if (ret != 0) {
-		ARGPARSE_LOG(ERR, "argument %s parse value fail!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s parse value fail!", arg_name);
 		return ret;
 	}
 
@@ -582,7 +585,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 				return -EINVAL;
 			}
 			arg = find_position_arg(obj, position_index);
-			ret = parse_arg_val(obj, arg, curr_argv);
+			ret = parse_arg_val(obj, arg->name_long, arg, curr_argv);
 			if (ret != 0)
 				return ret;
 			continue;
@@ -629,7 +632,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 			/* Do nothing, because it's optional value, only support arg=val or arg. */
 		}
 
-		ret = parse_arg_val(obj, arg, value);
+		ret = parse_arg_val(obj, arg_name, arg, value);
 		if (ret != 0)
 			return ret;
 
-- 
2.17.1

[PATCH 2/4] argparse: remove dead code

[Chengwen Feng]

The judgement "obj->callback == NULL" is dead code which can't be
reached, because verify_arg_saver() already make sure obj->callback
must not be NULL when arg->val_saver is NULL.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c    | 7 -------
 lib/argparse/rte_argparse.c | 6 ------
 2 files changed, 13 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index df11a129ba..c98bcee56d 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -288,13 +288,6 @@ test_argparse_invalid_arg_flags(void)
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
-	obj = test_argparse_init_obj();
-	obj->args[0].val_saver = NULL;
-	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_SUPPORT_MULTI;
-	obj->callback = NULL;
-	ret = rte_argparse_parse(obj, default_argc, default_argv);
-	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
-
 	return 0;
 }
 
diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 48738cd07b..6e890cdc0d 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -195,12 +195,6 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 		return -EINVAL;
 	}
 
-	if (obj->callback == NULL) {
-		ARGPARSE_LOG(ERR, "argument %s should use callback to parse, but callback is NULL!",
-			     arg->name_long);
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH 3/4] argparse: fix argument flags operate as uint32 type

[Chengwen Feng]

The struct rte_argparse_arg's flags was 64bit type, uint64_t should be
used instead of uint32_t where the operation happened.

Also, the flags' bit16 was also unused, so don't test bit16 in testcase
test_argparse_invalid_arg_flags.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Fixes: 31ed9f9f43bb ("argparse: parse parameters")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c    | 16 ++++++++--------
 lib/argparse/rte_argparse.c |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index c98bcee56d..708a575e16 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -188,7 +188,7 @@ test_argparse_invalid_arg_help(void)
 static int
 test_argparse_invalid_has_val(void)
 {
-	uint32_t set_mask[] = { 0,
+	uint64_t set_mask[] = { 0,
 				RTE_ARGPARSE_ARG_NO_VALUE,
 				RTE_ARGPARSE_ARG_OPTIONAL_VALUE
 			      };
@@ -197,7 +197,7 @@ test_argparse_invalid_has_val(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	obj->args[0].flags &= ~0x3u;
+	obj->args[0].flags &= ~0x3ull;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
@@ -205,7 +205,7 @@ test_argparse_invalid_has_val(void)
 		obj = test_argparse_init_obj();
 		obj->args[0].name_long = "abc";
 		obj->args[0].name_short = NULL;
-		obj->args[0].flags &= ~0x3u;
+		obj->args[0].flags &= ~0x3ull;
 		obj->args[0].flags |= set_mask[index];
 		ret = rte_argparse_parse(obj, default_argc, default_argv);
 		TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
@@ -269,7 +269,7 @@ test_argparse_invalid_arg_flags(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	obj->args[0].flags |= ~0x107FFu;
+	obj->args[0].flags |= ~0x7FFull;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
@@ -337,7 +337,7 @@ test_argparse_invalid_option(void)
 static int
 test_argparse_opt_autosave_parse_int_of_no_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_NO_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_NO_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[2];
@@ -369,7 +369,7 @@ test_argparse_opt_autosave_parse_int_of_no_val(void)
 static int
 test_argparse_opt_autosave_parse_int_of_required_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[3];
@@ -410,7 +410,7 @@ test_argparse_opt_autosave_parse_int_of_required_val(void)
 static int
 test_argparse_opt_autosave_parse_int_of_optional_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[2];
@@ -645,7 +645,7 @@ test_argparse_opt_callback_parse_int_of_optional_val(void)
 static int
 test_argparse_pos_autosave_parse_int(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[3];
diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 6e890cdc0d..9e799a16c5 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -50,7 +50,7 @@ arg_attr_flag_multi(const struct rte_argparse_arg *arg)
 	return RTE_FIELD_GET64(ARG_ATTR_SUPPORT_MULTI_MASK, arg->flags);
 }
 
-static inline uint32_t
+static inline uint64_t
 arg_attr_unused_bits(const struct rte_argparse_arg *arg)
 {
 #define USED_BIT_MASK	(ARG_ATTR_HAS_VAL_MASK | ARG_ATTR_VAL_TYPE_MASK | \
@@ -172,7 +172,7 @@ static int
 verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 {
 	const struct rte_argparse_arg *arg = &obj->args[index];
-	uint32_t unused_bits = arg_attr_unused_bits(arg);
+	uint64_t unused_bits = arg_attr_unused_bits(arg);
 
 	if (unused_bits != 0) {
 		ARGPARSE_LOG(ERR, "argument %s flags unused bits should not be set!",
-- 
2.17.1

[PATCH 4/4] test/argparse: refine testcases

[Chengwen Feng]

Refine testcases, including:
1. add testcase comment.
2. argv[0] should set obj->prog_name.
3. set val_set as NULL in test_argparse_invalid_arg_flags, let it
test to the specified code logic.
4. enable index verification in opt_callback_parse_int_of_no_val.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Fixes: 31ed9f9f43bb ("argparse: parse parameters")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index 708a575e16..5be9552aae 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -196,11 +196,13 @@ test_argparse_invalid_has_val(void)
 	uint32_t index;
 	int ret;
 
+	/* test optional arg don't config has-value. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags &= ~0x3ull;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test positional arg don't config required-value. */
 	for (index = 0; index < RTE_DIM(set_mask); index++) {
 		obj = test_argparse_init_obj();
 		obj->args[0].name_long = "abc";
@@ -268,21 +270,24 @@ test_argparse_invalid_arg_flags(void)
 	struct rte_argparse *obj;
 	int ret;
 
+	/* test set unused bits. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags |= ~0x7FFull;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test positional arg should not config multiple.  */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "positional";
 	obj->args[0].name_short = NULL;
 	obj->args[0].val_saver = (void *)1;
-	obj->args[0].val_set = (void *)1;
+	obj->args[0].val_set = NULL;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT |
 			     RTE_ARGPARSE_ARG_SUPPORT_MULTI;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test optional arg enabled multiple but prased by autosave. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags |= RTE_ARGPARSE_ARG_SUPPORT_MULTI;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
@@ -320,13 +325,13 @@ test_argparse_invalid_option(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--invalid");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
 	obj = test_argparse_init_obj();
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("invalid");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
@@ -350,7 +355,7 @@ test_argparse_opt_autosave_parse_int_of_no_val(void)
 	obj->args[0].val_set = (void *)100;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -382,7 +387,7 @@ test_argparse_opt_autosave_parse_int_of_required_val(void)
 	obj->args[0].val_set = NULL;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	argv[2] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -416,6 +421,7 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 	char *argv[2];
 	int ret;
 
+	/* test without value. */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "--test-long";
 	obj->args[0].name_short = "-t";
@@ -423,7 +429,7 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 	obj->args[0].val_set = (void *)100;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -467,7 +473,8 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 static int
 opt_callback_parse_int_of_no_val(uint32_t index, const char *value, void *opaque)
 {
-	RTE_SET_USED(index);
+	if (index != 1)
+		return -EINVAL;
 	if (value != NULL)
 		return -EINVAL;
 	*(int *)opaque = 100;
@@ -488,10 +495,10 @@ test_argparse_opt_callback_parse_int_of_no_val(void)
 	obj->args[0].name_long = "--test-long";
 	obj->args[0].name_short = "-t";
 	obj->args[0].val_saver = NULL;
-	obj->args[0].val_set = (void *)100;
+	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_NO_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -542,7 +549,7 @@ test_argparse_opt_callback_parse_int_of_required_val(void)
 	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	argv[2] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -606,7 +613,7 @@ test_argparse_opt_callback_parse_int_of_optional_val(void)
 	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -651,6 +658,7 @@ test_argparse_pos_autosave_parse_int(void)
 	char *argv[3];
 	int ret;
 
+	/* test positional autosave parse successful. */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "test-long";
 	obj->args[0].name_short = NULL;
@@ -658,19 +666,20 @@ test_argparse_pos_autosave_parse_int(void)
 	obj->args[0].val_set = NULL;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
 	TEST_ASSERT(val_saver == 100, "Argparse parse expect success!");
 
+	/* test positional autosave parse failed. */
 	obj->args[0].flags = flags;
 	val_saver = 0;
 	argv[1] = test_strdup("100a");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
-	/* test over position parameters. */
+	/* test too much position parameters. */
 	obj->args[0].flags = flags;
 	argv[1] = test_strdup("100");
 	argv[2] = test_strdup("200");
@@ -708,6 +717,7 @@ test_argparse_pos_callback_parse_int(void)
 	char *argv[3];
 	int ret;
 
+	/* test positional callback parse successful. */
 	obj = test_argparse_init_obj();
 	obj->callback = pos_callback_parse_int;
 	obj->opaque = (void *)val_saver;
@@ -722,7 +732,7 @@ test_argparse_pos_callback_parse_int(void)
 	obj->args[1].val_set = (void *)2;
 	obj->args[1].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[2].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("100");
 	argv[2] = test_strdup("200");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -730,7 +740,7 @@ test_argparse_pos_callback_parse_int(void)
 	TEST_ASSERT(val_saver[1] == 100, "Argparse parse expect success!");
 	TEST_ASSERT(val_saver[2] == 200, "Argparse parse expect success!");
 
-	/* test callback return failed. */
+	/* test positional callback parse failed. */
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[1].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	argv[2] = test_strdup("200a");
-- 
2.17.1

Re: [PATCH 3/4] argparse: fix argument flags operate as uint32 type

[David Marchand]

On Tue, Feb 20, 2024 at 2:16 PM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> The struct rte_argparse_arg's flags was 64bit type, uint64_t should be
> used instead of uint32_t where the operation happened.

Something is strange.
An enum in C is represented as an int.

Plus, this enum type is not used anywhere:
lib/argparse/rte_argparse.h:enum rte_argparse_flag {
lib/argparse/rte_argparse.h:    /** @see rte_argparse_flag */

I understand the flags are a bitmask.
So please remove this enum and define macros instead.


>
> Also, the flags' bit16 was also unused, so don't test bit16 in testcase
> test_argparse_invalid_arg_flags.
>
> Fixes: 6c5c6571601c ("argparse: verify argument config")
> Fixes: 31ed9f9f43bb ("argparse: parse parameters")
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  app/test/test_argparse.c    | 16 ++++++++--------
>  lib/argparse/rte_argparse.c |  4 ++--
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
> index c98bcee56d..708a575e16 100644
> --- a/app/test/test_argparse.c
> +++ b/app/test/test_argparse.c
> @@ -188,7 +188,7 @@ test_argparse_invalid_arg_help(void)
>  static int
>  test_argparse_invalid_has_val(void)
>  {
> -       uint32_t set_mask[] = { 0,
> +       uint64_t set_mask[] = { 0,
>                                 RTE_ARGPARSE_ARG_NO_VALUE,
>                                 RTE_ARGPARSE_ARG_OPTIONAL_VALUE
>                               };
> @@ -197,7 +197,7 @@ test_argparse_invalid_has_val(void)
>         int ret;
>
>         obj = test_argparse_init_obj();
> -       obj->args[0].flags &= ~0x3u;
> +       obj->args[0].flags &= ~0x3ull;

If flags is a uint64_t, use RTE_BIT64().


I don't know the argparse API, but why do we need this hardcoded (and
hard to understand) ~3 value?
Can it be expressed with the flags defined in the API?


>         ret = rte_argparse_parse(obj, default_argc, default_argv);
>         TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
>
> @@ -205,7 +205,7 @@ test_argparse_invalid_has_val(void)
>                 obj = test_argparse_init_obj();
>                 obj->args[0].name_long = "abc";
>                 obj->args[0].name_short = NULL;
> -               obj->args[0].flags &= ~0x3u;
> +               obj->args[0].flags &= ~0x3ull;
>                 obj->args[0].flags |= set_mask[index];
>                 ret = rte_argparse_parse(obj, default_argc, default_argv);
>                 TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
> @@ -269,7 +269,7 @@ test_argparse_invalid_arg_flags(void)
>         int ret;
>
>         obj = test_argparse_init_obj();
> -       obj->args[0].flags |= ~0x107FFu;
> +       obj->args[0].flags |= ~0x7FFull;

Same comments as above.


-- 
David Marchand

[PATCH v2 0/5] refine argparse library

[Chengwen Feng]

I found a couple of issues when I revisited the argparse_autotest
output, so got this patchset.

Chengwen Feng (5):
  argparse: refine error message
  argparse: remove dead code
  argparse: replace flag enum with marco
  argparse: fix argument flags operate as uint32 type
  test/argparse: refine testcases

---
v2: address David Marchand's comment:
- replace flag enum with marco.
- replace flag's hardcode with macro in test_argparse.c.

 app/test/test_argparse.c    | 68 ++++++++++++++++++--------------
 lib/argparse/rte_argparse.c | 41 +++++++++----------
 lib/argparse/rte_argparse.h | 78 ++++++++++++++++---------------------
 3 files changed, 90 insertions(+), 97 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] argparse: refine error message

[Chengwen Feng]

This patch refines the error message.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/argparse/rte_argparse.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 2d953f1694..48738cd07b 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -67,7 +67,7 @@ verify_arg_name(const struct rte_argparse_arg *arg)
 			return -EINVAL;
 		}
 		if (arg->name_long[1] != '-') {
-			ARGPARSE_LOG(ERR, "optional long name %s must only start with '--'",
+			ARGPARSE_LOG(ERR, "optional long name %s doesn't start with '--'",
 				     arg->name_long);
 			return -EINVAL;
 		}
@@ -101,7 +101,7 @@ static int
 verify_arg_help(const struct rte_argparse_arg *arg)
 {
 	if (arg->help == NULL) {
-		ARGPARSE_LOG(ERR, "argument %s must have help info!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s doesn't have help info!", arg->name_long);
 		return -EINVAL;
 	}
 
@@ -116,13 +116,13 @@ verify_arg_has_val(const struct rte_argparse_arg *arg)
 	if (is_arg_positional(arg)) {
 		if (has_val == RTE_ARGPARSE_ARG_REQUIRED_VALUE)
 			return 0;
-		ARGPARSE_LOG(ERR, "argument %s is positional, should has zero or required-val!",
+		ARGPARSE_LOG(ERR, "argument %s is positional, must config required-val!",
 			     arg->name_long);
 		return -EINVAL;
 	}
 
 	if (has_val == 0) {
-		ARGPARSE_LOG(ERR, "argument %s is optional, has-val config wrong!",
+		ARGPARSE_LOG(ERR, "argument %s is optional, has-value config wrong!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -140,13 +140,13 @@ verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 
 	if (arg->val_saver == NULL) {
 		if (val_type != 0) {
-			ARGPARSE_LOG(ERR, "argument %s parse by callback, val-type must be zero!",
+			ARGPARSE_LOG(ERR, "argument %s parsed by callback, value-type should not be set!",
 				     arg->name_long);
 			return -EINVAL;
 		}
 
 		if (obj->callback == NULL) {
-			ARGPARSE_LOG(ERR, "argument %s parse by callback, but callback is NULL!",
+			ARGPARSE_LOG(ERR, "argument %s parsed by callback, but callback is NULL!",
 				     arg->name_long);
 			return -EINVAL;
 		}
@@ -155,12 +155,12 @@ verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 	}
 
 	if (val_type == 0 || val_type >= cmp_max) {
-		ARGPARSE_LOG(ERR, "argument %s val-type config wrong!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s value-type config wrong!", arg->name_long);
 		return -EINVAL;
 	}
 
 	if (has_val == RTE_ARGPARSE_ARG_REQUIRED_VALUE && arg->val_set != NULL) {
-		ARGPARSE_LOG(ERR, "argument %s has required value, val-set should be NULL!",
+		ARGPARSE_LOG(ERR, "argument %s has required value, value-set should be NULL!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -175,7 +175,8 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 	uint32_t unused_bits = arg_attr_unused_bits(arg);
 
 	if (unused_bits != 0) {
-		ARGPARSE_LOG(ERR, "argument %s flags set wrong!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s flags unused bits should not be set!",
+			     arg->name_long);
 		return -EINVAL;
 	}
 
@@ -189,7 +190,7 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 	}
 
 	if (arg->val_saver != NULL) {
-		ARGPARSE_LOG(ERR, "argument %s could occur multiple times, should use callback to parse!",
+		ARGPARSE_LOG(ERR, "argument %s supports multiple times, should use callback to parse!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -536,8 +537,10 @@ parse_arg_autosave(struct rte_argparse_arg *arg, const char *value)
 	return ret;
 }
 
+/* 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, struct rte_argparse_arg *arg, char *value)
+parse_arg_val(struct rte_argparse *obj, const char *arg_name,
+	      struct rte_argparse_arg *arg, char *value)
 {
 	int ret;
 
@@ -546,7 +549,7 @@ parse_arg_val(struct rte_argparse *obj, struct rte_argparse_arg *arg, char *valu
 	else
 		ret = parse_arg_autosave(arg, value);
 	if (ret != 0) {
-		ARGPARSE_LOG(ERR, "argument %s parse value fail!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s parse value fail!", arg_name);
 		return ret;
 	}
 
@@ -582,7 +585,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 				return -EINVAL;
 			}
 			arg = find_position_arg(obj, position_index);
-			ret = parse_arg_val(obj, arg, curr_argv);
+			ret = parse_arg_val(obj, arg->name_long, arg, curr_argv);
 			if (ret != 0)
 				return ret;
 			continue;
@@ -629,7 +632,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 			/* Do nothing, because it's optional value, only support arg=val or arg. */
 		}
 
-		ret = parse_arg_val(obj, arg, value);
+		ret = parse_arg_val(obj, arg_name, arg, value);
 		if (ret != 0)
 			return ret;
 
-- 
2.17.1

[PATCH v2 2/5] argparse: remove dead code

[Chengwen Feng]

The judgement "obj->callback == NULL" is dead code which can't be
reached, because verify_arg_saver() already make sure obj->callback
must not be NULL when arg->val_saver is NULL.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c    | 7 -------
 lib/argparse/rte_argparse.c | 6 ------
 2 files changed, 13 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index df11a129ba..c98bcee56d 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -288,13 +288,6 @@ test_argparse_invalid_arg_flags(void)
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
-	obj = test_argparse_init_obj();
-	obj->args[0].val_saver = NULL;
-	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_SUPPORT_MULTI;
-	obj->callback = NULL;
-	ret = rte_argparse_parse(obj, default_argc, default_argv);
-	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
-
 	return 0;
 }
 
diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 48738cd07b..6e890cdc0d 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -195,12 +195,6 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 		return -EINVAL;
 	}
 
-	if (obj->callback == NULL) {
-		ARGPARSE_LOG(ERR, "argument %s should use callback to parse, but callback is NULL!",
-			     arg->name_long);
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH v2 3/5] argparse: replace flag enum with marco

[Chengwen Feng]

The enum rte_argparse_flag's value is u64, but an enum in C is
represented as an int. This commit replace these enum values with
macro.

Fixes: e3e579f5bab5 ("argparse: introduce argparse library")
Fixes: 5357c248c960 ("argparse: parse unsigned integers")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/argparse/rte_argparse.h | 78 ++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index 47e231bef9..a6a7790cb4 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -37,52 +37,40 @@
 extern "C" {
 #endif
 
+/**@{@name Flag definition (in bitmask form) for an argument
+ *
+ * @note Bits[0~1] represent the argument whether has value,
+ * bits[2~9] represent the value type which used when autosave.
+ *
+ * @see struct rte_argparse_arg::flags
+ */
+/** The argument has no value. */
+#define RTE_ARGPARSE_ARG_NO_VALUE       RTE_SHIFT_VAL64(1, 0)
+/** The argument must have a value. */
+#define RTE_ARGPARSE_ARG_REQUIRED_VALUE RTE_SHIFT_VAL64(2, 0)
+/** The argument has optional value. */
+#define RTE_ARGPARSE_ARG_OPTIONAL_VALUE RTE_SHIFT_VAL64(3, 0)
+/** The argument's value is int type. */
+#define RTE_ARGPARSE_ARG_VALUE_INT      RTE_SHIFT_VAL64(1, 2)
+/** The argument's value is uint8 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U8       RTE_SHIFT_VAL64(2, 2)
+/** The argument's value is uint16 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U16      RTE_SHIFT_VAL64(3, 2)
+/** The argument's value is uint32 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U32      RTE_SHIFT_VAL64(4, 2)
+/** The argument's value is uint64 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U64      RTE_SHIFT_VAL64(5, 2)
+/** Max value type. */
+#define RTE_ARGPARSE_ARG_VALUE_MAX      RTE_SHIFT_VAL64(6, 2)
 /**
- * Flag definition (in bitmask form) for an argument.
+ * Flag for that argument support occur multiple times.
+ * This flag can be set only when the argument is optional.
+ * When this flag is set, the callback type must be used for parsing.
  */
-enum rte_argparse_flag {
-	/*
-	 * Bits 0-1: represent the argument whether has value.
-	 */
-
-	/** The argument has no value. */
-	RTE_ARGPARSE_ARG_NO_VALUE = RTE_SHIFT_VAL64(1, 0),
-	/** The argument must have a value. */
-	RTE_ARGPARSE_ARG_REQUIRED_VALUE = RTE_SHIFT_VAL64(2, 0),
-	/** The argument has optional value. */
-	RTE_ARGPARSE_ARG_OPTIONAL_VALUE = RTE_SHIFT_VAL64(3, 0),
-
-
-	/*
-	 * Bits 2-9: represent the value type which used when autosave
-	 */
-
-	/** The argument's value is int type. */
-	RTE_ARGPARSE_ARG_VALUE_INT = RTE_SHIFT_VAL64(1, 2),
-	/** The argument's value is uint8 type. */
-	RTE_ARGPARSE_ARG_VALUE_U8 = RTE_SHIFT_VAL64(2, 2),
-	/** The argument's value is uint16 type. */
-	RTE_ARGPARSE_ARG_VALUE_U16 = RTE_SHIFT_VAL64(3, 2),
-	/** The argument's value is uint32 type. */
-	RTE_ARGPARSE_ARG_VALUE_U32 = RTE_SHIFT_VAL64(4, 2),
-	/** The argument's value is uint64 type. */
-	RTE_ARGPARSE_ARG_VALUE_U64 = RTE_SHIFT_VAL64(5, 2),
-	/** Max value type. */
-	RTE_ARGPARSE_ARG_VALUE_MAX = RTE_SHIFT_VAL64(6, 2),
-
-
-	/**
-	 * Bit 10: flag for that argument support occur multiple times.
-	 * This flag can be set only when the argument is optional.
-	 * When this flag is set, the callback type must be used for parsing.
-	 */
-	RTE_ARGPARSE_ARG_SUPPORT_MULTI = RTE_BIT64(10),
-
-	/**
-	 * Bits 48-63: reserved for this library implementation usage.
-	 */
-	RTE_ARGPARSE_ARG_RESERVED_FIELD = RTE_GENMASK64(63, 48),
-};
+#define RTE_ARGPARSE_ARG_SUPPORT_MULTI  RTE_BIT64(10)
+/** Reserved for this library implementation usage. */
+#define RTE_ARGPARSE_ARG_RESERVED_FIELD RTE_GENMASK64(63, 48)
+/**@}*/
 
 /**
  * A structure used to hold argument's configuration.
@@ -126,7 +114,7 @@ struct rte_argparse_arg {
 	 */
 	void *val_set;
 
-	/** @see rte_argparse_flag */
+	/** Flag definition (RTE_ARGPARSE_ARG_*) for the argument. */
 	uint64_t flags;
 };
 
-- 
2.17.1

[PATCH v2 4/5] argparse: fix argument flags operate as uint32 type

[Chengwen Feng]

The struct rte_argparse_arg's flags was 64bit type, uint64_t should be
used instead of uint32_t where the operation happened.

Also, the flags' bit16 was also unused, so don't test bit16 in testcase
test_argparse_invalid_arg_flags.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Fixes: 31ed9f9f43bb ("argparse: parse parameters")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c    | 21 +++++++++++++--------
 lib/argparse/rte_argparse.c |  4 ++--
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index c98bcee56d..7cecd7116c 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -16,6 +16,9 @@ static char *default_argv[1];
 static char *strdup_store_array[MAX_STRDUP_STORE_NUM];
 static uint32_t strdup_store_index;
 
+#define TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK	RTE_SHIFT_VAL64(3, 0)
+#define TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK	RTE_SHIFT_VAL64(255, 2)
+
 /*
  * Define strdup wrapper.
  * 1. Mainly to fix compile error "warning: assignment discards 'const'
@@ -188,7 +191,7 @@ test_argparse_invalid_arg_help(void)
 static int
 test_argparse_invalid_has_val(void)
 {
-	uint32_t set_mask[] = { 0,
+	uint64_t set_mask[] = { 0,
 				RTE_ARGPARSE_ARG_NO_VALUE,
 				RTE_ARGPARSE_ARG_OPTIONAL_VALUE
 			      };
@@ -197,7 +200,7 @@ test_argparse_invalid_has_val(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	obj->args[0].flags &= ~0x3u;
+	obj->args[0].flags &= ~TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
@@ -205,7 +208,7 @@ test_argparse_invalid_has_val(void)
 		obj = test_argparse_init_obj();
 		obj->args[0].name_long = "abc";
 		obj->args[0].name_short = NULL;
-		obj->args[0].flags &= ~0x3u;
+		obj->args[0].flags &= ~TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK;
 		obj->args[0].flags |= set_mask[index];
 		ret = rte_argparse_parse(obj, default_argc, default_argv);
 		TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
@@ -269,7 +272,9 @@ test_argparse_invalid_arg_flags(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	obj->args[0].flags |= ~0x107FFu;
+	obj->args[0].flags |= ~(TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK |
+				TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK |
+				RTE_ARGPARSE_ARG_SUPPORT_MULTI);
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
@@ -337,7 +342,7 @@ test_argparse_invalid_option(void)
 static int
 test_argparse_opt_autosave_parse_int_of_no_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_NO_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_NO_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[2];
@@ -369,7 +374,7 @@ test_argparse_opt_autosave_parse_int_of_no_val(void)
 static int
 test_argparse_opt_autosave_parse_int_of_required_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[3];
@@ -410,7 +415,7 @@ test_argparse_opt_autosave_parse_int_of_required_val(void)
 static int
 test_argparse_opt_autosave_parse_int_of_optional_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[2];
@@ -645,7 +650,7 @@ test_argparse_opt_callback_parse_int_of_optional_val(void)
 static int
 test_argparse_pos_autosave_parse_int(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[3];
diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 6e890cdc0d..9e799a16c5 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -50,7 +50,7 @@ arg_attr_flag_multi(const struct rte_argparse_arg *arg)
 	return RTE_FIELD_GET64(ARG_ATTR_SUPPORT_MULTI_MASK, arg->flags);
 }
 
-static inline uint32_t
+static inline uint64_t
 arg_attr_unused_bits(const struct rte_argparse_arg *arg)
 {
 #define USED_BIT_MASK	(ARG_ATTR_HAS_VAL_MASK | ARG_ATTR_VAL_TYPE_MASK | \
@@ -172,7 +172,7 @@ static int
 verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 {
 	const struct rte_argparse_arg *arg = &obj->args[index];
-	uint32_t unused_bits = arg_attr_unused_bits(arg);
+	uint64_t unused_bits = arg_attr_unused_bits(arg);
 
 	if (unused_bits != 0) {
 		ARGPARSE_LOG(ERR, "argument %s flags unused bits should not be set!",
-- 
2.17.1

[PATCH v2 5/5] test/argparse: refine testcases

[Chengwen Feng]

Refine testcases, including:
1. add testcase comment.
2. argv[0] should set obj->prog_name.
3. set val_set as NULL in test_argparse_invalid_arg_flags, let it
test to the specified code logic.
4. enable index verification in opt_callback_parse_int_of_no_val.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Fixes: 31ed9f9f43bb ("argparse: parse parameters")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index 7cecd7116c..09a6366b9f 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -199,11 +199,13 @@ test_argparse_invalid_has_val(void)
 	uint32_t index;
 	int ret;
 
+	/* test optional arg don't config has-value. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags &= ~TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test positional arg don't config required-value. */
 	for (index = 0; index < RTE_DIM(set_mask); index++) {
 		obj = test_argparse_init_obj();
 		obj->args[0].name_long = "abc";
@@ -271,6 +273,7 @@ test_argparse_invalid_arg_flags(void)
 	struct rte_argparse *obj;
 	int ret;
 
+	/* test set unused bits. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags |= ~(TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK |
 				TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK |
@@ -278,16 +281,18 @@ test_argparse_invalid_arg_flags(void)
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test positional arg should not config multiple.  */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "positional";
 	obj->args[0].name_short = NULL;
 	obj->args[0].val_saver = (void *)1;
-	obj->args[0].val_set = (void *)1;
+	obj->args[0].val_set = NULL;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT |
 			     RTE_ARGPARSE_ARG_SUPPORT_MULTI;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test optional arg enabled multiple but prased by autosave. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags |= RTE_ARGPARSE_ARG_SUPPORT_MULTI;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
@@ -325,13 +330,13 @@ test_argparse_invalid_option(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--invalid");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
 	obj = test_argparse_init_obj();
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("invalid");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
@@ -355,7 +360,7 @@ test_argparse_opt_autosave_parse_int_of_no_val(void)
 	obj->args[0].val_set = (void *)100;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -387,7 +392,7 @@ test_argparse_opt_autosave_parse_int_of_required_val(void)
 	obj->args[0].val_set = NULL;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	argv[2] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -421,6 +426,7 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 	char *argv[2];
 	int ret;
 
+	/* test without value. */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "--test-long";
 	obj->args[0].name_short = "-t";
@@ -428,7 +434,7 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 	obj->args[0].val_set = (void *)100;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -472,7 +478,8 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 static int
 opt_callback_parse_int_of_no_val(uint32_t index, const char *value, void *opaque)
 {
-	RTE_SET_USED(index);
+	if (index != 1)
+		return -EINVAL;
 	if (value != NULL)
 		return -EINVAL;
 	*(int *)opaque = 100;
@@ -493,10 +500,10 @@ test_argparse_opt_callback_parse_int_of_no_val(void)
 	obj->args[0].name_long = "--test-long";
 	obj->args[0].name_short = "-t";
 	obj->args[0].val_saver = NULL;
-	obj->args[0].val_set = (void *)100;
+	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_NO_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -547,7 +554,7 @@ test_argparse_opt_callback_parse_int_of_required_val(void)
 	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	argv[2] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -611,7 +618,7 @@ test_argparse_opt_callback_parse_int_of_optional_val(void)
 	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -656,6 +663,7 @@ test_argparse_pos_autosave_parse_int(void)
 	char *argv[3];
 	int ret;
 
+	/* test positional autosave parse successful. */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "test-long";
 	obj->args[0].name_short = NULL;
@@ -663,19 +671,20 @@ test_argparse_pos_autosave_parse_int(void)
 	obj->args[0].val_set = NULL;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
 	TEST_ASSERT(val_saver == 100, "Argparse parse expect success!");
 
+	/* test positional autosave parse failed. */
 	obj->args[0].flags = flags;
 	val_saver = 0;
 	argv[1] = test_strdup("100a");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
-	/* test over position parameters. */
+	/* test too much position parameters. */
 	obj->args[0].flags = flags;
 	argv[1] = test_strdup("100");
 	argv[2] = test_strdup("200");
@@ -713,6 +722,7 @@ test_argparse_pos_callback_parse_int(void)
 	char *argv[3];
 	int ret;
 
+	/* test positional callback parse successful. */
 	obj = test_argparse_init_obj();
 	obj->callback = pos_callback_parse_int;
 	obj->opaque = (void *)val_saver;
@@ -727,7 +737,7 @@ test_argparse_pos_callback_parse_int(void)
 	obj->args[1].val_set = (void *)2;
 	obj->args[1].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[2].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("100");
 	argv[2] = test_strdup("200");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -735,7 +745,7 @@ test_argparse_pos_callback_parse_int(void)
 	TEST_ASSERT(val_saver[1] == 100, "Argparse parse expect success!");
 	TEST_ASSERT(val_saver[2] == 200, "Argparse parse expect success!");
 
-	/* test callback return failed. */
+	/* test positional callback parse failed. */
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[1].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	argv[2] = test_strdup("200a");
-- 
2.17.1

Re: [PATCH 3/4] argparse: fix argument flags operate as uint32 type

[fengchengwen]

Hi David,

On 2024/3/7 1:34, David Marchand wrote:
> On Tue, Feb 20, 2024 at 2:16 PM Chengwen Feng <fengchengwen@huawei.com> wrote:
>>
>> The struct rte_argparse_arg's flags was 64bit type, uint64_t should be
>> used instead of uint32_t where the operation happened.
> 
> Something is strange.
> An enum in C is represented as an int.
> 
> Plus, this enum type is not used anywhere:
> lib/argparse/rte_argparse.h:enum rte_argparse_flag {
> lib/argparse/rte_argparse.h:    /** @see rte_argparse_flag */
> 
> I understand the flags are a bitmask.
> So please remove this enum and define macros instead.

Thanks for point it out, already send v2.

> 
> 
>>
>> Also, the flags' bit16 was also unused, so don't test bit16 in testcase
>> test_argparse_invalid_arg_flags.
>>
>> Fixes: 6c5c6571601c ("argparse: verify argument config")
>> Fixes: 31ed9f9f43bb ("argparse: parse parameters")
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  app/test/test_argparse.c    | 16 ++++++++--------
>>  lib/argparse/rte_argparse.c |  4 ++--
>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
>> index c98bcee56d..708a575e16 100644
>> --- a/app/test/test_argparse.c
>> +++ b/app/test/test_argparse.c
>> @@ -188,7 +188,7 @@ test_argparse_invalid_arg_help(void)
>>  static int
>>  test_argparse_invalid_has_val(void)
>>  {
>> -       uint32_t set_mask[] = { 0,
>> +       uint64_t set_mask[] = { 0,
>>                                 RTE_ARGPARSE_ARG_NO_VALUE,
>>                                 RTE_ARGPARSE_ARG_OPTIONAL_VALUE
>>                               };
>> @@ -197,7 +197,7 @@ test_argparse_invalid_has_val(void)
>>         int ret;
>>
>>         obj = test_argparse_init_obj();
>> -       obj->args[0].flags &= ~0x3u;
>> +       obj->args[0].flags &= ~0x3ull;
> 
> If flags is a uint64_t, use RTE_BIT64().
> 
> 
> I don't know the argparse API, but why do we need this hardcoded (and
> hard to understand) ~3 value?

The lowest two bits was a represent whether has value.

> Can it be expressed with the flags defined in the API?

Its not part of API, I defined a macro in test_argparse.c to express its meaning in v2.

> 
> 
>>         ret = rte_argparse_parse(obj, default_argc, default_argv);
>>         TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
>>
>> @@ -205,7 +205,7 @@ test_argparse_invalid_has_val(void)
>>                 obj = test_argparse_init_obj();
>>                 obj->args[0].name_long = "abc";
>>                 obj->args[0].name_short = NULL;
>> -               obj->args[0].flags &= ~0x3u;
>> +               obj->args[0].flags &= ~0x3ull;
>>                 obj->args[0].flags |= set_mask[index];
>>                 ret = rte_argparse_parse(obj, default_argc, default_argv);
>>                 TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
>> @@ -269,7 +269,7 @@ test_argparse_invalid_arg_flags(void)
>>         int ret;
>>
>>         obj = test_argparse_init_obj();
>> -       obj->args[0].flags |= ~0x107FFu;
>> +       obj->args[0].flags |= ~0x7FFull;
> 
> Same comments as above.

Done in v2, defined macros in test_argpase.c and express with multi-marco's or value.

Thanks

> 
> 

Re: [PATCH v2 3/5] argparse: replace flag enum with marco

[Tyler Retzlaff]

On Thu, Mar 07, 2024 at 01:07:40PM +0000, Chengwen Feng wrote:
> The enum rte_argparse_flag's value is u64, but an enum in C is
> represented as an int. This commit replace these enum values with
> macro.
> 
> Fixes: e3e579f5bab5 ("argparse: introduce argparse library")
> Fixes: 5357c248c960 ("argparse: parse unsigned integers")
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/argparse/rte_argparse.h | 78 ++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 45 deletions(-)
> 
> diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
> index 47e231bef9..a6a7790cb4 100644
> --- a/lib/argparse/rte_argparse.h
> +++ b/lib/argparse/rte_argparse.h
> @@ -37,52 +37,40 @@
>  extern "C" {
>  #endif
>  
> +/**@{@name Flag definition (in bitmask form) for an argument
> + *
> + * @note Bits[0~1] represent the argument whether has value,
> + * bits[2~9] represent the value type which used when autosave.
> + *
> + * @see struct rte_argparse_arg::flags
> + */
> +/** The argument has no value. */
> +#define RTE_ARGPARSE_ARG_NO_VALUE       RTE_SHIFT_VAL64(1, 0)
> +/** The argument must have a value. */
> +#define RTE_ARGPARSE_ARG_REQUIRED_VALUE RTE_SHIFT_VAL64(2, 0)
> +/** The argument has optional value. */
> +#define RTE_ARGPARSE_ARG_OPTIONAL_VALUE RTE_SHIFT_VAL64(3, 0)
> +/** The argument's value is int type. */
> +#define RTE_ARGPARSE_ARG_VALUE_INT      RTE_SHIFT_VAL64(1, 2)
> +/** The argument's value is uint8 type. */
> +#define RTE_ARGPARSE_ARG_VALUE_U8       RTE_SHIFT_VAL64(2, 2)
> +/** The argument's value is uint16 type. */
> +#define RTE_ARGPARSE_ARG_VALUE_U16      RTE_SHIFT_VAL64(3, 2)
> +/** The argument's value is uint32 type. */
> +#define RTE_ARGPARSE_ARG_VALUE_U32      RTE_SHIFT_VAL64(4, 2)
> +/** The argument's value is uint64 type. */
> +#define RTE_ARGPARSE_ARG_VALUE_U64      RTE_SHIFT_VAL64(5, 2)
> +/** Max value type. */
> +#define RTE_ARGPARSE_ARG_VALUE_MAX      RTE_SHIFT_VAL64(6, 2)

it was good to get rid of the enum but will the above macros expand to
signed or unsigned integer given the type of field could we make sure
they expand unsigned?

>  /**
> - * Flag definition (in bitmask form) for an argument.
> + * Flag for that argument support occur multiple times.
> + * This flag can be set only when the argument is optional.
> + * When this flag is set, the callback type must be used for parsing.
>   */
> -enum rte_argparse_flag {
> -	/*
> -	 * Bits 0-1: represent the argument whether has value.
> -	 */
> -
> -	/** The argument has no value. */
> -	RTE_ARGPARSE_ARG_NO_VALUE = RTE_SHIFT_VAL64(1, 0),
> -	/** The argument must have a value. */
> -	RTE_ARGPARSE_ARG_REQUIRED_VALUE = RTE_SHIFT_VAL64(2, 0),
> -	/** The argument has optional value. */
> -	RTE_ARGPARSE_ARG_OPTIONAL_VALUE = RTE_SHIFT_VAL64(3, 0),
> -
> -
> -	/*
> -	 * Bits 2-9: represent the value type which used when autosave
> -	 */
> -
> -	/** The argument's value is int type. */
> -	RTE_ARGPARSE_ARG_VALUE_INT = RTE_SHIFT_VAL64(1, 2),
> -	/** The argument's value is uint8 type. */
> -	RTE_ARGPARSE_ARG_VALUE_U8 = RTE_SHIFT_VAL64(2, 2),
> -	/** The argument's value is uint16 type. */
> -	RTE_ARGPARSE_ARG_VALUE_U16 = RTE_SHIFT_VAL64(3, 2),
> -	/** The argument's value is uint32 type. */
> -	RTE_ARGPARSE_ARG_VALUE_U32 = RTE_SHIFT_VAL64(4, 2),
> -	/** The argument's value is uint64 type. */
> -	RTE_ARGPARSE_ARG_VALUE_U64 = RTE_SHIFT_VAL64(5, 2),
> -	/** Max value type. */
> -	RTE_ARGPARSE_ARG_VALUE_MAX = RTE_SHIFT_VAL64(6, 2),
> -
> -
> -	/**
> -	 * Bit 10: flag for that argument support occur multiple times.
> -	 * This flag can be set only when the argument is optional.
> -	 * When this flag is set, the callback type must be used for parsing.
> -	 */
> -	RTE_ARGPARSE_ARG_SUPPORT_MULTI = RTE_BIT64(10),
> -
> -	/**
> -	 * Bits 48-63: reserved for this library implementation usage.
> -	 */
> -	RTE_ARGPARSE_ARG_RESERVED_FIELD = RTE_GENMASK64(63, 48),
> -};
> +#define RTE_ARGPARSE_ARG_SUPPORT_MULTI  RTE_BIT64(10)
> +/** Reserved for this library implementation usage. */
> +#define RTE_ARGPARSE_ARG_RESERVED_FIELD RTE_GENMASK64(63, 48)
> +/**@}*/
>  
>  /**
>   * A structure used to hold argument's configuration.
> @@ -126,7 +114,7 @@ struct rte_argparse_arg {
>  	 */
>  	void *val_set;
>  
> -	/** @see rte_argparse_flag */
> +	/** Flag definition (RTE_ARGPARSE_ARG_*) for the argument. */
>  	uint64_t flags;
>  };
>  
> -- 
> 2.17.1

Re: [PATCH v2 3/5] argparse: replace flag enum with marco

[David Marchand]

On Thu, Mar 7, 2024 at 6:43 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Thu, Mar 07, 2024 at 01:07:40PM +0000, Chengwen Feng wrote:
> > The enum rte_argparse_flag's value is u64, but an enum in C is
> > represented as an int. This commit replace these enum values with
> > macro.
> >
> > Fixes: e3e579f5bab5 ("argparse: introduce argparse library")
> > Fixes: 5357c248c960 ("argparse: parse unsigned integers")
> >
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > ---
> >  lib/argparse/rte_argparse.h | 78 ++++++++++++++++---------------------
> >  1 file changed, 33 insertions(+), 45 deletions(-)
> >
> > diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
> > index 47e231bef9..a6a7790cb4 100644
> > --- a/lib/argparse/rte_argparse.h
> > +++ b/lib/argparse/rte_argparse.h
> > @@ -37,52 +37,40 @@
> >  extern "C" {
> >  #endif
> >
> > +/**@{@name Flag definition (in bitmask form) for an argument
> > + *
> > + * @note Bits[0~1] represent the argument whether has value,
> > + * bits[2~9] represent the value type which used when autosave.
> > + *
> > + * @see struct rte_argparse_arg::flags
> > + */
> > +/** The argument has no value. */
> > +#define RTE_ARGPARSE_ARG_NO_VALUE       RTE_SHIFT_VAL64(1, 0)
> > +/** The argument must have a value. */
> > +#define RTE_ARGPARSE_ARG_REQUIRED_VALUE RTE_SHIFT_VAL64(2, 0)
> > +/** The argument has optional value. */
> > +#define RTE_ARGPARSE_ARG_OPTIONAL_VALUE RTE_SHIFT_VAL64(3, 0)
> > +/** The argument's value is int type. */
> > +#define RTE_ARGPARSE_ARG_VALUE_INT      RTE_SHIFT_VAL64(1, 2)
> > +/** The argument's value is uint8 type. */
> > +#define RTE_ARGPARSE_ARG_VALUE_U8       RTE_SHIFT_VAL64(2, 2)
> > +/** The argument's value is uint16 type. */
> > +#define RTE_ARGPARSE_ARG_VALUE_U16      RTE_SHIFT_VAL64(3, 2)
> > +/** The argument's value is uint32 type. */
> > +#define RTE_ARGPARSE_ARG_VALUE_U32      RTE_SHIFT_VAL64(4, 2)
> > +/** The argument's value is uint64 type. */
> > +#define RTE_ARGPARSE_ARG_VALUE_U64      RTE_SHIFT_VAL64(5, 2)
> > +/** Max value type. */
> > +#define RTE_ARGPARSE_ARG_VALUE_MAX      RTE_SHIFT_VAL64(6, 2)
>
> it was good to get rid of the enum but will the above macros expand to
> signed or unsigned integer given the type of field could we make sure
> they expand unsigned?

Afaiu, those should expand to unsigned:
#define RTE_SHIFT_VAL64(val, nr) (UINT64_C(val) << (nr))


-- 
David Marchand

Re: [PATCH v2 3/5] argparse: replace flag enum with marco

[Tyler Retzlaff]

On Thu, Mar 07, 2024 at 06:58:20PM +0100, David Marchand wrote:
> On Thu, Mar 7, 2024 at 6:43 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > On Thu, Mar 07, 2024 at 01:07:40PM +0000, Chengwen Feng wrote:
> > > The enum rte_argparse_flag's value is u64, but an enum in C is
> > > represented as an int. This commit replace these enum values with
> > > macro.
> > >
> > > Fixes: e3e579f5bab5 ("argparse: introduce argparse library")
> > > Fixes: 5357c248c960 ("argparse: parse unsigned integers")
> > >
> > > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > > ---
> > >  lib/argparse/rte_argparse.h | 78 ++++++++++++++++---------------------
> > >  1 file changed, 33 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
> > > index 47e231bef9..a6a7790cb4 100644
> > > --- a/lib/argparse/rte_argparse.h
> > > +++ b/lib/argparse/rte_argparse.h
> > > @@ -37,52 +37,40 @@
> > >  extern "C" {
> > >  #endif
> > >
> > > +/**@{@name Flag definition (in bitmask form) for an argument
> > > + *
> > > + * @note Bits[0~1] represent the argument whether has value,
> > > + * bits[2~9] represent the value type which used when autosave.
> > > + *
> > > + * @see struct rte_argparse_arg::flags
> > > + */
> > > +/** The argument has no value. */
> > > +#define RTE_ARGPARSE_ARG_NO_VALUE       RTE_SHIFT_VAL64(1, 0)
> > > +/** The argument must have a value. */
> > > +#define RTE_ARGPARSE_ARG_REQUIRED_VALUE RTE_SHIFT_VAL64(2, 0)
> > > +/** The argument has optional value. */
> > > +#define RTE_ARGPARSE_ARG_OPTIONAL_VALUE RTE_SHIFT_VAL64(3, 0)
> > > +/** The argument's value is int type. */
> > > +#define RTE_ARGPARSE_ARG_VALUE_INT      RTE_SHIFT_VAL64(1, 2)
> > > +/** The argument's value is uint8 type. */
> > > +#define RTE_ARGPARSE_ARG_VALUE_U8       RTE_SHIFT_VAL64(2, 2)
> > > +/** The argument's value is uint16 type. */
> > > +#define RTE_ARGPARSE_ARG_VALUE_U16      RTE_SHIFT_VAL64(3, 2)
> > > +/** The argument's value is uint32 type. */
> > > +#define RTE_ARGPARSE_ARG_VALUE_U32      RTE_SHIFT_VAL64(4, 2)
> > > +/** The argument's value is uint64 type. */
> > > +#define RTE_ARGPARSE_ARG_VALUE_U64      RTE_SHIFT_VAL64(5, 2)
> > > +/** Max value type. */
> > > +#define RTE_ARGPARSE_ARG_VALUE_MAX      RTE_SHIFT_VAL64(6, 2)
> >
> > it was good to get rid of the enum but will the above macros expand to
> > signed or unsigned integer given the type of field could we make sure
> > they expand unsigned?
> 
> Afaiu, those should expand to unsigned:
> #define RTE_SHIFT_VAL64(val, nr) (UINT64_C(val) << (nr))

Thanks for confirming!

> 
> 
> -- 
> David Marchand

Re: [PATCH v2 4/5] argparse: fix argument flags operate as uint32 type

[Thomas Monjalon]

07/03/2024 14:07, Chengwen Feng:
> --- a/app/test/test_argparse.c
> +++ b/app/test/test_argparse.c
> +#define TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK	RTE_SHIFT_VAL64(3, 0)
> +#define TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK	RTE_SHIFT_VAL64(255, 2)

These masks should be defined in the API probably.

[PATCH v3 0/6] refine argparse library

[Chengwen Feng]

I found a couple of issues when I revisited the argparse_autotest
output, so got this patchset.

Chengwen Feng (6):
  argparse: refine error message
  argparse: remove dead code
  argparse: replace flag enum with marco
  argparse: fix argument flags operate as uint32 type
  test/argparse: refine testcases
  argparse: fix doc don't display two hyphens

---
v3:
- address Thomas's comment on 4/6 comit.
- add commit: fix doc don't display two hyphens.
v2: address David Marchand's comment:
- replace flag enum with marco.
- replace flag's hardcode with macro in test_argparse.c.

 app/test/test_argparse.c               | 68 ++++++++++++---------
 doc/guides/prog_guide/argparse_lib.rst | 47 +++++++-------
 lib/argparse/rte_argparse.c            | 61 +++++++++---------
 lib/argparse/rte_argparse.h            | 85 ++++++++++++--------------
 4 files changed, 130 insertions(+), 131 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] argparse: refine error message

[Chengwen Feng]

This patch refines the error message.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/argparse/rte_argparse.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 2d953f1694..48738cd07b 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -67,7 +67,7 @@ verify_arg_name(const struct rte_argparse_arg *arg)
 			return -EINVAL;
 		}
 		if (arg->name_long[1] != '-') {
-			ARGPARSE_LOG(ERR, "optional long name %s must only start with '--'",
+			ARGPARSE_LOG(ERR, "optional long name %s doesn't start with '--'",
 				     arg->name_long);
 			return -EINVAL;
 		}
@@ -101,7 +101,7 @@ static int
 verify_arg_help(const struct rte_argparse_arg *arg)
 {
 	if (arg->help == NULL) {
-		ARGPARSE_LOG(ERR, "argument %s must have help info!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s doesn't have help info!", arg->name_long);
 		return -EINVAL;
 	}
 
@@ -116,13 +116,13 @@ verify_arg_has_val(const struct rte_argparse_arg *arg)
 	if (is_arg_positional(arg)) {
 		if (has_val == RTE_ARGPARSE_ARG_REQUIRED_VALUE)
 			return 0;
-		ARGPARSE_LOG(ERR, "argument %s is positional, should has zero or required-val!",
+		ARGPARSE_LOG(ERR, "argument %s is positional, must config required-val!",
 			     arg->name_long);
 		return -EINVAL;
 	}
 
 	if (has_val == 0) {
-		ARGPARSE_LOG(ERR, "argument %s is optional, has-val config wrong!",
+		ARGPARSE_LOG(ERR, "argument %s is optional, has-value config wrong!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -140,13 +140,13 @@ verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 
 	if (arg->val_saver == NULL) {
 		if (val_type != 0) {
-			ARGPARSE_LOG(ERR, "argument %s parse by callback, val-type must be zero!",
+			ARGPARSE_LOG(ERR, "argument %s parsed by callback, value-type should not be set!",
 				     arg->name_long);
 			return -EINVAL;
 		}
 
 		if (obj->callback == NULL) {
-			ARGPARSE_LOG(ERR, "argument %s parse by callback, but callback is NULL!",
+			ARGPARSE_LOG(ERR, "argument %s parsed by callback, but callback is NULL!",
 				     arg->name_long);
 			return -EINVAL;
 		}
@@ -155,12 +155,12 @@ verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 	}
 
 	if (val_type == 0 || val_type >= cmp_max) {
-		ARGPARSE_LOG(ERR, "argument %s val-type config wrong!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s value-type config wrong!", arg->name_long);
 		return -EINVAL;
 	}
 
 	if (has_val == RTE_ARGPARSE_ARG_REQUIRED_VALUE && arg->val_set != NULL) {
-		ARGPARSE_LOG(ERR, "argument %s has required value, val-set should be NULL!",
+		ARGPARSE_LOG(ERR, "argument %s has required value, value-set should be NULL!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -175,7 +175,8 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 	uint32_t unused_bits = arg_attr_unused_bits(arg);
 
 	if (unused_bits != 0) {
-		ARGPARSE_LOG(ERR, "argument %s flags set wrong!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s flags unused bits should not be set!",
+			     arg->name_long);
 		return -EINVAL;
 	}
 
@@ -189,7 +190,7 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 	}
 
 	if (arg->val_saver != NULL) {
-		ARGPARSE_LOG(ERR, "argument %s could occur multiple times, should use callback to parse!",
+		ARGPARSE_LOG(ERR, "argument %s supports multiple times, should use callback to parse!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -536,8 +537,10 @@ parse_arg_autosave(struct rte_argparse_arg *arg, const char *value)
 	return ret;
 }
 
+/* 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, struct rte_argparse_arg *arg, char *value)
+parse_arg_val(struct rte_argparse *obj, const char *arg_name,
+	      struct rte_argparse_arg *arg, char *value)
 {
 	int ret;
 
@@ -546,7 +549,7 @@ parse_arg_val(struct rte_argparse *obj, struct rte_argparse_arg *arg, char *valu
 	else
 		ret = parse_arg_autosave(arg, value);
 	if (ret != 0) {
-		ARGPARSE_LOG(ERR, "argument %s parse value fail!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s parse value fail!", arg_name);
 		return ret;
 	}
 
@@ -582,7 +585,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 				return -EINVAL;
 			}
 			arg = find_position_arg(obj, position_index);
-			ret = parse_arg_val(obj, arg, curr_argv);
+			ret = parse_arg_val(obj, arg->name_long, arg, curr_argv);
 			if (ret != 0)
 				return ret;
 			continue;
@@ -629,7 +632,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 			/* Do nothing, because it's optional value, only support arg=val or arg. */
 		}
 
-		ret = parse_arg_val(obj, arg, value);
+		ret = parse_arg_val(obj, arg_name, arg, value);
 		if (ret != 0)
 			return ret;
 
-- 
2.17.1

[PATCH v3 2/6] argparse: remove dead code

[Chengwen Feng]

The judgement "obj->callback == NULL" is dead code which can't be
reached, because verify_arg_saver() already make sure obj->callback
must not be NULL when arg->val_saver is NULL.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c    | 7 -------
 lib/argparse/rte_argparse.c | 6 ------
 2 files changed, 13 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index df11a129ba..c98bcee56d 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -288,13 +288,6 @@ test_argparse_invalid_arg_flags(void)
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
-	obj = test_argparse_init_obj();
-	obj->args[0].val_saver = NULL;
-	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_SUPPORT_MULTI;
-	obj->callback = NULL;
-	ret = rte_argparse_parse(obj, default_argc, default_argv);
-	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
-
 	return 0;
 }
 
diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 48738cd07b..6e890cdc0d 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -195,12 +195,6 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 		return -EINVAL;
 	}
 
-	if (obj->callback == NULL) {
-		ARGPARSE_LOG(ERR, "argument %s should use callback to parse, but callback is NULL!",
-			     arg->name_long);
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH v3 3/6] argparse: replace flag enum with marco

[Chengwen Feng]

The enum rte_argparse_flag's value is u64, but an enum in C is
represented as an int. This commit replace these enum values with
macro.

Fixes: e3e579f5bab5 ("argparse: introduce argparse library")
Fixes: 5357c248c960 ("argparse: parse unsigned integers")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/argparse/rte_argparse.h | 78 ++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index 47e231bef9..a6a7790cb4 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -37,52 +37,40 @@
 extern "C" {
 #endif
 
+/**@{@name Flag definition (in bitmask form) for an argument
+ *
+ * @note Bits[0~1] represent the argument whether has value,
+ * bits[2~9] represent the value type which used when autosave.
+ *
+ * @see struct rte_argparse_arg::flags
+ */
+/** The argument has no value. */
+#define RTE_ARGPARSE_ARG_NO_VALUE       RTE_SHIFT_VAL64(1, 0)
+/** The argument must have a value. */
+#define RTE_ARGPARSE_ARG_REQUIRED_VALUE RTE_SHIFT_VAL64(2, 0)
+/** The argument has optional value. */
+#define RTE_ARGPARSE_ARG_OPTIONAL_VALUE RTE_SHIFT_VAL64(3, 0)
+/** The argument's value is int type. */
+#define RTE_ARGPARSE_ARG_VALUE_INT      RTE_SHIFT_VAL64(1, 2)
+/** The argument's value is uint8 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U8       RTE_SHIFT_VAL64(2, 2)
+/** The argument's value is uint16 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U16      RTE_SHIFT_VAL64(3, 2)
+/** The argument's value is uint32 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U32      RTE_SHIFT_VAL64(4, 2)
+/** The argument's value is uint64 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U64      RTE_SHIFT_VAL64(5, 2)
+/** Max value type. */
+#define RTE_ARGPARSE_ARG_VALUE_MAX      RTE_SHIFT_VAL64(6, 2)
 /**
- * Flag definition (in bitmask form) for an argument.
+ * Flag for that argument support occur multiple times.
+ * This flag can be set only when the argument is optional.
+ * When this flag is set, the callback type must be used for parsing.
  */
-enum rte_argparse_flag {
-	/*
-	 * Bits 0-1: represent the argument whether has value.
-	 */
-
-	/** The argument has no value. */
-	RTE_ARGPARSE_ARG_NO_VALUE = RTE_SHIFT_VAL64(1, 0),
-	/** The argument must have a value. */
-	RTE_ARGPARSE_ARG_REQUIRED_VALUE = RTE_SHIFT_VAL64(2, 0),
-	/** The argument has optional value. */
-	RTE_ARGPARSE_ARG_OPTIONAL_VALUE = RTE_SHIFT_VAL64(3, 0),
-
-
-	/*
-	 * Bits 2-9: represent the value type which used when autosave
-	 */
-
-	/** The argument's value is int type. */
-	RTE_ARGPARSE_ARG_VALUE_INT = RTE_SHIFT_VAL64(1, 2),
-	/** The argument's value is uint8 type. */
-	RTE_ARGPARSE_ARG_VALUE_U8 = RTE_SHIFT_VAL64(2, 2),
-	/** The argument's value is uint16 type. */
-	RTE_ARGPARSE_ARG_VALUE_U16 = RTE_SHIFT_VAL64(3, 2),
-	/** The argument's value is uint32 type. */
-	RTE_ARGPARSE_ARG_VALUE_U32 = RTE_SHIFT_VAL64(4, 2),
-	/** The argument's value is uint64 type. */
-	RTE_ARGPARSE_ARG_VALUE_U64 = RTE_SHIFT_VAL64(5, 2),
-	/** Max value type. */
-	RTE_ARGPARSE_ARG_VALUE_MAX = RTE_SHIFT_VAL64(6, 2),
-
-
-	/**
-	 * Bit 10: flag for that argument support occur multiple times.
-	 * This flag can be set only when the argument is optional.
-	 * When this flag is set, the callback type must be used for parsing.
-	 */
-	RTE_ARGPARSE_ARG_SUPPORT_MULTI = RTE_BIT64(10),
-
-	/**
-	 * Bits 48-63: reserved for this library implementation usage.
-	 */
-	RTE_ARGPARSE_ARG_RESERVED_FIELD = RTE_GENMASK64(63, 48),
-};
+#define RTE_ARGPARSE_ARG_SUPPORT_MULTI  RTE_BIT64(10)
+/** Reserved for this library implementation usage. */
+#define RTE_ARGPARSE_ARG_RESERVED_FIELD RTE_GENMASK64(63, 48)
+/**@}*/
 
 /**
  * A structure used to hold argument's configuration.
@@ -126,7 +114,7 @@ struct rte_argparse_arg {
 	 */
 	void *val_set;
 
-	/** @see rte_argparse_flag */
+	/** Flag definition (RTE_ARGPARSE_ARG_*) for the argument. */
 	uint64_t flags;
 };
 
-- 
2.17.1

[PATCH v3 4/6] argparse: fix argument flags operate as uint32 type

[Chengwen Feng]

The struct rte_argparse_arg's flags was 64bit type, uint64_t should be
used instead of uint32_t where the operation happened.

Also, the flags' bit16 was also unused, so don't test bit16 in testcase
test_argparse_invalid_arg_flags.

In addition, this commit introduces two bitmask marcros and removes an
internal duplicate macro.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Fixes: 31ed9f9f43bb ("argparse: parse parameters")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c    | 21 +++++++++++++--------
 lib/argparse/rte_argparse.c | 24 ++++++++++++------------
 lib/argparse/rte_argparse.h |  5 +++++
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index c98bcee56d..d8dbbee499 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -16,6 +16,9 @@ static char *default_argv[1];
 static char *strdup_store_array[MAX_STRDUP_STORE_NUM];
 static uint32_t strdup_store_index;
 
+#define TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK	RTE_SHIFT_VAL64(3, 0)
+#define TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK	RTE_SHIFT_VAL64(255, 2)
+
 /*
  * Define strdup wrapper.
  * 1. Mainly to fix compile error "warning: assignment discards 'const'
@@ -188,7 +191,7 @@ test_argparse_invalid_arg_help(void)
 static int
 test_argparse_invalid_has_val(void)
 {
-	uint32_t set_mask[] = { 0,
+	uint64_t set_mask[] = { 0,
 				RTE_ARGPARSE_ARG_NO_VALUE,
 				RTE_ARGPARSE_ARG_OPTIONAL_VALUE
 			      };
@@ -197,7 +200,7 @@ test_argparse_invalid_has_val(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	obj->args[0].flags &= ~0x3u;
+	obj->args[0].flags &= ~RTE_ARGPARSE_HAS_VAL_BITMASK;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
@@ -205,7 +208,7 @@ test_argparse_invalid_has_val(void)
 		obj = test_argparse_init_obj();
 		obj->args[0].name_long = "abc";
 		obj->args[0].name_short = NULL;
-		obj->args[0].flags &= ~0x3u;
+		obj->args[0].flags &= ~RTE_ARGPARSE_HAS_VAL_BITMASK;
 		obj->args[0].flags |= set_mask[index];
 		ret = rte_argparse_parse(obj, default_argc, default_argv);
 		TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
@@ -269,7 +272,9 @@ test_argparse_invalid_arg_flags(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	obj->args[0].flags |= ~0x107FFu;
+	obj->args[0].flags |= ~(RTE_ARGPARSE_HAS_VAL_BITMASK |
+				RTE_ARGPARSE_VAL_TYPE_BITMASK |
+				RTE_ARGPARSE_ARG_SUPPORT_MULTI);
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
@@ -337,7 +342,7 @@ test_argparse_invalid_option(void)
 static int
 test_argparse_opt_autosave_parse_int_of_no_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_NO_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_NO_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[2];
@@ -369,7 +374,7 @@ test_argparse_opt_autosave_parse_int_of_no_val(void)
 static int
 test_argparse_opt_autosave_parse_int_of_required_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[3];
@@ -410,7 +415,7 @@ test_argparse_opt_autosave_parse_int_of_required_val(void)
 static int
 test_argparse_opt_autosave_parse_int_of_optional_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[2];
@@ -645,7 +650,7 @@ test_argparse_opt_callback_parse_int_of_optional_val(void)
 static int
 test_argparse_pos_autosave_parse_int(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[3];
diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 6e890cdc0d..e7007afc6a 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -15,9 +15,6 @@ RTE_LOG_REGISTER_DEFAULT(rte_argparse_logtype, INFO);
 #define ARGPARSE_LOG(level, ...) \
 	RTE_LOG_LINE(level, ARGPARSE, "" __VA_ARGS__)
 
-#define ARG_ATTR_HAS_VAL_MASK		RTE_GENMASK64(1, 0)
-#define ARG_ATTR_VAL_TYPE_MASK		RTE_GENMASK64(9, 2)
-#define ARG_ATTR_SUPPORT_MULTI_MASK	RTE_BIT64(10)
 #define ARG_ATTR_FLAG_PARSED_MASK	RTE_BIT64(63)
 
 static inline bool
@@ -35,26 +32,27 @@ is_arg_positional(const struct rte_argparse_arg *arg)
 static inline uint32_t
 arg_attr_has_val(const struct rte_argparse_arg *arg)
 {
-	return RTE_FIELD_GET64(ARG_ATTR_HAS_VAL_MASK, arg->flags);
+	return RTE_FIELD_GET64(RTE_ARGPARSE_HAS_VAL_BITMASK, arg->flags);
 }
 
 static inline uint32_t
 arg_attr_val_type(const struct rte_argparse_arg *arg)
 {
-	return RTE_FIELD_GET64(ARG_ATTR_VAL_TYPE_MASK, arg->flags);
+	return RTE_FIELD_GET64(RTE_ARGPARSE_VAL_TYPE_BITMASK, arg->flags);
 }
 
 static inline bool
 arg_attr_flag_multi(const struct rte_argparse_arg *arg)
 {
-	return RTE_FIELD_GET64(ARG_ATTR_SUPPORT_MULTI_MASK, arg->flags);
+	return RTE_FIELD_GET64(RTE_ARGPARSE_ARG_SUPPORT_MULTI, arg->flags);
 }
 
-static inline uint32_t
+static inline uint64_t
 arg_attr_unused_bits(const struct rte_argparse_arg *arg)
 {
-#define USED_BIT_MASK	(ARG_ATTR_HAS_VAL_MASK | ARG_ATTR_VAL_TYPE_MASK | \
-			 ARG_ATTR_SUPPORT_MULTI_MASK)
+#define USED_BIT_MASK	(RTE_ARGPARSE_HAS_VAL_BITMASK | \
+			 RTE_ARGPARSE_VAL_TYPE_BITMASK | \
+			 RTE_ARGPARSE_ARG_SUPPORT_MULTI)
 	return arg->flags & ~USED_BIT_MASK;
 }
 
@@ -133,7 +131,8 @@ verify_arg_has_val(const struct rte_argparse_arg *arg)
 static int
 verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 {
-	uint32_t cmp_max = RTE_FIELD_GET64(ARG_ATTR_VAL_TYPE_MASK, RTE_ARGPARSE_ARG_VALUE_MAX);
+	uint32_t cmp_max = RTE_FIELD_GET64(RTE_ARGPARSE_VAL_TYPE_BITMASK,
+					   RTE_ARGPARSE_ARG_VALUE_MAX);
 	const struct rte_argparse_arg *arg = &obj->args[index];
 	uint32_t val_type = arg_attr_val_type(arg);
 	uint32_t has_val = arg_attr_has_val(arg);
@@ -172,7 +171,7 @@ static int
 verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 {
 	const struct rte_argparse_arg *arg = &obj->args[index];
-	uint32_t unused_bits = arg_attr_unused_bits(arg);
+	uint64_t unused_bits = arg_attr_unused_bits(arg);
 
 	if (unused_bits != 0) {
 		ARGPARSE_LOG(ERR, "argument %s flags unused bits should not be set!",
@@ -768,7 +767,8 @@ rte_argparse_parse(struct rte_argparse *obj, int argc, char **argv)
 int
 rte_argparse_parse_type(const char *str, uint64_t val_type, void *val)
 {
-	uint32_t cmp_max = RTE_FIELD_GET64(ARG_ATTR_VAL_TYPE_MASK, RTE_ARGPARSE_ARG_VALUE_MAX);
+	uint32_t cmp_max = RTE_FIELD_GET64(RTE_ARGPARSE_VAL_TYPE_BITMASK,
+					   RTE_ARGPARSE_ARG_VALUE_MAX);
 	struct rte_argparse_arg arg = {
 		.name_long = str,
 		.name_short = NULL,
diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index a6a7790cb4..98ad9971ea 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -72,6 +72,11 @@ extern "C" {
 #define RTE_ARGPARSE_ARG_RESERVED_FIELD RTE_GENMASK64(63, 48)
 /**@}*/
 
+/** Bitmask used to get the argument whether has value. */
+#define RTE_ARGPARSE_HAS_VAL_BITMASK	RTE_GENMASK64(1, 0)
+/** Bitmask used to get the argument's value type. */
+#define RTE_ARGPARSE_VAL_TYPE_BITMASK	RTE_GENMASK64(9, 2)
+
 /**
  * A structure used to hold argument's configuration.
  */
-- 
2.17.1

[PATCH v3 5/6] test/argparse: refine testcases

[Chengwen Feng]

Refine testcases, including:
1. add testcase comment.
2. argv[0] should set obj->prog_name.
3. set val_set as NULL in test_argparse_invalid_arg_flags, let it
test to the specified code logic.
4. enable index verification in opt_callback_parse_int_of_no_val.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Fixes: 31ed9f9f43bb ("argparse: parse parameters")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index d8dbbee499..4034e7a343 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -199,11 +199,13 @@ test_argparse_invalid_has_val(void)
 	uint32_t index;
 	int ret;
 
+	/* test optional arg don't config has-value. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags &= ~RTE_ARGPARSE_HAS_VAL_BITMASK;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test positional arg don't config required-value. */
 	for (index = 0; index < RTE_DIM(set_mask); index++) {
 		obj = test_argparse_init_obj();
 		obj->args[0].name_long = "abc";
@@ -271,6 +273,7 @@ test_argparse_invalid_arg_flags(void)
 	struct rte_argparse *obj;
 	int ret;
 
+	/* test set unused bits. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags |= ~(RTE_ARGPARSE_HAS_VAL_BITMASK |
 				RTE_ARGPARSE_VAL_TYPE_BITMASK |
@@ -278,16 +281,18 @@ test_argparse_invalid_arg_flags(void)
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test positional arg should not config multiple.  */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "positional";
 	obj->args[0].name_short = NULL;
 	obj->args[0].val_saver = (void *)1;
-	obj->args[0].val_set = (void *)1;
+	obj->args[0].val_set = NULL;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT |
 			     RTE_ARGPARSE_ARG_SUPPORT_MULTI;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test optional arg enabled multiple but prased by autosave. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags |= RTE_ARGPARSE_ARG_SUPPORT_MULTI;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
@@ -325,13 +330,13 @@ test_argparse_invalid_option(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--invalid");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
 	obj = test_argparse_init_obj();
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("invalid");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
@@ -355,7 +360,7 @@ test_argparse_opt_autosave_parse_int_of_no_val(void)
 	obj->args[0].val_set = (void *)100;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -387,7 +392,7 @@ test_argparse_opt_autosave_parse_int_of_required_val(void)
 	obj->args[0].val_set = NULL;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	argv[2] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -421,6 +426,7 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 	char *argv[2];
 	int ret;
 
+	/* test without value. */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "--test-long";
 	obj->args[0].name_short = "-t";
@@ -428,7 +434,7 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 	obj->args[0].val_set = (void *)100;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -472,7 +478,8 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 static int
 opt_callback_parse_int_of_no_val(uint32_t index, const char *value, void *opaque)
 {
-	RTE_SET_USED(index);
+	if (index != 1)
+		return -EINVAL;
 	if (value != NULL)
 		return -EINVAL;
 	*(int *)opaque = 100;
@@ -493,10 +500,10 @@ test_argparse_opt_callback_parse_int_of_no_val(void)
 	obj->args[0].name_long = "--test-long";
 	obj->args[0].name_short = "-t";
 	obj->args[0].val_saver = NULL;
-	obj->args[0].val_set = (void *)100;
+	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_NO_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -547,7 +554,7 @@ test_argparse_opt_callback_parse_int_of_required_val(void)
 	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	argv[2] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -611,7 +618,7 @@ test_argparse_opt_callback_parse_int_of_optional_val(void)
 	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -656,6 +663,7 @@ test_argparse_pos_autosave_parse_int(void)
 	char *argv[3];
 	int ret;
 
+	/* test positional autosave parse successful. */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "test-long";
 	obj->args[0].name_short = NULL;
@@ -663,19 +671,20 @@ test_argparse_pos_autosave_parse_int(void)
 	obj->args[0].val_set = NULL;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
 	TEST_ASSERT(val_saver == 100, "Argparse parse expect success!");
 
+	/* test positional autosave parse failed. */
 	obj->args[0].flags = flags;
 	val_saver = 0;
 	argv[1] = test_strdup("100a");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
-	/* test over position parameters. */
+	/* test too much position parameters. */
 	obj->args[0].flags = flags;
 	argv[1] = test_strdup("100");
 	argv[2] = test_strdup("200");
@@ -713,6 +722,7 @@ test_argparse_pos_callback_parse_int(void)
 	char *argv[3];
 	int ret;
 
+	/* test positional callback parse successful. */
 	obj = test_argparse_init_obj();
 	obj->callback = pos_callback_parse_int;
 	obj->opaque = (void *)val_saver;
@@ -727,7 +737,7 @@ test_argparse_pos_callback_parse_int(void)
 	obj->args[1].val_set = (void *)2;
 	obj->args[1].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[2].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("100");
 	argv[2] = test_strdup("200");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -735,7 +745,7 @@ test_argparse_pos_callback_parse_int(void)
 	TEST_ASSERT(val_saver[1] == 100, "Argparse parse expect success!");
 	TEST_ASSERT(val_saver[2] == 200, "Argparse parse expect success!");
 
-	/* test callback return failed. */
+	/* test positional callback parse failed. */
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[1].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	argv[2] = test_strdup("200a");
-- 
2.17.1

[PATCH v3 6/6] argparse: fix doc don't display two hyphens

[Chengwen Feng]

With the line in rst file:
	The single mode: "--aaa" or "-a".
corresponding line in html doc:
	The single mode: -aaa or -a.
the two hyphens (--aaa) become one (-aaa).

According to [1], this commit uses the backquote (``xxx``) to fix it.
And for consistency, use this format for all arguments.

Fixes: e3e579f5bab5 ("argparse: introduce argparse library")

[1] https://stackoverflow.com/questions/51075907/display-two-dashes-in-rst-file

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 doc/guides/prog_guide/argparse_lib.rst | 47 +++++++++++++-------------
 lib/argparse/rte_argparse.h            |  4 +--
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/doc/guides/prog_guide/argparse_lib.rst b/doc/guides/prog_guide/argparse_lib.rst
index a6ac11b1c0..f827312daa 100644
--- a/doc/guides/prog_guide/argparse_lib.rst
+++ b/doc/guides/prog_guide/argparse_lib.rst
@@ -90,9 +90,9 @@ The following code demonstrates how to use:
    }
 
 In this example, the arguments which start with a hyphen (-) are optional
-arguments (they're "--aaa"/"--bbb"/"--ccc"/"--ddd"/"--eee"/"--fff"); and the
-arguments which don't start with a hyphen (-) are positional arguments
-(they're "ooo"/"ppp").
+arguments (they're ``--aaa``/``--bbb``/``--ccc``/``--ddd``/``--eee``/``--fff``);
+and the arguments which don't start with a hyphen (-) are positional arguments
+(they're ``ooo``/``ppp``).
 
 Every argument must be set whether to carry a value (one of
 ``RTE_ARGPARSE_ARG_NO_VALUE``, ``RTE_ARGPARSE_ARG_REQUIRED_VALUE`` and
@@ -106,23 +106,23 @@ User Input Requirements
 ~~~~~~~~~~~~~~~~~~~~~~~
 
 For optional arguments which take no-value,
-the following mode is supported (take above "--aaa" as an example):
+the following mode is supported (take above ``--aaa`` as an example):
 
-- The single mode: "--aaa" or "-a".
+- The single mode: ``--aaa`` or ``-a``.
 
 For optional arguments which take required-value,
-the following two modes are supported (take above "--bbb" as an example):
+the following two modes are supported (take above ``--bbb`` as an example):
 
-- The kv mode: "--bbb=1234" or "-b=1234".
+- The kv mode: ``--bbb=1234`` or ``-b=1234``.
 
-- The split mode: "--bbb 1234" or "-b 1234".
+- The split mode: ``--bbb 1234`` or ``-b 1234``.
 
 For optional arguments which take optional-value,
-the following two modes are supported (take above "--ccc" as an example):
+the following two modes are supported (take above ``--ccc`` as an example):
 
-- The single mode: "--ccc" or "-c".
+- The single mode: ``--ccc`` or ``-c``.
 
-- The kv mode: "--ccc=123" or "-c=123".
+- The kv mode: ``--ccc=123`` or ``-c=123``.
 
 For positional arguments which must take required-value,
 their values are parsing in the order defined.
@@ -130,7 +130,7 @@ their values are parsing in the order defined.
 .. note::
 
    The compact mode is not supported.
-   Take above "-a" and "-d" as an example, don't support "-ad" input.
+   Take above ``-a`` and ``-d`` as an example, don't support ``-ad`` input.
 
 Parsing by autosave way
 ~~~~~~~~~~~~~~~~~~~~~~~
@@ -139,23 +139,23 @@ Argument of known value type (e.g. ``RTE_ARGPARSE_ARG_VALUE_INT``)
 could be parsed using this autosave way,
 and its result will save in the ``val_saver`` field.
 
-In the above example, the arguments "--aaa"/"--bbb"/"--ccc" and "ooo"
+In the above example, the arguments ``--aaa``/``--bbb``/``--ccc`` and ``ooo``
 both use this way, the parsing is as follows:
 
-- For argument "--aaa", it is configured as no-value,
+- For argument ``--aaa``, it is configured as no-value,
   so the ``aaa_val`` will be set to ``val_set`` field
   which is 100 in the above example.
 
-- For argument "--bbb", it is configured as required-value,
+- For argument ``--bbb``, it is configured as required-value,
   so the ``bbb_val`` will be set to user input's value
-  (e.g. will be set to 1234 with input "--bbb 1234").
+  (e.g. will be set to 1234 with input ``--bbb 1234``).
 
-- For argument "--ccc", it is configured as optional-value,
-  if user only input "--ccc" then the ``ccc_val`` will be set to ``val_set`` field
-  which is 200 in the above example;
-  if user input "--ccc=123", then the ``ccc_val`` will be set to 123.
+- For argument ``--ccc``, it is configured as optional-value,
+  if user only input ``--ccc`` then the ``ccc_val`` will be set to ``val_set``
+  field which is 200 in the above example;
+  if user input ``--ccc=123``, then the ``ccc_val`` will be set to 123.
 
-- For argument "ooo", it is positional argument,
+- For argument ``ooo``, it is positional argument,
   the ``ooo_val`` will be set to user input's value.
 
 Parsing by callback way
@@ -165,7 +165,8 @@ It could also choose to use callback to parse,
 just define a unique index for the argument
 and make the ``val_save`` field to be NULL also zero value-type.
 
-In the above example, the arguments "--ddd"/"--eee"/"--fff" and "ppp" both use this way.
+In the above example, the arguments ``--ddd``/``--eee``/``--fff`` and ``ppp``
+both use this way.
 
 Multiple times argument
 ~~~~~~~~~~~~~~~~~~~~~~~
@@ -178,7 +179,7 @@ For example:
 
    { "--xyz", "-x", "xyz argument", NULL, (void *)10, RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_SUPPORT_MULTI },
 
-Then the user input could contain multiple "--xyz" arguments.
+Then the user input could contain multiple ``--xyz`` arguments.
 
 .. note::
 
diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index 98ad9971ea..b6b016e388 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -83,8 +83,8 @@ extern "C" {
 struct rte_argparse_arg {
 	/**
 	 * Long name of the argument:
-	 * 1) If the argument is optional, it must start with '--'.
-	 * 2) If the argument is positional, it must not start with '-'.
+	 * 1) If the argument is optional, it must start with ``--``.
+	 * 2) If the argument is positional, it must not start with ``-``.
 	 * 3) Other case will be considered as error.
 	 */
 	const char *name_long;
-- 
2.17.1

Re: [PATCH v2 4/5] argparse: fix argument flags operate as uint32 type

[fengchengwen]

Hi Thomas,

On 2024/3/18 10:31, Thomas Monjalon wrote:
> 07/03/2024 14:07, Chengwen Feng:
>> --- a/app/test/test_argparse.c
>> +++ b/app/test/test_argparse.c
>> +#define TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK	RTE_SHIFT_VAL64(3, 0)
>> +#define TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK	RTE_SHIFT_VAL64(255, 2)
> 
> These masks should be defined in the API probably.

done in v3

Thanks

> 
> 
> .
> 

Re: [PATCH v3 4/6] argparse: fix argument flags operate as uint32 type

[Thomas Monjalon]

18/03/2024 10:18, Chengwen Feng:
> --- a/app/test/test_argparse.c
> +++ b/app/test/test_argparse.c
> +#define TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK	RTE_SHIFT_VAL64(3, 0)
> +#define TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK	RTE_SHIFT_VAL64(255, 2)

Don't you want to make it part of the API?
I thought it was what you did in this v3.

[PATCH v4 0/6] refine argparse library

[Chengwen Feng]

I found a couple of issues when I revisited the argparse_autotest
output, so got this patchset.

Chengwen Feng (6):
  argparse: refine error message
  argparse: remove dead code
  argparse: replace flag enum with marco
  argparse: fix argument flags operate as uint32 type
  test/argparse: refine testcases
  argparse: fix doc don't display two hyphens

---
v4: address Thomas's comment on 4/6 commit:
- remove unused macros TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK and
  TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK.
v3:
- address Thomas's comment on 4/6 comit.
- add commit: fix doc don't display two hyphens.
v2: address David Marchand's comment:
- replace flag enum with marco.
- replace flag's hardcode with macro in test_argparse.c.

 app/test/test_argparse.c               | 65 +++++++++++---------
 doc/guides/prog_guide/argparse_lib.rst | 47 +++++++-------
 lib/argparse/rte_argparse.c            | 61 +++++++++---------
 lib/argparse/rte_argparse.h            | 85 ++++++++++++--------------
 4 files changed, 127 insertions(+), 131 deletions(-)

-- 
2.17.1

[PATCH v4 1/6] argparse: refine error message

[Chengwen Feng]

This patch refines the error message.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/argparse/rte_argparse.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 2d953f1694..48738cd07b 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -67,7 +67,7 @@ verify_arg_name(const struct rte_argparse_arg *arg)
 			return -EINVAL;
 		}
 		if (arg->name_long[1] != '-') {
-			ARGPARSE_LOG(ERR, "optional long name %s must only start with '--'",
+			ARGPARSE_LOG(ERR, "optional long name %s doesn't start with '--'",
 				     arg->name_long);
 			return -EINVAL;
 		}
@@ -101,7 +101,7 @@ static int
 verify_arg_help(const struct rte_argparse_arg *arg)
 {
 	if (arg->help == NULL) {
-		ARGPARSE_LOG(ERR, "argument %s must have help info!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s doesn't have help info!", arg->name_long);
 		return -EINVAL;
 	}
 
@@ -116,13 +116,13 @@ verify_arg_has_val(const struct rte_argparse_arg *arg)
 	if (is_arg_positional(arg)) {
 		if (has_val == RTE_ARGPARSE_ARG_REQUIRED_VALUE)
 			return 0;
-		ARGPARSE_LOG(ERR, "argument %s is positional, should has zero or required-val!",
+		ARGPARSE_LOG(ERR, "argument %s is positional, must config required-val!",
 			     arg->name_long);
 		return -EINVAL;
 	}
 
 	if (has_val == 0) {
-		ARGPARSE_LOG(ERR, "argument %s is optional, has-val config wrong!",
+		ARGPARSE_LOG(ERR, "argument %s is optional, has-value config wrong!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -140,13 +140,13 @@ verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 
 	if (arg->val_saver == NULL) {
 		if (val_type != 0) {
-			ARGPARSE_LOG(ERR, "argument %s parse by callback, val-type must be zero!",
+			ARGPARSE_LOG(ERR, "argument %s parsed by callback, value-type should not be set!",
 				     arg->name_long);
 			return -EINVAL;
 		}
 
 		if (obj->callback == NULL) {
-			ARGPARSE_LOG(ERR, "argument %s parse by callback, but callback is NULL!",
+			ARGPARSE_LOG(ERR, "argument %s parsed by callback, but callback is NULL!",
 				     arg->name_long);
 			return -EINVAL;
 		}
@@ -155,12 +155,12 @@ verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 	}
 
 	if (val_type == 0 || val_type >= cmp_max) {
-		ARGPARSE_LOG(ERR, "argument %s val-type config wrong!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s value-type config wrong!", arg->name_long);
 		return -EINVAL;
 	}
 
 	if (has_val == RTE_ARGPARSE_ARG_REQUIRED_VALUE && arg->val_set != NULL) {
-		ARGPARSE_LOG(ERR, "argument %s has required value, val-set should be NULL!",
+		ARGPARSE_LOG(ERR, "argument %s has required value, value-set should be NULL!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -175,7 +175,8 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 	uint32_t unused_bits = arg_attr_unused_bits(arg);
 
 	if (unused_bits != 0) {
-		ARGPARSE_LOG(ERR, "argument %s flags set wrong!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s flags unused bits should not be set!",
+			     arg->name_long);
 		return -EINVAL;
 	}
 
@@ -189,7 +190,7 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 	}
 
 	if (arg->val_saver != NULL) {
-		ARGPARSE_LOG(ERR, "argument %s could occur multiple times, should use callback to parse!",
+		ARGPARSE_LOG(ERR, "argument %s supports multiple times, should use callback to parse!",
 			     arg->name_long);
 		return -EINVAL;
 	}
@@ -536,8 +537,10 @@ parse_arg_autosave(struct rte_argparse_arg *arg, const char *value)
 	return ret;
 }
 
+/* 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, struct rte_argparse_arg *arg, char *value)
+parse_arg_val(struct rte_argparse *obj, const char *arg_name,
+	      struct rte_argparse_arg *arg, char *value)
 {
 	int ret;
 
@@ -546,7 +549,7 @@ parse_arg_val(struct rte_argparse *obj, struct rte_argparse_arg *arg, char *valu
 	else
 		ret = parse_arg_autosave(arg, value);
 	if (ret != 0) {
-		ARGPARSE_LOG(ERR, "argument %s parse value fail!", arg->name_long);
+		ARGPARSE_LOG(ERR, "argument %s parse value fail!", arg_name);
 		return ret;
 	}
 
@@ -582,7 +585,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 				return -EINVAL;
 			}
 			arg = find_position_arg(obj, position_index);
-			ret = parse_arg_val(obj, arg, curr_argv);
+			ret = parse_arg_val(obj, arg->name_long, arg, curr_argv);
 			if (ret != 0)
 				return ret;
 			continue;
@@ -629,7 +632,7 @@ parse_args(struct rte_argparse *obj, int argc, char **argv, bool *show_help)
 			/* Do nothing, because it's optional value, only support arg=val or arg. */
 		}
 
-		ret = parse_arg_val(obj, arg, value);
+		ret = parse_arg_val(obj, arg_name, arg, value);
 		if (ret != 0)
 			return ret;
 
-- 
2.17.1

[PATCH v4 2/6] argparse: remove dead code

[Chengwen Feng]

The judgement "obj->callback == NULL" is dead code which can't be
reached, because verify_arg_saver() already make sure obj->callback
must not be NULL when arg->val_saver is NULL.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c    | 7 -------
 lib/argparse/rte_argparse.c | 6 ------
 2 files changed, 13 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index df11a129ba..c98bcee56d 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -288,13 +288,6 @@ test_argparse_invalid_arg_flags(void)
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
-	obj = test_argparse_init_obj();
-	obj->args[0].val_saver = NULL;
-	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_SUPPORT_MULTI;
-	obj->callback = NULL;
-	ret = rte_argparse_parse(obj, default_argc, default_argv);
-	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
-
 	return 0;
 }
 
diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 48738cd07b..6e890cdc0d 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -195,12 +195,6 @@ verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 		return -EINVAL;
 	}
 
-	if (obj->callback == NULL) {
-		ARGPARSE_LOG(ERR, "argument %s should use callback to parse, but callback is NULL!",
-			     arg->name_long);
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH v4 3/6] argparse: replace flag enum with marco

[Chengwen Feng]

The enum rte_argparse_flag's value is u64, but an enum in C is
represented as an int. This commit replace these enum values with
macro.

Fixes: e3e579f5bab5 ("argparse: introduce argparse library")
Fixes: 5357c248c960 ("argparse: parse unsigned integers")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/argparse/rte_argparse.h | 78 ++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index 47e231bef9..a6a7790cb4 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -37,52 +37,40 @@
 extern "C" {
 #endif
 
+/**@{@name Flag definition (in bitmask form) for an argument
+ *
+ * @note Bits[0~1] represent the argument whether has value,
+ * bits[2~9] represent the value type which used when autosave.
+ *
+ * @see struct rte_argparse_arg::flags
+ */
+/** The argument has no value. */
+#define RTE_ARGPARSE_ARG_NO_VALUE       RTE_SHIFT_VAL64(1, 0)
+/** The argument must have a value. */
+#define RTE_ARGPARSE_ARG_REQUIRED_VALUE RTE_SHIFT_VAL64(2, 0)
+/** The argument has optional value. */
+#define RTE_ARGPARSE_ARG_OPTIONAL_VALUE RTE_SHIFT_VAL64(3, 0)
+/** The argument's value is int type. */
+#define RTE_ARGPARSE_ARG_VALUE_INT      RTE_SHIFT_VAL64(1, 2)
+/** The argument's value is uint8 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U8       RTE_SHIFT_VAL64(2, 2)
+/** The argument's value is uint16 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U16      RTE_SHIFT_VAL64(3, 2)
+/** The argument's value is uint32 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U32      RTE_SHIFT_VAL64(4, 2)
+/** The argument's value is uint64 type. */
+#define RTE_ARGPARSE_ARG_VALUE_U64      RTE_SHIFT_VAL64(5, 2)
+/** Max value type. */
+#define RTE_ARGPARSE_ARG_VALUE_MAX      RTE_SHIFT_VAL64(6, 2)
 /**
- * Flag definition (in bitmask form) for an argument.
+ * Flag for that argument support occur multiple times.
+ * This flag can be set only when the argument is optional.
+ * When this flag is set, the callback type must be used for parsing.
  */
-enum rte_argparse_flag {
-	/*
-	 * Bits 0-1: represent the argument whether has value.
-	 */
-
-	/** The argument has no value. */
-	RTE_ARGPARSE_ARG_NO_VALUE = RTE_SHIFT_VAL64(1, 0),
-	/** The argument must have a value. */
-	RTE_ARGPARSE_ARG_REQUIRED_VALUE = RTE_SHIFT_VAL64(2, 0),
-	/** The argument has optional value. */
-	RTE_ARGPARSE_ARG_OPTIONAL_VALUE = RTE_SHIFT_VAL64(3, 0),
-
-
-	/*
-	 * Bits 2-9: represent the value type which used when autosave
-	 */
-
-	/** The argument's value is int type. */
-	RTE_ARGPARSE_ARG_VALUE_INT = RTE_SHIFT_VAL64(1, 2),
-	/** The argument's value is uint8 type. */
-	RTE_ARGPARSE_ARG_VALUE_U8 = RTE_SHIFT_VAL64(2, 2),
-	/** The argument's value is uint16 type. */
-	RTE_ARGPARSE_ARG_VALUE_U16 = RTE_SHIFT_VAL64(3, 2),
-	/** The argument's value is uint32 type. */
-	RTE_ARGPARSE_ARG_VALUE_U32 = RTE_SHIFT_VAL64(4, 2),
-	/** The argument's value is uint64 type. */
-	RTE_ARGPARSE_ARG_VALUE_U64 = RTE_SHIFT_VAL64(5, 2),
-	/** Max value type. */
-	RTE_ARGPARSE_ARG_VALUE_MAX = RTE_SHIFT_VAL64(6, 2),
-
-
-	/**
-	 * Bit 10: flag for that argument support occur multiple times.
-	 * This flag can be set only when the argument is optional.
-	 * When this flag is set, the callback type must be used for parsing.
-	 */
-	RTE_ARGPARSE_ARG_SUPPORT_MULTI = RTE_BIT64(10),
-
-	/**
-	 * Bits 48-63: reserved for this library implementation usage.
-	 */
-	RTE_ARGPARSE_ARG_RESERVED_FIELD = RTE_GENMASK64(63, 48),
-};
+#define RTE_ARGPARSE_ARG_SUPPORT_MULTI  RTE_BIT64(10)
+/** Reserved for this library implementation usage. */
+#define RTE_ARGPARSE_ARG_RESERVED_FIELD RTE_GENMASK64(63, 48)
+/**@}*/
 
 /**
  * A structure used to hold argument's configuration.
@@ -126,7 +114,7 @@ struct rte_argparse_arg {
 	 */
 	void *val_set;
 
-	/** @see rte_argparse_flag */
+	/** Flag definition (RTE_ARGPARSE_ARG_*) for the argument. */
 	uint64_t flags;
 };
 
-- 
2.17.1

[PATCH v4 4/6] argparse: fix argument flags operate as uint32 type

[Chengwen Feng]

The struct rte_argparse_arg's flags was 64bit type, uint64_t should be
used instead of uint32_t where the operation happened.

Also, the flags' bit16 was also unused, so don't test bit16 in testcase
test_argparse_invalid_arg_flags.

In addition, this commit introduces two bitmask marcros and removes an
internal duplicate macro.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Fixes: 31ed9f9f43bb ("argparse: parse parameters")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c    | 18 ++++++++++--------
 lib/argparse/rte_argparse.c | 24 ++++++++++++------------
 lib/argparse/rte_argparse.h |  5 +++++
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index c98bcee56d..1a7211eb01 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -188,7 +188,7 @@ test_argparse_invalid_arg_help(void)
 static int
 test_argparse_invalid_has_val(void)
 {
-	uint32_t set_mask[] = { 0,
+	uint64_t set_mask[] = { 0,
 				RTE_ARGPARSE_ARG_NO_VALUE,
 				RTE_ARGPARSE_ARG_OPTIONAL_VALUE
 			      };
@@ -197,7 +197,7 @@ test_argparse_invalid_has_val(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	obj->args[0].flags &= ~0x3u;
+	obj->args[0].flags &= ~RTE_ARGPARSE_HAS_VAL_BITMASK;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
@@ -205,7 +205,7 @@ test_argparse_invalid_has_val(void)
 		obj = test_argparse_init_obj();
 		obj->args[0].name_long = "abc";
 		obj->args[0].name_short = NULL;
-		obj->args[0].flags &= ~0x3u;
+		obj->args[0].flags &= ~RTE_ARGPARSE_HAS_VAL_BITMASK;
 		obj->args[0].flags |= set_mask[index];
 		ret = rte_argparse_parse(obj, default_argc, default_argv);
 		TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
@@ -269,7 +269,9 @@ test_argparse_invalid_arg_flags(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	obj->args[0].flags |= ~0x107FFu;
+	obj->args[0].flags |= ~(RTE_ARGPARSE_HAS_VAL_BITMASK |
+				RTE_ARGPARSE_VAL_TYPE_BITMASK |
+				RTE_ARGPARSE_ARG_SUPPORT_MULTI);
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
@@ -337,7 +339,7 @@ test_argparse_invalid_option(void)
 static int
 test_argparse_opt_autosave_parse_int_of_no_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_NO_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_NO_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[2];
@@ -369,7 +371,7 @@ test_argparse_opt_autosave_parse_int_of_no_val(void)
 static int
 test_argparse_opt_autosave_parse_int_of_required_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[3];
@@ -410,7 +412,7 @@ test_argparse_opt_autosave_parse_int_of_required_val(void)
 static int
 test_argparse_opt_autosave_parse_int_of_optional_val(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[2];
@@ -645,7 +647,7 @@ test_argparse_opt_callback_parse_int_of_optional_val(void)
 static int
 test_argparse_pos_autosave_parse_int(void)
 {
-	uint32_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
+	uint64_t flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT;
 	struct rte_argparse *obj;
 	int val_saver = 0;
 	char *argv[3];
diff --git a/lib/argparse/rte_argparse.c b/lib/argparse/rte_argparse.c
index 6e890cdc0d..e7007afc6a 100644
--- a/lib/argparse/rte_argparse.c
+++ b/lib/argparse/rte_argparse.c
@@ -15,9 +15,6 @@ RTE_LOG_REGISTER_DEFAULT(rte_argparse_logtype, INFO);
 #define ARGPARSE_LOG(level, ...) \
 	RTE_LOG_LINE(level, ARGPARSE, "" __VA_ARGS__)
 
-#define ARG_ATTR_HAS_VAL_MASK		RTE_GENMASK64(1, 0)
-#define ARG_ATTR_VAL_TYPE_MASK		RTE_GENMASK64(9, 2)
-#define ARG_ATTR_SUPPORT_MULTI_MASK	RTE_BIT64(10)
 #define ARG_ATTR_FLAG_PARSED_MASK	RTE_BIT64(63)
 
 static inline bool
@@ -35,26 +32,27 @@ is_arg_positional(const struct rte_argparse_arg *arg)
 static inline uint32_t
 arg_attr_has_val(const struct rte_argparse_arg *arg)
 {
-	return RTE_FIELD_GET64(ARG_ATTR_HAS_VAL_MASK, arg->flags);
+	return RTE_FIELD_GET64(RTE_ARGPARSE_HAS_VAL_BITMASK, arg->flags);
 }
 
 static inline uint32_t
 arg_attr_val_type(const struct rte_argparse_arg *arg)
 {
-	return RTE_FIELD_GET64(ARG_ATTR_VAL_TYPE_MASK, arg->flags);
+	return RTE_FIELD_GET64(RTE_ARGPARSE_VAL_TYPE_BITMASK, arg->flags);
 }
 
 static inline bool
 arg_attr_flag_multi(const struct rte_argparse_arg *arg)
 {
-	return RTE_FIELD_GET64(ARG_ATTR_SUPPORT_MULTI_MASK, arg->flags);
+	return RTE_FIELD_GET64(RTE_ARGPARSE_ARG_SUPPORT_MULTI, arg->flags);
 }
 
-static inline uint32_t
+static inline uint64_t
 arg_attr_unused_bits(const struct rte_argparse_arg *arg)
 {
-#define USED_BIT_MASK	(ARG_ATTR_HAS_VAL_MASK | ARG_ATTR_VAL_TYPE_MASK | \
-			 ARG_ATTR_SUPPORT_MULTI_MASK)
+#define USED_BIT_MASK	(RTE_ARGPARSE_HAS_VAL_BITMASK | \
+			 RTE_ARGPARSE_VAL_TYPE_BITMASK | \
+			 RTE_ARGPARSE_ARG_SUPPORT_MULTI)
 	return arg->flags & ~USED_BIT_MASK;
 }
 
@@ -133,7 +131,8 @@ verify_arg_has_val(const struct rte_argparse_arg *arg)
 static int
 verify_arg_saver(const struct rte_argparse *obj, uint32_t index)
 {
-	uint32_t cmp_max = RTE_FIELD_GET64(ARG_ATTR_VAL_TYPE_MASK, RTE_ARGPARSE_ARG_VALUE_MAX);
+	uint32_t cmp_max = RTE_FIELD_GET64(RTE_ARGPARSE_VAL_TYPE_BITMASK,
+					   RTE_ARGPARSE_ARG_VALUE_MAX);
 	const struct rte_argparse_arg *arg = &obj->args[index];
 	uint32_t val_type = arg_attr_val_type(arg);
 	uint32_t has_val = arg_attr_has_val(arg);
@@ -172,7 +171,7 @@ static int
 verify_arg_flags(const struct rte_argparse *obj, uint32_t index)
 {
 	const struct rte_argparse_arg *arg = &obj->args[index];
-	uint32_t unused_bits = arg_attr_unused_bits(arg);
+	uint64_t unused_bits = arg_attr_unused_bits(arg);
 
 	if (unused_bits != 0) {
 		ARGPARSE_LOG(ERR, "argument %s flags unused bits should not be set!",
@@ -768,7 +767,8 @@ rte_argparse_parse(struct rte_argparse *obj, int argc, char **argv)
 int
 rte_argparse_parse_type(const char *str, uint64_t val_type, void *val)
 {
-	uint32_t cmp_max = RTE_FIELD_GET64(ARG_ATTR_VAL_TYPE_MASK, RTE_ARGPARSE_ARG_VALUE_MAX);
+	uint32_t cmp_max = RTE_FIELD_GET64(RTE_ARGPARSE_VAL_TYPE_BITMASK,
+					   RTE_ARGPARSE_ARG_VALUE_MAX);
 	struct rte_argparse_arg arg = {
 		.name_long = str,
 		.name_short = NULL,
diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index a6a7790cb4..98ad9971ea 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -72,6 +72,11 @@ extern "C" {
 #define RTE_ARGPARSE_ARG_RESERVED_FIELD RTE_GENMASK64(63, 48)
 /**@}*/
 
+/** Bitmask used to get the argument whether has value. */
+#define RTE_ARGPARSE_HAS_VAL_BITMASK	RTE_GENMASK64(1, 0)
+/** Bitmask used to get the argument's value type. */
+#define RTE_ARGPARSE_VAL_TYPE_BITMASK	RTE_GENMASK64(9, 2)
+
 /**
  * A structure used to hold argument's configuration.
  */
-- 
2.17.1

[PATCH v4 5/6] test/argparse: refine testcases

[Chengwen Feng]

Refine testcases, including:
1. add testcase comment.
2. argv[0] should set obj->prog_name.
3. set val_set as NULL in test_argparse_invalid_arg_flags, let it
test to the specified code logic.
4. enable index verification in opt_callback_parse_int_of_no_val.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Fixes: 31ed9f9f43bb ("argparse: parse parameters")

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/test_argparse.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index 1a7211eb01..fcea620501 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -196,11 +196,13 @@ test_argparse_invalid_has_val(void)
 	uint32_t index;
 	int ret;
 
+	/* test optional arg don't config has-value. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags &= ~RTE_ARGPARSE_HAS_VAL_BITMASK;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test positional arg don't config required-value. */
 	for (index = 0; index < RTE_DIM(set_mask); index++) {
 		obj = test_argparse_init_obj();
 		obj->args[0].name_long = "abc";
@@ -268,6 +270,7 @@ test_argparse_invalid_arg_flags(void)
 	struct rte_argparse *obj;
 	int ret;
 
+	/* test set unused bits. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags |= ~(RTE_ARGPARSE_HAS_VAL_BITMASK |
 				RTE_ARGPARSE_VAL_TYPE_BITMASK |
@@ -275,16 +278,18 @@ test_argparse_invalid_arg_flags(void)
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test positional arg should not config multiple.  */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "positional";
 	obj->args[0].name_short = NULL;
 	obj->args[0].val_saver = (void *)1;
-	obj->args[0].val_set = (void *)1;
+	obj->args[0].val_set = NULL;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_VALUE_INT |
 			     RTE_ARGPARSE_ARG_SUPPORT_MULTI;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
+	/* test optional arg enabled multiple but prased by autosave. */
 	obj = test_argparse_init_obj();
 	obj->args[0].flags |= RTE_ARGPARSE_ARG_SUPPORT_MULTI;
 	ret = rte_argparse_parse(obj, default_argc, default_argv);
@@ -322,13 +327,13 @@ test_argparse_invalid_option(void)
 	int ret;
 
 	obj = test_argparse_init_obj();
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--invalid");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
 	obj = test_argparse_init_obj();
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("invalid");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
@@ -352,7 +357,7 @@ test_argparse_opt_autosave_parse_int_of_no_val(void)
 	obj->args[0].val_set = (void *)100;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -384,7 +389,7 @@ test_argparse_opt_autosave_parse_int_of_required_val(void)
 	obj->args[0].val_set = NULL;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	argv[2] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -418,6 +423,7 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 	char *argv[2];
 	int ret;
 
+	/* test without value. */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "--test-long";
 	obj->args[0].name_short = "-t";
@@ -425,7 +431,7 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 	obj->args[0].val_set = (void *)100;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -469,7 +475,8 @@ test_argparse_opt_autosave_parse_int_of_optional_val(void)
 static int
 opt_callback_parse_int_of_no_val(uint32_t index, const char *value, void *opaque)
 {
-	RTE_SET_USED(index);
+	if (index != 1)
+		return -EINVAL;
 	if (value != NULL)
 		return -EINVAL;
 	*(int *)opaque = 100;
@@ -490,10 +497,10 @@ test_argparse_opt_callback_parse_int_of_no_val(void)
 	obj->args[0].name_long = "--test-long";
 	obj->args[0].name_short = "-t";
 	obj->args[0].val_saver = NULL;
-	obj->args[0].val_set = (void *)100;
+	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_NO_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -544,7 +551,7 @@ test_argparse_opt_callback_parse_int_of_required_val(void)
 	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	argv[2] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -608,7 +615,7 @@ test_argparse_opt_callback_parse_int_of_optional_val(void)
 	obj->args[0].val_set = (void *)1;
 	obj->args[0].flags = RTE_ARGPARSE_ARG_OPTIONAL_VALUE;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("--test-long");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
@@ -653,6 +660,7 @@ test_argparse_pos_autosave_parse_int(void)
 	char *argv[3];
 	int ret;
 
+	/* test positional autosave parse successful. */
 	obj = test_argparse_init_obj();
 	obj->args[0].name_long = "test-long";
 	obj->args[0].name_short = NULL;
@@ -660,19 +668,20 @@ test_argparse_pos_autosave_parse_int(void)
 	obj->args[0].val_set = NULL;
 	obj->args[0].flags = flags;
 	obj->args[1].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("100");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == 0, "Argparse parse expect success!");
 	TEST_ASSERT(val_saver == 100, "Argparse parse expect success!");
 
+	/* test positional autosave parse failed. */
 	obj->args[0].flags = flags;
 	val_saver = 0;
 	argv[1] = test_strdup("100a");
 	ret = rte_argparse_parse(obj, 2, argv);
 	TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!");
 
-	/* test over position parameters. */
+	/* test too much position parameters. */
 	obj->args[0].flags = flags;
 	argv[1] = test_strdup("100");
 	argv[2] = test_strdup("200");
@@ -710,6 +719,7 @@ test_argparse_pos_callback_parse_int(void)
 	char *argv[3];
 	int ret;
 
+	/* test positional callback parse successful. */
 	obj = test_argparse_init_obj();
 	obj->callback = pos_callback_parse_int;
 	obj->opaque = (void *)val_saver;
@@ -724,7 +734,7 @@ test_argparse_pos_callback_parse_int(void)
 	obj->args[1].val_set = (void *)2;
 	obj->args[1].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[2].name_long = NULL;
-	argv[0] = test_strdup(obj->usage);
+	argv[0] = test_strdup(obj->prog_name);
 	argv[1] = test_strdup("100");
 	argv[2] = test_strdup("200");
 	ret = rte_argparse_parse(obj, 3, argv);
@@ -732,7 +742,7 @@ test_argparse_pos_callback_parse_int(void)
 	TEST_ASSERT(val_saver[1] == 100, "Argparse parse expect success!");
 	TEST_ASSERT(val_saver[2] == 200, "Argparse parse expect success!");
 
-	/* test callback return failed. */
+	/* test positional callback parse failed. */
 	obj->args[0].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	obj->args[1].flags = RTE_ARGPARSE_ARG_REQUIRED_VALUE;
 	argv[2] = test_strdup("200a");
-- 
2.17.1

[PATCH v4 6/6] argparse: fix doc don't display two hyphens

[Chengwen Feng]

With the line in rst file:
	The single mode: "--aaa" or "-a".
corresponding line in html doc:
	The single mode: -aaa or -a.
the two hyphens (--aaa) become one (-aaa).

According to [1], this commit uses the backquote (``xxx``) to fix it.
And for consistency, use this format for all arguments.

Fixes: e3e579f5bab5 ("argparse: introduce argparse library")

[1] https://stackoverflow.com/questions/51075907/display-two-dashes-in-rst-file

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 doc/guides/prog_guide/argparse_lib.rst | 47 +++++++++++++-------------
 lib/argparse/rte_argparse.h            |  4 +--
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/doc/guides/prog_guide/argparse_lib.rst b/doc/guides/prog_guide/argparse_lib.rst
index a6ac11b1c0..f827312daa 100644
--- a/doc/guides/prog_guide/argparse_lib.rst
+++ b/doc/guides/prog_guide/argparse_lib.rst
@@ -90,9 +90,9 @@ The following code demonstrates how to use:
    }
 
 In this example, the arguments which start with a hyphen (-) are optional
-arguments (they're "--aaa"/"--bbb"/"--ccc"/"--ddd"/"--eee"/"--fff"); and the
-arguments which don't start with a hyphen (-) are positional arguments
-(they're "ooo"/"ppp").
+arguments (they're ``--aaa``/``--bbb``/``--ccc``/``--ddd``/``--eee``/``--fff``);
+and the arguments which don't start with a hyphen (-) are positional arguments
+(they're ``ooo``/``ppp``).
 
 Every argument must be set whether to carry a value (one of
 ``RTE_ARGPARSE_ARG_NO_VALUE``, ``RTE_ARGPARSE_ARG_REQUIRED_VALUE`` and
@@ -106,23 +106,23 @@ User Input Requirements
 ~~~~~~~~~~~~~~~~~~~~~~~
 
 For optional arguments which take no-value,
-the following mode is supported (take above "--aaa" as an example):
+the following mode is supported (take above ``--aaa`` as an example):
 
-- The single mode: "--aaa" or "-a".
+- The single mode: ``--aaa`` or ``-a``.
 
 For optional arguments which take required-value,
-the following two modes are supported (take above "--bbb" as an example):
+the following two modes are supported (take above ``--bbb`` as an example):
 
-- The kv mode: "--bbb=1234" or "-b=1234".
+- The kv mode: ``--bbb=1234`` or ``-b=1234``.
 
-- The split mode: "--bbb 1234" or "-b 1234".
+- The split mode: ``--bbb 1234`` or ``-b 1234``.
 
 For optional arguments which take optional-value,
-the following two modes are supported (take above "--ccc" as an example):
+the following two modes are supported (take above ``--ccc`` as an example):
 
-- The single mode: "--ccc" or "-c".
+- The single mode: ``--ccc`` or ``-c``.
 
-- The kv mode: "--ccc=123" or "-c=123".
+- The kv mode: ``--ccc=123`` or ``-c=123``.
 
 For positional arguments which must take required-value,
 their values are parsing in the order defined.
@@ -130,7 +130,7 @@ their values are parsing in the order defined.
 .. note::
 
    The compact mode is not supported.
-   Take above "-a" and "-d" as an example, don't support "-ad" input.
+   Take above ``-a`` and ``-d`` as an example, don't support ``-ad`` input.
 
 Parsing by autosave way
 ~~~~~~~~~~~~~~~~~~~~~~~
@@ -139,23 +139,23 @@ Argument of known value type (e.g. ``RTE_ARGPARSE_ARG_VALUE_INT``)
 could be parsed using this autosave way,
 and its result will save in the ``val_saver`` field.
 
-In the above example, the arguments "--aaa"/"--bbb"/"--ccc" and "ooo"
+In the above example, the arguments ``--aaa``/``--bbb``/``--ccc`` and ``ooo``
 both use this way, the parsing is as follows:
 
-- For argument "--aaa", it is configured as no-value,
+- For argument ``--aaa``, it is configured as no-value,
   so the ``aaa_val`` will be set to ``val_set`` field
   which is 100 in the above example.
 
-- For argument "--bbb", it is configured as required-value,
+- For argument ``--bbb``, it is configured as required-value,
   so the ``bbb_val`` will be set to user input's value
-  (e.g. will be set to 1234 with input "--bbb 1234").
+  (e.g. will be set to 1234 with input ``--bbb 1234``).
 
-- For argument "--ccc", it is configured as optional-value,
-  if user only input "--ccc" then the ``ccc_val`` will be set to ``val_set`` field
-  which is 200 in the above example;
-  if user input "--ccc=123", then the ``ccc_val`` will be set to 123.
+- For argument ``--ccc``, it is configured as optional-value,
+  if user only input ``--ccc`` then the ``ccc_val`` will be set to ``val_set``
+  field which is 200 in the above example;
+  if user input ``--ccc=123``, then the ``ccc_val`` will be set to 123.
 
-- For argument "ooo", it is positional argument,
+- For argument ``ooo``, it is positional argument,
   the ``ooo_val`` will be set to user input's value.
 
 Parsing by callback way
@@ -165,7 +165,8 @@ It could also choose to use callback to parse,
 just define a unique index for the argument
 and make the ``val_save`` field to be NULL also zero value-type.
 
-In the above example, the arguments "--ddd"/"--eee"/"--fff" and "ppp" both use this way.
+In the above example, the arguments ``--ddd``/``--eee``/``--fff`` and ``ppp``
+both use this way.
 
 Multiple times argument
 ~~~~~~~~~~~~~~~~~~~~~~~
@@ -178,7 +179,7 @@ For example:
 
    { "--xyz", "-x", "xyz argument", NULL, (void *)10, RTE_ARGPARSE_ARG_REQUIRED_VALUE | RTE_ARGPARSE_ARG_SUPPORT_MULTI },
 
-Then the user input could contain multiple "--xyz" arguments.
+Then the user input could contain multiple ``--xyz`` arguments.
 
 .. note::
 
diff --git a/lib/argparse/rte_argparse.h b/lib/argparse/rte_argparse.h
index 98ad9971ea..b6b016e388 100644
--- a/lib/argparse/rte_argparse.h
+++ b/lib/argparse/rte_argparse.h
@@ -83,8 +83,8 @@ extern "C" {
 struct rte_argparse_arg {
 	/**
 	 * Long name of the argument:
-	 * 1) If the argument is optional, it must start with '--'.
-	 * 2) If the argument is positional, it must not start with '-'.
+	 * 1) If the argument is optional, it must start with ``--``.
+	 * 2) If the argument is positional, it must not start with ``-``.
 	 * 3) Other case will be considered as error.
 	 */
 	const char *name_long;
-- 
2.17.1

Re: [PATCH v3 4/6] argparse: fix argument flags operate as uint32 type

[fengchengwen]

Hi Thomas,

On 2024/3/18 17:38, Thomas Monjalon wrote:
> 18/03/2024 10:18, Chengwen Feng:
>> --- a/app/test/test_argparse.c
>> +++ b/app/test/test_argparse.c
>> +#define TEST_ARGPARSE_FLAG_HAS_ARG_BITMASK	RTE_SHIFT_VAL64(3, 0)
>> +#define TEST_ARGPARSE_FLAG_VAL_TYPE_BITMASK	RTE_SHIFT_VAL64(255, 2)
> 
> Don't you want to make it part of the API?
> I thought it was what you did in this v3.

In v3 I defined two macros: RTE_ARGPARSE_HAS_VAL_BITMASK and RTE_ARGPARSE_VAL_TYPE_BITMASK in API file.
but forgot to remove the above two TEST_ARGPARSE_XXX_BITMASK macros in v3.

I already send v4 to fix it, please review it.

Thanks

> 
> 
> 
> .
> 

Re: [PATCH v4 0/6] refine argparse library

[Thomas Monjalon]

18/03/2024 12:18, Chengwen Feng:
> I found a couple of issues when I revisited the argparse_autotest
> output, so got this patchset.
> 
> Chengwen Feng (6):
>   argparse: refine error message
>   argparse: remove dead code
>   argparse: replace flag enum with marco
>   argparse: fix argument flags operate as uint32 type
>   test/argparse: refine testcases
>   argparse: fix doc don't display two hyphens

I'd like to get an approval from someone else for the API changes here.
I feel it is better to wait post 24.03.
Anyway it is a fresh experimental API.

Re: [PATCH v4 0/6] refine argparse library

[fengchengwen]

Hi Thomas,

On 2024/3/22 1:27, Thomas Monjalon wrote:
> 18/03/2024 12:18, Chengwen Feng:
>> I found a couple of issues when I revisited the argparse_autotest
>> output, so got this patchset.
>>
>> Chengwen Feng (6):
>>   argparse: refine error message
>>   argparse: remove dead code
>>   argparse: replace flag enum with marco
>>   argparse: fix argument flags operate as uint32 type
>>   test/argparse: refine testcases
>>   argparse: fix doc don't display two hyphens
> 
> I'd like to get an approval from someone else for the API changes here.
> I feel it is better to wait post 24.03.
> Anyway it is a fresh experimental API.

That's OK

Thanks

> 
> 
> 
> .
> 

Re: [PATCH v4 0/6] refine argparse library

[fengchengwen]

Hi Thomas,

On 2024/3/22 1:27, Thomas Monjalon wrote:
> 18/03/2024 12:18, Chengwen Feng:
>> I found a couple of issues when I revisited the argparse_autotest
>> output, so got this patchset.
>>
>> Chengwen Feng (6):
>>   argparse: refine error message
>>   argparse: remove dead code
>>   argparse: replace flag enum with marco
>>   argparse: fix argument flags operate as uint32 type
>>   test/argparse: refine testcases
>>   argparse: fix doc don't display two hyphens
> 
> I'd like to get an approval from someone else for the API changes here.
> I feel it is better to wait post 24.03.
> Anyway it is a fresh experimental API.

Could you review this patchset and merge?
BTW: this patchset also fixes bug 1409

Thanks

> 
> 
> 
> .
>