[dpdk-dev] [PATCH v1 0/8] Clean up rte_option

[dpdk-dev] [PATCH v1 0/8] Clean up rte_option

[Gaetan Rivet]

rte_option lib might benefit from a few improvements.

Overview:

  - list registered options on /rte/app --help
  - check duplicates within base EAL options
  - rename some fields to be closer to standard struct option
    and just cleaner overall.
  - fix some strange code (static iterator?)
  - fix doc, improve API a little...

Gaetan Rivet (8):
  option: use bare option string as name
  option: do not use static iterator
  option: add usage string
  option: rename name field
  option: check against common option on register
  option: fix log level of error in register
  option: improve library documentation
  option: return an error when register fail

 lib/librte_eal/common/eal_common_options.c |  1 +
 lib/librte_eal/common/eal_private.h        |  7 +++
 lib/librte_eal/common/include/rte_option.h | 17 +++++--
 lib/librte_eal/common/rte_option.c         | 57 +++++++++++++++++++---
 lib/librte_telemetry/rte_telemetry.c       |  3 +-
 5 files changed, 72 insertions(+), 13 deletions(-)

-- 
2.19.1

[dpdk-dev] [PATCH v1 1/8] option: use bare option string as name

[Gaetan Rivet]

Current options name can be passed with arbitrary format.
Force the use of "--" prefix and thus POSIX long options format.

This restricts the ability to introduce surprising options and will help
future additional checks.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/rte_option.c   | 6 +++++-
 lib/librte_telemetry/rte_telemetry.c | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
index 088e0fd23..3fbc13a6a 100644
--- a/lib/librte_eal/common/rte_option.c
+++ b/lib/librte_eal/common/rte_option.c
@@ -20,9 +20,13 @@ static struct rte_option *option;
 int
 rte_option_parse(const char *opt)
 {
+	if (strlen(opt) <= 2 ||
+	    strncmp(opt, "--", 2))
+		return -1;
+
 	/* Check if the option is registered */
 	TAILQ_FOREACH(option, &rte_option_list, next) {
-		if (strcmp(opt, option->opt_str) == 0) {
+		if (strcmp(&opt[2], option->opt_str) == 0) {
 			option->enabled = 1;
 			return 0;
 		}
diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c
index 016431f12..3080bb715 100644
--- a/lib/librte_telemetry/rte_telemetry.c
+++ b/lib/librte_telemetry/rte_telemetry.c
@@ -1798,7 +1798,7 @@ rte_telemetry_json_socket_message_test(struct telemetry_impl *telemetry, int fd)
 int telemetry_log_level;
 
 static struct rte_option option = {
-	.opt_str = "--telemetry",
+	.opt_str = "telemetry",
 	.cb = &rte_telemetry_init,
 	.enabled = 0
 };
-- 
2.19.1

[dpdk-dev] [PATCH v1 2/8] option: do not use static iterator

[Gaetan Rivet]

This is rather weird. Someone should have caught that during review.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/rte_option.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
index 3fbc13a6a..9e233f7d2 100644
--- a/lib/librte_eal/common/rte_option.c
+++ b/lib/librte_eal/common/rte_option.c
@@ -15,11 +15,11 @@ TAILQ_HEAD(rte_option_list, rte_option);
 struct rte_option_list rte_option_list =
 	TAILQ_HEAD_INITIALIZER(rte_option_list);
 
-static struct rte_option *option;
-
 int
 rte_option_parse(const char *opt)
 {
+	struct rte_option *option;
+
 	if (strlen(opt) <= 2 ||
 	    strncmp(opt, "--", 2))
 		return -1;
@@ -38,6 +38,8 @@ rte_option_parse(const char *opt)
 void __rte_experimental
 rte_option_register(struct rte_option *opt)
 {
+	struct rte_option *option;
+
 	TAILQ_FOREACH(option, &rte_option_list, next) {
 		if (strcmp(opt->opt_str, option->opt_str) == 0) {
 			RTE_LOG(INFO, EAL, "Option %s has already been registered.\n",
@@ -52,6 +54,8 @@ rte_option_register(struct rte_option *opt)
 void
 rte_option_init(void)
 {
+	struct rte_option *option;
+
 	TAILQ_FOREACH(option, &rte_option_list, next) {
 		if (option->enabled)
 			option->cb();
-- 
2.19.1

[dpdk-dev] [PATCH v1 3/8] option: add usage string

[Gaetan Rivet]

Add a usage string field in rte_option, allowing to display
help to the user and describe which options are currently available.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_options.c |  1 +
 lib/librte_eal/common/eal_private.h        |  7 +++++++
 lib/librte_eal/common/include/rte_option.h |  1 +
 lib/librte_eal/common/rte_option.c         | 17 +++++++++++++++++
 lib/librte_telemetry/rte_telemetry.c       |  1 +
 5 files changed, 27 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 6e3a83b98..e096ac896 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -1494,4 +1494,5 @@ eal_common_usage(void)
 	       "  --"OPT_NO_HPET"           Disable HPET\n"
 	       "  --"OPT_NO_SHCONF"         No shared config (mmap'd files)\n"
 	       "\n", RTE_MAX_LCORE);
+	rte_option_usage();
 }
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 442c6dc48..50ab58385 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -370,4 +370,11 @@ rte_option_parse(const char *opt);
 void
 rte_option_init(void);
 
+/**
+ * Iterate through the registered options and show the associated
+ * usage string.
+ */
+void
+rte_option_usage(void);
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/common/include/rte_option.h b/lib/librte_eal/common/include/rte_option.h
index 8957b970c..bbcc6cec9 100644
--- a/lib/librte_eal/common/include/rte_option.h
+++ b/lib/librte_eal/common/include/rte_option.h
@@ -35,6 +35,7 @@ typedef int (*rte_option_cb)(void);
 struct rte_option {
 	TAILQ_ENTRY(rte_option) next; /**< Next entry in the list. */
 	char *opt_str;             /**< The option name. */
+	const char *usage; /**< Option summary string. */
 	rte_option_cb cb;          /**< Function called when option is used. */
 	int enabled;               /**< Set when the option is used. */
 };
diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
index 9e233f7d2..2ed74873b 100644
--- a/lib/librte_eal/common/rte_option.c
+++ b/lib/librte_eal/common/rte_option.c
@@ -61,3 +61,20 @@ rte_option_init(void)
 			option->cb();
 	}
 }
+
+void
+rte_option_usage(void)
+{
+	struct rte_option *option;
+	int opt_count = 0;
+
+	TAILQ_FOREACH(option, &rte_option_list, next)
+		opt_count += 1;
+	if (opt_count == 0)
+		return;
+
+	printf("EAL dynamic options:\n");
+	TAILQ_FOREACH(option, &rte_option_list, next)
+		printf("  --%-*s %s\n", 17, option->opt_str, option->usage);
+	printf("\n");
+}
diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c
index 3080bb715..0ba6626c1 100644
--- a/lib/librte_telemetry/rte_telemetry.c
+++ b/lib/librte_telemetry/rte_telemetry.c
@@ -1799,6 +1799,7 @@ int telemetry_log_level;
 
 static struct rte_option option = {
 	.opt_str = "telemetry",
+	.usage = "Enable telemetry backend",
 	.cb = &rte_telemetry_init,
 	.enabled = 0
 };
-- 
2.19.1

[dpdk-dev] [PATCH v1 4/8] option: rename name field

[Gaetan Rivet]

option->opt_* is redundant.
The field should also be constant.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/include/rte_option.h | 2 +-
 lib/librte_eal/common/rte_option.c         | 8 ++++----
 lib/librte_telemetry/rte_telemetry.c       | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_option.h b/lib/librte_eal/common/include/rte_option.h
index bbcc6cec9..eea95e477 100644
--- a/lib/librte_eal/common/include/rte_option.h
+++ b/lib/librte_eal/common/include/rte_option.h
@@ -34,7 +34,7 @@ typedef int (*rte_option_cb)(void);
  */
 struct rte_option {
 	TAILQ_ENTRY(rte_option) next; /**< Next entry in the list. */
-	char *opt_str;             /**< The option name. */
+	const char *name; /**< The option name. */
 	const char *usage; /**< Option summary string. */
 	rte_option_cb cb;          /**< Function called when option is used. */
 	int enabled;               /**< Set when the option is used. */
diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
index 2ed74873b..d94363872 100644
--- a/lib/librte_eal/common/rte_option.c
+++ b/lib/librte_eal/common/rte_option.c
@@ -26,7 +26,7 @@ rte_option_parse(const char *opt)
 
 	/* Check if the option is registered */
 	TAILQ_FOREACH(option, &rte_option_list, next) {
-		if (strcmp(&opt[2], option->opt_str) == 0) {
+		if (strcmp(&opt[2], option->name) == 0) {
 			option->enabled = 1;
 			return 0;
 		}
@@ -41,9 +41,9 @@ rte_option_register(struct rte_option *opt)
 	struct rte_option *option;
 
 	TAILQ_FOREACH(option, &rte_option_list, next) {
-		if (strcmp(opt->opt_str, option->opt_str) == 0) {
+		if (strcmp(opt->name, option->name) == 0) {
 			RTE_LOG(INFO, EAL, "Option %s has already been registered.\n",
-					opt->opt_str);
+					opt->name);
 			return;
 		}
 	}
@@ -75,6 +75,6 @@ rte_option_usage(void)
 
 	printf("EAL dynamic options:\n");
 	TAILQ_FOREACH(option, &rte_option_list, next)
-		printf("  --%-*s %s\n", 17, option->opt_str, option->usage);
+		printf("  --%-*s %s\n", 17, option->name, option->usage);
 	printf("\n");
 }
diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c
index 0ba6626c1..3ba7ae8de 100644
--- a/lib/librte_telemetry/rte_telemetry.c
+++ b/lib/librte_telemetry/rte_telemetry.c
@@ -1798,7 +1798,7 @@ rte_telemetry_json_socket_message_test(struct telemetry_impl *telemetry, int fd)
 int telemetry_log_level;
 
 static struct rte_option option = {
-	.opt_str = "telemetry",
+	.name = "telemetry",
 	.usage = "Enable telemetry backend",
 	.cb = &rte_telemetry_init,
 	.enabled = 0
-- 
2.19.1

[dpdk-dev] [PATCH v1 5/8] option: check against common option on register

[Gaetan Rivet]

Not only check against other registered options, but also common EAL
options. This will mitigate user confusion.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/rte_option.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
index d94363872..ae8a0e2ca 100644
--- a/lib/librte_eal/common/rte_option.c
+++ b/lib/librte_eal/common/rte_option.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2018 Intel Corporation.
  */
 
+#include <getopt.h>
 #include <unistd.h>
 #include <string.h>
 
@@ -9,6 +10,8 @@
 #include <rte_option.h>
 
 #include "eal_private.h"
+#include "eal_internal_cfg.h" /* Necessary for eal_options.h */
+#include "eal_options.h"
 
 TAILQ_HEAD(rte_option_list, rte_option);
 
@@ -39,6 +42,17 @@ void __rte_experimental
 rte_option_register(struct rte_option *opt)
 {
 	struct rte_option *option;
+	const struct option *gopt;
+
+	gopt = &eal_long_options[0];
+	while (gopt->name != NULL) {
+		if (strcmp(gopt->name, opt->name) == 0) {
+			RTE_LOG(ERR, EAL, "Option %s is already a common EAL option.\n",
+					opt->name);
+			return;
+		}
+		gopt++;
+	}
 
 	TAILQ_FOREACH(option, &rte_option_list, next) {
 		if (strcmp(opt->name, option->name) == 0) {
-- 
2.19.1

[dpdk-dev] [PATCH v1 6/8] option: fix log level of error in register

[Gaetan Rivet]

INFO is not correct when logging an error.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/rte_option.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
index ae8a0e2ca..b2c93b220 100644
--- a/lib/librte_eal/common/rte_option.c
+++ b/lib/librte_eal/common/rte_option.c
@@ -56,7 +56,7 @@ rte_option_register(struct rte_option *opt)
 
 	TAILQ_FOREACH(option, &rte_option_list, next) {
 		if (strcmp(opt->name, option->name) == 0) {
-			RTE_LOG(INFO, EAL, "Option %s has already been registered.\n",
+			RTE_LOG(ERR, EAL, "Option %s has already been registered.\n",
 					opt->name);
 			return;
 		}
-- 
2.19.1

[dpdk-dev] [PATCH v1 7/8] option: improve library documentation

[Gaetan Rivet]

Use doxygen to describe the main structure and describe a little more
why it exists.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/include/rte_option.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_option.h b/lib/librte_eal/common/include/rte_option.h
index eea95e477..20be47d91 100644
--- a/lib/librte_eal/common/include/rte_option.h
+++ b/lib/librte_eal/common/include/rte_option.h
@@ -29,8 +29,12 @@ extern "C" {
 
 typedef int (*rte_option_cb)(void);
 
-/*
- * Structure describing the EAL command line option being registered.
+/**
+ * Structure describing an EAL command line option dynamically registered.
+ *
+ * Common EAL options are mostly statically defined.
+ * Some libraries need additional options to be dynamically added.
+ * This structure describes such options.
  */
 struct rte_option {
 	TAILQ_ENTRY(rte_option) next; /**< Next entry in the list. */
-- 
2.19.1

[dpdk-dev] [PATCH v1 8/8] option: return an error when register fail

[Gaetan Rivet]

Make rte_option_register return a negative value when
an error occur.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/include/rte_option.h | 6 +++++-
 lib/librte_eal/common/rte_option.c         | 8 +++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_option.h b/lib/librte_eal/common/include/rte_option.h
index 20be47d91..7ad65a4eb 100644
--- a/lib/librte_eal/common/include/rte_option.h
+++ b/lib/librte_eal/common/include/rte_option.h
@@ -57,8 +57,12 @@ struct rte_option {
  *
  * @param opt
  *  Structure describing the option to parse.
+ *
+ * @return
+ *  0 on success, <0 otherwise.
  */
-void __rte_experimental
+__rte_experimental
+int
 rte_option_register(struct rte_option *opt);
 
 #ifdef __cplusplus
diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
index b2c93b220..fe7f2ab4e 100644
--- a/lib/librte_eal/common/rte_option.c
+++ b/lib/librte_eal/common/rte_option.c
@@ -38,7 +38,8 @@ rte_option_parse(const char *opt)
 	return -1;
 }
 
-void __rte_experimental
+__rte_experimental
+int
 rte_option_register(struct rte_option *opt)
 {
 	struct rte_option *option;
@@ -49,7 +50,7 @@ rte_option_register(struct rte_option *opt)
 		if (strcmp(gopt->name, opt->name) == 0) {
 			RTE_LOG(ERR, EAL, "Option %s is already a common EAL option.\n",
 					opt->name);
-			return;
+			return -1;
 		}
 		gopt++;
 	}
@@ -58,11 +59,12 @@ rte_option_register(struct rte_option *opt)
 		if (strcmp(opt->name, option->name) == 0) {
 			RTE_LOG(ERR, EAL, "Option %s has already been registered.\n",
 					opt->name);
-			return;
+			return -1;
 		}
 	}
 
 	TAILQ_INSERT_HEAD(&rte_option_list, opt, next);
+	return 0;
 }
 
 void
-- 
2.19.1

Re: [dpdk-dev] [PATCH v1 0/8] Clean up rte_option

[Thomas Monjalon]

20/12/2018 18:06, Gaetan Rivet:
> rte_option lib might benefit from a few improvements.
> 
> Overview:
> 
>   - list registered options on /rte/app --help
>   - check duplicates within base EAL options
>   - rename some fields to be closer to standard struct option
>     and just cleaner overall.
>   - fix some strange code (static iterator?)
>   - fix doc, improve API a little...
> 
> Gaetan Rivet (8):
>   option: use bare option string as name
>   option: do not use static iterator
>   option: add usage string
>   option: rename name field
>   option: check against common option on register
>   option: fix log level of error in register
>   option: improve library documentation
>   option: return an error when register fail

Several weeks passed without any comment.

Applied, thanks