DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage
@ 2020-11-24 12:32 Ibtisam Tariq
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: " Ibtisam Tariq
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-24 12:32 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia, cristian.dumitrescu,
	jasvinder.singh, john.mcnamara, reshma.pattan,
	konstantin.ananyev, marko.kovacevic
  Cc: dev, Ibtisam Tariq

Instead of using getopt_long return value, strcmp was used to
compare the input parameters with the struct option array. This
patch get rid of all those strcmp by directly binding each longopt
with an int enum. This is to improve readability and consistency in
all examples.

Bugzilla ID: 238
Cc: marko.kovacevic@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
v3:
* None.

v2:
* Added parser_read_uint16 function to remove extra variable.
* Remove extra indentations.
* Remove extra block brackets in switch statement.
* Replace the pre-processor with actual value, if it used once.
* Change enum names to start with OPT_ and remove KEYWORD from enum names.

v1:
* enhance getopt_long usage.
---
 examples/fips_validation/fips_validation.c |  16 ++
 examples/fips_validation/fips_validation.h |   3 +
 examples/fips_validation/main.c            | 253 +++++++++++----------
 3 files changed, 157 insertions(+), 115 deletions(-)

diff --git a/examples/fips_validation/fips_validation.c b/examples/fips_validation/fips_validation.c
index 4c3ed80c8..52a7bf952 100644
--- a/examples/fips_validation/fips_validation.c
+++ b/examples/fips_validation/fips_validation.c
@@ -650,6 +650,22 @@ parser_read_uint32(uint32_t *value, char *p)
 	return 0;
 }
 
+int
+parser_read_uint16(uint16_t *value, const char *p)
+{
+	uint64_t val = 0;
+	int ret = parser_read_uint64(&val, p);
+
+	if (ret < 0)
+		return ret;
+
+	if (val > UINT16_MAX)
+		return -ERANGE;
+
+	*value = val;
+	return 0;
+}
+
 void
 parse_write_hex_str(struct fips_val *src)
 {
diff --git a/examples/fips_validation/fips_validation.h b/examples/fips_validation/fips_validation.h
index 8396932f9..6cda4e0bd 100644
--- a/examples/fips_validation/fips_validation.h
+++ b/examples/fips_validation/fips_validation.h
@@ -257,6 +257,9 @@ parser_read_uint32(uint32_t *value, char *p);
 int
 parser_read_uint32_val(const char *key, char *src, struct fips_val *val);
 
+int
+parser_read_uint16(uint16_t *value, const char *p);
+
 int
 writeback_hex_str(const char *key, char *dst, struct fips_val *val);
 
diff --git a/examples/fips_validation/main.c b/examples/fips_validation/main.c
index cad6bcb18..36ed4b546 100644
--- a/examples/fips_validation/main.c
+++ b/examples/fips_validation/main.c
@@ -15,17 +15,26 @@
 #include "fips_validation.h"
 #include "fips_dev_self_test.h"
 
-#define REQ_FILE_PATH_KEYWORD	"req-file"
-#define RSP_FILE_PATH_KEYWORD	"rsp-file"
-#define MBUF_DATAROOM_KEYWORD	"mbuf-dataroom"
-#define FOLDER_KEYWORD		"path-is-folder"
-#define CRYPTODEV_KEYWORD	"cryptodev"
-#define CRYPTODEV_ID_KEYWORD	"cryptodev-id"
-#define CRYPTODEV_ST_KEYWORD	"self-test"
-#define CRYPTODEV_BK_ID_KEYWORD	"broken-test-id"
-#define CRYPTODEV_BK_DIR_KEY	"broken-test-dir"
-#define CRYPTODEV_ENC_KEYWORD	"enc"
-#define CRYPTODEV_DEC_KEYWORD	"dec"
+enum {
+#define OPT_REQ_FILE_PATH	"req-file"
+	OPT_REQ_FILE_PATH_NUM = 256,
+#define OPT_RSP_FILE_PATH	"rsp-file"
+	OPT_RSP_FILE_PATH_NUM,
+#define OPT_MBUF_DATAROOM	"mbuf-dataroom"
+	OPT_MBUF_DATAROOM_NUM,
+#define OPT_FOLDER		    "path-is-folder"
+	OPT_FOLDER_NUM,
+#define OPT_CRYPTODEV	    "cryptodev"
+	OPT_CRYPTODEV_NUM,
+#define OPT_CRYPTODEV_ID	"cryptodev-id"
+	OPT_CRYPTODEV_ID_NUM,
+#define OPT_CRYPTODEV_ST	"self-test"
+	OPT_CRYPTODEV_ST_NUM,
+#define OPT_CRYPTODEV_BK_ID	"broken-test-id"
+	OPT_CRYPTODEV_BK_ID_NUM,
+#define OPT_CRYPTODEV_BK_DIR_KEY	"broken-test-dir"
+	OPT_CRYPTODEV_BK_DIR_KEY_NUM,
+};
 
 struct fips_test_vector vec;
 struct fips_test_interim_info info;
@@ -212,10 +221,10 @@ cryptodev_fips_validate_usage(const char *prgname)
 		"  --%s: self test indicator\n"
 		"  --%s: self broken test ID\n"
 		"  --%s: self broken test direction\n",
-		prgname, REQ_FILE_PATH_KEYWORD, RSP_FILE_PATH_KEYWORD,
-		FOLDER_KEYWORD, MBUF_DATAROOM_KEYWORD, def_mbuf_seg_size,
-		CRYPTODEV_KEYWORD, CRYPTODEV_ID_KEYWORD, CRYPTODEV_ST_KEYWORD,
-		CRYPTODEV_BK_ID_KEYWORD, CRYPTODEV_BK_DIR_KEY);
+		prgname, OPT_REQ_FILE_PATH, OPT_RSP_FILE_PATH,
+		OPT_FOLDER, OPT_MBUF_DATAROOM, def_mbuf_seg_size,
+		OPT_CRYPTODEV, OPT_CRYPTODEV_ID, OPT_CRYPTODEV_ST,
+		OPT_CRYPTODEV_BK_ID, OPT_CRYPTODEV_BK_DIR_KEY);
 }
 
 static int
@@ -226,16 +235,25 @@ cryptodev_fips_validate_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	struct option lgopts[] = {
-			{REQ_FILE_PATH_KEYWORD, required_argument, 0, 0},
-			{RSP_FILE_PATH_KEYWORD, required_argument, 0, 0},
-			{FOLDER_KEYWORD, no_argument, 0, 0},
-			{MBUF_DATAROOM_KEYWORD, required_argument, 0, 0},
-			{CRYPTODEV_KEYWORD, required_argument, 0, 0},
-			{CRYPTODEV_ID_KEYWORD, required_argument, 0, 0},
-			{CRYPTODEV_ST_KEYWORD, no_argument, 0, 0},
-			{CRYPTODEV_BK_ID_KEYWORD, required_argument, 0, 0},
-			{CRYPTODEV_BK_DIR_KEY, required_argument, 0, 0},
-			{NULL, 0, 0, 0}
+		{OPT_REQ_FILE_PATH, required_argument,
+				NULL, OPT_REQ_FILE_PATH_NUM},
+		{OPT_RSP_FILE_PATH, required_argument,
+				NULL, OPT_RSP_FILE_PATH_NUM},
+		{OPT_FOLDER, no_argument,
+				NULL, OPT_FOLDER_NUM},
+		{OPT_MBUF_DATAROOM, required_argument,
+				NULL, OPT_MBUF_DATAROOM_NUM},
+		{OPT_CRYPTODEV, required_argument,
+				NULL, OPT_CRYPTODEV_NUM},
+		{OPT_CRYPTODEV_ID, required_argument,
+				NULL, OPT_CRYPTODEV_ID_NUM},
+		{OPT_CRYPTODEV_ST, no_argument,
+				NULL, OPT_CRYPTODEV_ST_NUM},
+		{OPT_CRYPTODEV_BK_ID, required_argument,
+				NULL, OPT_CRYPTODEV_BK_ID_NUM},
+		{OPT_CRYPTODEV_BK_DIR_KEY, required_argument,
+				NULL, OPT_CRYPTODEV_BK_DIR_KEY_NUM},
+		{NULL, 0, 0, 0}
 	};
 
 	argvopt = argv;
@@ -248,108 +266,113 @@ cryptodev_fips_validate_parse_args(int argc, char **argv)
 		return -EINVAL;
 	}
 
-	while ((opt = getopt_long(argc, argvopt, "s:",
+	while ((opt = getopt_long(argc, argvopt, "",
 				  lgopts, &option_index)) != EOF) {
 
+		if (opt == '?') {
+			cryptodev_fips_validate_usage(prgname);
+			return -1;
+		}
+
 		switch (opt) {
-		case 0:
-			if (strcmp(lgopts[option_index].name,
-					REQ_FILE_PATH_KEYWORD) == 0)
-				env.req_path = optarg;
-			else if (strcmp(lgopts[option_index].name,
-					RSP_FILE_PATH_KEYWORD) == 0)
-				env.rsp_path = optarg;
-			else if (strcmp(lgopts[option_index].name,
-					FOLDER_KEYWORD) == 0)
-				env.is_path_folder = 1;
-			else if (strcmp(lgopts[option_index].name,
-					CRYPTODEV_KEYWORD) == 0) {
-				ret = parse_cryptodev_arg(optarg);
-				if (ret < 0) {
-					cryptodev_fips_validate_usage(prgname);
-					return -EINVAL;
-				}
-			} else if (strcmp(lgopts[option_index].name,
-					CRYPTODEV_ID_KEYWORD) == 0) {
-				ret = parse_cryptodev_id_arg(optarg);
-				if (ret < 0) {
-					cryptodev_fips_validate_usage(prgname);
-					return -EINVAL;
-				}
-			} else if (strcmp(lgopts[option_index].name,
-					CRYPTODEV_ST_KEYWORD) == 0) {
-				env.self_test = 1;
-			} else if (strcmp(lgopts[option_index].name,
-					CRYPTODEV_BK_ID_KEYWORD) == 0) {
-				if (!env.broken_test_config) {
-					env.broken_test_config = rte_malloc(
-						NULL,
-						sizeof(*env.broken_test_config),
-						0);
-					if (!env.broken_test_config)
-						return -ENOMEM;
-
-					env.broken_test_config->expect_fail_dir =
-						self_test_dir_enc_auth_gen;
-				}
+		case OPT_REQ_FILE_PATH_NUM:
+			env.req_path = optarg;
+			break;
 
-				if (parser_read_uint32(
-					&env.broken_test_config->expect_fail_test_idx,
-						optarg) < 0) {
-					rte_free(env.broken_test_config);
-					cryptodev_fips_validate_usage(prgname);
-					return -EINVAL;
-				}
-			} else if (strcmp(lgopts[option_index].name,
-					CRYPTODEV_BK_DIR_KEY) == 0) {
-				if (!env.broken_test_config) {
-					env.broken_test_config = rte_malloc(
-						NULL,
-						sizeof(*env.broken_test_config),
-						0);
-					if (!env.broken_test_config)
-						return -ENOMEM;
-
-					env.broken_test_config->
-						expect_fail_test_idx = 0;
-				}
+		case OPT_RSP_FILE_PATH_NUM:
+			env.rsp_path = optarg;
+			break;
 
-				if (strcmp(optarg, CRYPTODEV_ENC_KEYWORD) == 0)
-					env.broken_test_config->expect_fail_dir =
-						self_test_dir_enc_auth_gen;
-				else if (strcmp(optarg, CRYPTODEV_DEC_KEYWORD)
-						== 0)
-					env.broken_test_config->expect_fail_dir =
-						self_test_dir_dec_auth_verify;
-				else {
-					rte_free(env.broken_test_config);
-					cryptodev_fips_validate_usage(prgname);
-					return -EINVAL;
-				}
-			} else if (strcmp(lgopts[option_index].name,
-					MBUF_DATAROOM_KEYWORD) == 0) {
-				uint32_t data_room_size;
-
-				if (parser_read_uint32(&data_room_size,
-						optarg) < 0) {
-					cryptodev_fips_validate_usage(prgname);
-					return -EINVAL;
-				}
+		case OPT_FOLDER_NUM:
+			env.is_path_folder = 1;
+			break;
 
-				if (data_room_size == 0 ||
-						data_room_size > UINT16_MAX) {
-					cryptodev_fips_validate_usage(prgname);
-					return -EINVAL;
-				}
+		case OPT_CRYPTODEV_NUM:
+			ret = parse_cryptodev_arg(optarg);
+			if (ret < 0) {
+				cryptodev_fips_validate_usage(prgname);
+				return -EINVAL;
+			}
+			break;
 
-				env.mbuf_data_room = data_room_size;
-			} else {
+		case OPT_CRYPTODEV_ID_NUM:
+			ret = parse_cryptodev_id_arg(optarg);
+			if (ret < 0) {
 				cryptodev_fips_validate_usage(prgname);
 				return -EINVAL;
 			}
 			break;
+
+		case OPT_CRYPTODEV_ST_NUM:
+			env.self_test = 1;
+			break;
+
+		case OPT_CRYPTODEV_BK_ID_NUM:
+			if (!env.broken_test_config) {
+				env.broken_test_config = rte_malloc(
+					NULL,
+					sizeof(*env.broken_test_config),
+					0);
+				if (!env.broken_test_config)
+					return -ENOMEM;
+
+				env.broken_test_config->expect_fail_dir =
+					self_test_dir_enc_auth_gen;
+			}
+
+			if (parser_read_uint32(
+				&env.broken_test_config->expect_fail_test_idx,
+					optarg) < 0) {
+				rte_free(env.broken_test_config);
+				cryptodev_fips_validate_usage(prgname);
+				return -EINVAL;
+			}
+			break;
+
+		case OPT_CRYPTODEV_BK_DIR_KEY_NUM:
+			if (!env.broken_test_config) {
+				env.broken_test_config = rte_malloc(
+					NULL,
+					sizeof(*env.broken_test_config),
+					0);
+				if (!env.broken_test_config)
+					return -ENOMEM;
+
+				env.broken_test_config->expect_fail_test_idx =
+					0;
+			}
+
+			if (strcmp(optarg, "enc") == 0)
+				env.broken_test_config->expect_fail_dir =
+					self_test_dir_enc_auth_gen;
+			else if (strcmp(optarg, "dec")
+					== 0)
+				env.broken_test_config->expect_fail_dir =
+					self_test_dir_dec_auth_verify;
+			else {
+				rte_free(env.broken_test_config);
+				cryptodev_fips_validate_usage(prgname);
+				return -EINVAL;
+			}
+			break;
+
+
+		case OPT_MBUF_DATAROOM_NUM:
+			if (parser_read_uint16(&env.mbuf_data_room,
+					optarg) < 0) {
+				cryptodev_fips_validate_usage(prgname);
+				return -EINVAL;
+			}
+
+			if (env.mbuf_data_room == 0) {
+				cryptodev_fips_validate_usage(prgname);
+				return -EINVAL;
+			}
+			break;
+
 		default:
-			return -1;
+			cryptodev_fips_validate_usage(prgname);
+			return -EINVAL;
 		}
 	}
 
-- 
2.17.1


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

* [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: enhance getopt_long usage
  2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
@ 2020-11-24 12:32 ` Ibtisam Tariq
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 3/7] examples/packet_ordering: " Ibtisam Tariq
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-24 12:32 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia, cristian.dumitrescu,
	jasvinder.singh, john.mcnamara, reshma.pattan,
	konstantin.ananyev, marko.kovacevic
  Cc: dev, Ibtisam Tariq

Instead of using getopt_long return value, strcmp was used to
compare the input parameters with the struct option array. This
patch get rid of all those strcmp by directly binding each longopt
with an int enum. This is to improve readability and consistency in
all examples.

Bugzilla ID: 238
Cc: konstantin.ananyev@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
v3:
* None.

v2:
* Remove extra indentations.
* Remove extra block brackets in switch statement.
* Change enum names to start with OPT_ and remove KEYWORD from enum names.

v1:
* enhance getopt_long usage.
---
 examples/l3fwd-acl/main.c | 219 +++++++++++++++++++-------------------
 1 file changed, 110 insertions(+), 109 deletions(-)

diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c
index 961594f5f..5725963ad 100644
--- a/examples/l3fwd-acl/main.c
+++ b/examples/l3fwd-acl/main.c
@@ -195,13 +195,24 @@ send_single_packet(struct rte_mbuf *m, uint16_t port);
 #define ACL_LEAD_CHAR		('@')
 #define ROUTE_LEAD_CHAR		('R')
 #define COMMENT_LEAD_CHAR	('#')
-#define OPTION_CONFIG		"config"
-#define OPTION_NONUMA		"no-numa"
-#define OPTION_ENBJMO		"enable-jumbo"
-#define OPTION_RULE_IPV4	"rule_ipv4"
-#define OPTION_RULE_IPV6	"rule_ipv6"
-#define OPTION_ALG		"alg"
-#define OPTION_ETH_DEST		"eth-dest"
+
+enum {
+#define OPT_CONFIG		"config"
+	OPT_CONFIG_NUM = 256,
+#define OPT_NONUMA		"no-numa"
+	OPT_NONUMA_NUM,
+#define OPT_ENBJMO		"enable-jumbo"
+	OPT_ENBJMO_NUM,
+#define OPT_RULE_IPV4	"rule_ipv4"
+	OPT_RULE_IPV4_NUM,
+#define OPT_RULE_IPV6	"rule_ipv6"
+	OPT_RULE_IPV6_NUM,
+#define OPT_ALG		    "alg"
+	OPT_ALG_NUM,
+#define OPT_ETH_DEST    "eth-dest"
+	OPT_ETH_DEST_NUM,
+};
+
 #define ACL_DENY_SIGNATURE	0xf0000000
 #define RTE_LOGTYPE_L3FWDACL	RTE_LOGTYPE_USER3
 #define acl_log(format, ...)	RTE_LOG(ERR, L3FWDACL, format, ##__VA_ARGS__)
@@ -1177,9 +1188,9 @@ static void
 dump_acl_config(void)
 {
 	printf("ACL option are:\n");
-	printf(OPTION_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
-	printf(OPTION_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
-	printf(OPTION_ALG": %s\n", str_acl_alg(parm_config.alg));
+	printf(OPT_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
+	printf(OPT_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
+	printf(OPT_ALG": %s\n", str_acl_alg(parm_config.alg));
 }
 
 static int
@@ -1608,27 +1619,27 @@ print_usage(const char *prgname)
 
 	usage_acl_alg(alg, sizeof(alg));
 	printf("%s [EAL options] -- -p PORTMASK -P"
-		"--"OPTION_RULE_IPV4"=FILE"
-		"--"OPTION_RULE_IPV6"=FILE"
-		"  [--"OPTION_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
-		"  [--"OPTION_ENBJMO" [--max-pkt-len PKTLEN]]\n"
+		"--"OPT_RULE_IPV4"=FILE"
+		"--"OPT_RULE_IPV6"=FILE"
+		"  [--"OPT_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
+		"  [--"OPT_ENBJMO" [--max-pkt-len PKTLEN]]\n"
 		"  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
 		"  -P : enable promiscuous mode\n"
-		"  --"OPTION_CONFIG": (port,queue,lcore): "
+		"  --"OPT_CONFIG": (port,queue,lcore): "
 		"rx queues configuration\n"
-		"  --"OPTION_NONUMA": optional, disable numa awareness\n"
-		"  --"OPTION_ENBJMO": enable jumbo frame"
+		"  --"OPT_NONUMA": optional, disable numa awareness\n"
+		"  --"OPT_ENBJMO": enable jumbo frame"
 		" which max packet len is PKTLEN in decimal (64-9600)\n"
-		"  --"OPTION_RULE_IPV4"=FILE: specify the ipv4 rules entries "
+		"  --"OPT_RULE_IPV4"=FILE: specify the ipv4 rules entries "
 		"file. "
 		"Each rule occupy one line. "
 		"2 kinds of rules are supported. "
 		"One is ACL entry at while line leads with character '%c', "
 		"another is route entry at while line leads with "
 		"character '%c'.\n"
-		"  --"OPTION_RULE_IPV6"=FILE: specify the ipv6 rules "
+		"  --"OPT_RULE_IPV6"=FILE: specify the ipv6 rules "
 		"entries file.\n"
-		"  --"OPTION_ALG": ACL classify method to use, one of: %s\n",
+		"  --"OPT_ALG": ACL classify method to use, one of: %s\n",
 		prgname, ACL_LEAD_CHAR, ROUTE_LEAD_CHAR, alg);
 }
 
@@ -1747,14 +1758,14 @@ parse_args(int argc, char **argv)
 	int option_index;
 	char *prgname = argv[0];
 	static struct option lgopts[] = {
-		{OPTION_CONFIG, 1, 0, 0},
-		{OPTION_NONUMA, 0, 0, 0},
-		{OPTION_ENBJMO, 0, 0, 0},
-		{OPTION_RULE_IPV4, 1, 0, 0},
-		{OPTION_RULE_IPV6, 1, 0, 0},
-		{OPTION_ALG, 1, 0, 0},
-		{OPTION_ETH_DEST, 1, 0, 0},
-		{NULL, 0, 0, 0}
+		{OPT_CONFIG,    1, NULL, OPT_CONFIG_NUM    },
+		{OPT_NONUMA,    0, NULL, OPT_NONUMA_NUM    },
+		{OPT_ENBJMO,    0, NULL, OPT_ENBJMO_NUM    },
+		{OPT_RULE_IPV4, 1, NULL, OPT_RULE_IPV4_NUM },
+		{OPT_RULE_IPV6, 1, NULL, OPT_RULE_IPV6_NUM },
+		{OPT_ALG,       1, NULL, OPT_ALG_NUM       },
+		{OPT_ETH_DEST,  1, NULL, OPT_ETH_DEST_NUM  },
+		{NULL,          0, 0,    0                 }
 	};
 
 	argvopt = argv;
@@ -1772,104 +1783,94 @@ parse_args(int argc, char **argv)
 				return -1;
 			}
 			break;
+
 		case 'P':
 			printf("Promiscuous mode selected\n");
 			promiscuous_on = 1;
 			break;
 
 		/* long options */
-		case 0:
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_CONFIG,
-					sizeof(OPTION_CONFIG))) {
-				ret = parse_config(optarg);
-				if (ret) {
-					printf("invalid config\n");
-					print_usage(prgname);
-					return -1;
-				}
-			}
-
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_NONUMA,
-					sizeof(OPTION_NONUMA))) {
-				printf("numa is disabled\n");
-				numa_on = 0;
-			}
-
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_ENBJMO, sizeof(OPTION_ENBJMO))) {
-				struct option lenopts = {
-					"max-pkt-len",
-					required_argument,
-					0,
-					0
-				};
-
-				printf("jumbo frame is enabled\n");
-				port_conf.rxmode.offloads |=
-						DEV_RX_OFFLOAD_JUMBO_FRAME;
-				port_conf.txmode.offloads |=
-						DEV_TX_OFFLOAD_MULTI_SEGS;
-
-				/*
-				 * if no max-pkt-len set, then use the
-				 * default value RTE_ETHER_MAX_LEN
-				 */
-				if (0 == getopt_long(argc, argvopt, "",
-						&lenopts, &option_index)) {
-					ret = parse_max_pkt_len(optarg);
-					if ((ret < 64) ||
-						(ret > MAX_JUMBO_PKT_LEN)) {
-						printf("invalid packet "
-							"length\n");
-						print_usage(prgname);
-						return -1;
-					}
-					port_conf.rxmode.max_rx_pkt_len = ret;
-				}
-				printf("set jumbo frame max packet length "
-					"to %u\n",
-					(unsigned int)
-					port_conf.rxmode.max_rx_pkt_len);
+		case OPT_CONFIG_NUM:
+			ret = parse_config(optarg);
+			if (ret) {
+				printf("invalid config\n");
+				print_usage(prgname);
+				return -1;
 			}
+			break;
 
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_RULE_IPV4,
-					sizeof(OPTION_RULE_IPV4)))
-				parm_config.rule_ipv4_name = optarg;
-
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_RULE_IPV6,
-					sizeof(OPTION_RULE_IPV6))) {
-				parm_config.rule_ipv6_name = optarg;
-			}
+		case OPT_NONUMA_NUM:
+			printf("numa is disabled\n");
+			numa_on = 0;
+			break;
 
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_ALG, sizeof(OPTION_ALG))) {
-				parm_config.alg = parse_acl_alg(optarg);
-				if (parm_config.alg ==
-						RTE_ACL_CLASSIFY_DEFAULT) {
-					printf("unknown %s value:\"%s\"\n",
-						OPTION_ALG, optarg);
+		case OPT_ENBJMO_NUM:
+		{
+			struct option lenopts = {
+				"max-pkt-len",
+				required_argument,
+				0,
+				0
+			};
+
+			printf("jumbo frame is enabled\n");
+			port_conf.rxmode.offloads |=
+					DEV_RX_OFFLOAD_JUMBO_FRAME;
+			port_conf.txmode.offloads |=
+					DEV_TX_OFFLOAD_MULTI_SEGS;
+
+			/*
+			 * if no max-pkt-len set, then use the
+			 * default value RTE_ETHER_MAX_LEN
+			 */
+			if (0 == getopt_long(argc, argvopt, "",
+					&lenopts, &option_index)) {
+				ret = parse_max_pkt_len(optarg);
+				if ((ret < 64) ||
+					(ret > MAX_JUMBO_PKT_LEN)) {
+					printf("invalid packet "
+						"length\n");
 					print_usage(prgname);
 					return -1;
 				}
+				port_conf.rxmode.max_rx_pkt_len = ret;
 			}
+			printf("set jumbo frame max packet length "
+				"to %u\n",
+				(unsigned int)
+				port_conf.rxmode.max_rx_pkt_len);
+			break;
+		}
+		case OPT_RULE_IPV4_NUM:
+			parm_config.rule_ipv4_name = optarg;
+			break;
 
-			if (!strncmp(lgopts[option_index].name, OPTION_ETH_DEST,
-					sizeof(OPTION_ETH_DEST))) {
-				const char *serr = parse_eth_dest(optarg);
-				if (serr != NULL) {
-					printf("invalid %s value:\"%s\": %s\n",
-						OPTION_ETH_DEST, optarg, serr);
-					print_usage(prgname);
-					return -1;
-				}
-			}
+		case OPT_RULE_IPV6_NUM:
+			parm_config.rule_ipv6_name = optarg;
+			break;
 
+		case OPT_ALG_NUM:
+			parm_config.alg = parse_acl_alg(optarg);
+			if (parm_config.alg ==
+					RTE_ACL_CLASSIFY_DEFAULT) {
+				printf("unknown %s value:\"%s\"\n",
+					OPT_ALG, optarg);
+				print_usage(prgname);
+				return -1;
+			}
 			break;
 
+		case OPT_ETH_DEST_NUM:
+		{
+			const char *serr = parse_eth_dest(optarg);
+			if (serr != NULL) {
+				printf("invalid %s value:\"%s\": %s\n",
+					OPT_ETH_DEST, optarg, serr);
+				print_usage(prgname);
+				return -1;
+			}
+			break;
+		}
 		default:
 			print_usage(prgname);
 			return -1;
-- 
2.17.1


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

* [dpdk-dev] [PATCH 3/7] examples/packet_ordering: enhance getopt_long usage
  2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: " Ibtisam Tariq
@ 2020-11-24 12:32 ` Ibtisam Tariq
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 4/7] examples/performance-thread/l3fwd-thread: " Ibtisam Tariq
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-24 12:32 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia, cristian.dumitrescu,
	jasvinder.singh, john.mcnamara, reshma.pattan,
	konstantin.ananyev, marko.kovacevic
  Cc: dev, Ibtisam Tariq, sergio.gonzalez.monroy, phil.yang

Instead of using getopt_long return value, strcmp was used to
compare the input parameters with the struct option array. This
patch get rid of all those strcmp by directly binding each longopt
with an int enum. This is to improve readability and consistency in
all examples.

Bugzilla ID: 238
Cc: sergio.gonzalez.monroy@intel.com
Cc: phil.yang@arm.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
v3:
* None.

v2:
* Remove extra indentations.
* Remove extra block brackets in switch statement.
* Change enum names to start with OPT_ and remove KEYWORD from enum names.

v1:
* enhance getopt_long usage.
---
 examples/packet_ordering/main.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/examples/packet_ordering/main.c b/examples/packet_ordering/main.c
index 4bea1982d..32e58c9cd 100644
--- a/examples/packet_ordering/main.c
+++ b/examples/packet_ordering/main.c
@@ -29,6 +29,13 @@
 /* Macros for printing using RTE_LOG */
 #define RTE_LOGTYPE_REORDERAPP          RTE_LOGTYPE_USER1
 
+enum {
+#define OPT_DISABLE_REORDER "disable-reorder"
+	OPT_DISABLE_REORDER_NUM = 256,
+#define OPT_INSIGHT_WORKER "insight-worker"
+	OPT_INSIGHT_WORKER_NUM,
+};
+
 unsigned int portmask;
 unsigned int disable_reorder;
 unsigned int insight_worker;
@@ -157,9 +164,9 @@ parse_args(int argc, char **argv)
 	char **argvopt;
 	char *prgname = argv[0];
 	static struct option lgopts[] = {
-		{"disable-reorder", 0, 0, 0},
-		{"insight-worker", 0, 0, 0},
-		{NULL, 0, 0, 0}
+		{OPT_DISABLE_REORDER, 0, NULL, OPT_DISABLE_REORDER_NUM},
+		{OPT_INSIGHT_WORKER,  0, NULL, OPT_INSIGHT_WORKER_NUM },
+		{NULL,                0, 0,    0                      }
 	};
 
 	argvopt = argv;
@@ -176,18 +183,18 @@ parse_args(int argc, char **argv)
 				return -1;
 			}
 			break;
+
 		/* long options */
-		case 0:
-			if (!strcmp(lgopts[option_index].name, "disable-reorder")) {
-				printf("reorder disabled\n");
-				disable_reorder = 1;
-			}
-			if (!strcmp(lgopts[option_index].name,
-						"insight-worker")) {
-				printf("print all worker statistics\n");
-				insight_worker = 1;
-			}
+		case OPT_DISABLE_REORDER_NUM:
+			printf("reorder disabled\n");
+			disable_reorder = 1;
+			break;
+
+		case OPT_INSIGHT_WORKER_NUM:
+			printf("print all worker statistics\n");
+			insight_worker = 1;
 			break;
+
 		default:
 			print_usage(prgname);
 			return -1;
-- 
2.17.1


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

* [dpdk-dev] [PATCH 4/7] examples/performance-thread/l3fwd-thread: enhance getopt_long usage
  2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: " Ibtisam Tariq
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 3/7] examples/packet_ordering: " Ibtisam Tariq
@ 2020-11-24 12:32 ` Ibtisam Tariq
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 5/7] examples/qos_sched: " Ibtisam Tariq
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-24 12:32 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia, cristian.dumitrescu,
	jasvinder.singh, john.mcnamara, reshma.pattan,
	konstantin.ananyev, marko.kovacevic
  Cc: dev, Ibtisam Tariq, ian.betts

Instead of using getopt_long return value, strcmp was used to
compare the input parameters with the struct option array. This
patch get rid of all those strcmp by directly binding each longopt
with an int enum. This is to improve readability and consistency in
all examples.

Bugzilla ID: 238
Cc: ian.betts@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
v3:
* None.

v2:
* Remove extra indentations.
* Remove extra block brackets in switch statement.
* Change enum names to start with OPT_ and remove KEYWORD from enum names.

v1:
* enhance getopt_long usage.
---
 .../performance-thread/l3fwd-thread/main.c    | 208 +++++++++---------
 1 file changed, 106 insertions(+), 102 deletions(-)

diff --git a/examples/performance-thread/l3fwd-thread/main.c b/examples/performance-thread/l3fwd-thread/main.c
index 4d82fb82e..52efe98e5 100644
--- a/examples/performance-thread/l3fwd-thread/main.c
+++ b/examples/performance-thread/l3fwd-thread/main.c
@@ -2864,16 +2864,28 @@ parse_eth_dest(const char *optarg)
 	*(uint64_t *)(val_eth + portid) = dest_eth_addr[portid];
 }
 
-#define CMD_LINE_OPT_RX_CONFIG "rx"
-#define CMD_LINE_OPT_TX_CONFIG "tx"
-#define CMD_LINE_OPT_STAT_LCORE "stat-lcore"
-#define CMD_LINE_OPT_ETH_DEST "eth-dest"
-#define CMD_LINE_OPT_NO_NUMA "no-numa"
-#define CMD_LINE_OPT_IPV6 "ipv6"
-#define CMD_LINE_OPT_ENABLE_JUMBO "enable-jumbo"
-#define CMD_LINE_OPT_HASH_ENTRY_NUM "hash-entry-num"
-#define CMD_LINE_OPT_NO_LTHREADS "no-lthreads"
-#define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
+enum {
+#define OPT_RX_CONFIG "rx"
+	OPT_RX_CONFIG_NUM = 256,
+#define OPT_TX_CONFIG "tx"
+	OPT_TX_CONFIG_NUM,
+#define OPT_STAT_LCORE "stat-lcore"
+	OPT_STAT_LCORE_NUM,
+#define OPT_ETH_DEST "eth-dest"
+	OPT_ETH_DEST_NUM,
+#define OPT_NO_NUMA "no-numa"
+	OPT_NO_NUMA_NUM,
+#define OPT_IPV6 "ipv6"
+	OPT_IPV6_NUM,
+#define OPT_ENABLE_JUMBO "enable-jumbo"
+	OPT_ENABLE_JUMBO_NUM,
+#define OPT_HASH_ENTRY_NUM "hash-entry-num"
+	OPT_HASH_ENTRY_NUM_NUM,
+#define OPT_NO_LTHREADS "no-lthreads"
+	OPT_NO_LTHREADS_NUM,
+#define OPT_PARSE_PTYPE "parse-ptype"
+	OPT_PARSE_PTYPE_NUM,
+};
 
 /* Parse the argument given in the command line of the application */
 static int
@@ -2884,17 +2896,17 @@ parse_args(int argc, char **argv)
 	int option_index;
 	char *prgname = argv[0];
 	static struct option lgopts[] = {
-		{CMD_LINE_OPT_RX_CONFIG, 1, 0, 0},
-		{CMD_LINE_OPT_TX_CONFIG, 1, 0, 0},
-		{CMD_LINE_OPT_STAT_LCORE, 1, 0, 0},
-		{CMD_LINE_OPT_ETH_DEST, 1, 0, 0},
-		{CMD_LINE_OPT_NO_NUMA, 0, 0, 0},
-		{CMD_LINE_OPT_IPV6, 0, 0, 0},
-		{CMD_LINE_OPT_ENABLE_JUMBO, 0, 0, 0},
-		{CMD_LINE_OPT_HASH_ENTRY_NUM, 1, 0, 0},
-		{CMD_LINE_OPT_NO_LTHREADS, 0, 0, 0},
-		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
-		{NULL, 0, 0, 0}
+		{OPT_RX_CONFIG,      1, NULL, OPT_RX_CONFIG_NUM      },
+		{OPT_TX_CONFIG,      1, NULL, OPT_TX_CONFIG_NUM      },
+		{OPT_STAT_LCORE,     1, NULL, OPT_STAT_LCORE_NUM     },
+		{OPT_ETH_DEST,       1, NULL, OPT_ETH_DEST_NUM       },
+		{OPT_NO_NUMA,        0, NULL, OPT_NO_NUMA_NUM        },
+		{OPT_IPV6,           0, NULL, OPT_IPV6_NUM           },
+		{OPT_ENABLE_JUMBO,   0, NULL, OPT_ENABLE_JUMBO_NUM   },
+		{OPT_HASH_ENTRY_NUM, 1, NULL, OPT_HASH_ENTRY_NUM_NUM },
+		{OPT_NO_LTHREADS,    0, NULL, OPT_NO_LTHREADS_NUM    },
+		{OPT_PARSE_PTYPE,    0, NULL, OPT_PARSE_PTYPE_NUM    },
+		{NULL,               0, 0,    0                      }
 	};
 
 	argvopt = argv;
@@ -2912,112 +2924,104 @@ parse_args(int argc, char **argv)
 				return -1;
 			}
 			break;
+
 		case 'P':
 			printf("Promiscuous mode selected\n");
 			promiscuous_on = 1;
 			break;
 
 		/* long options */
-		case 0:
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_RX_CONFIG,
-				sizeof(CMD_LINE_OPT_RX_CONFIG))) {
-				ret = parse_rx_config(optarg);
-				if (ret) {
-					printf("invalid rx-config\n");
-					print_usage(prgname);
-					return -1;
-				}
+		case OPT_RX_CONFIG_NUM:
+			ret = parse_rx_config(optarg);
+			if (ret) {
+				printf("invalid rx-config\n");
+				print_usage(prgname);
+				return -1;
 			}
+			break;
 
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_TX_CONFIG,
-				sizeof(CMD_LINE_OPT_TX_CONFIG))) {
-				ret = parse_tx_config(optarg);
-				if (ret) {
-					printf("invalid tx-config\n");
-					print_usage(prgname);
-					return -1;
-				}
+		case OPT_TX_CONFIG_NUM:
+			ret = parse_tx_config(optarg);
+			if (ret) {
+				printf("invalid tx-config\n");
+				print_usage(prgname);
+				return -1;
 			}
+			break;
 
 #if (APP_CPU_LOAD > 0)
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_STAT_LCORE,
-					sizeof(CMD_LINE_OPT_STAT_LCORE))) {
-				cpu_load_lcore_id = parse_stat_lcore(optarg);
-			}
+		case OPT_STAT_LCORE_NUM:
+			cpu_load_lcore_id = parse_stat_lcore(optarg);
+			break;
 #endif
 
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_ETH_DEST,
-				sizeof(CMD_LINE_OPT_ETH_DEST)))
-					parse_eth_dest(optarg);
+		case OPT_ETH_DEST_NUM:
+			parse_eth_dest(optarg);
+			break;
 
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_NO_NUMA,
-				sizeof(CMD_LINE_OPT_NO_NUMA))) {
-				printf("numa is disabled\n");
-				numa_on = 0;
-			}
+		case OPT_NO_NUMA_NUM:
+			printf("numa is disabled\n");
+			numa_on = 0;
+			break;
 
 #if (APP_LOOKUP_METHOD == APP_LOOKUP_EXACT_MATCH)
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_IPV6,
-				sizeof(CMD_LINE_OPT_IPV6))) {
-				printf("ipv6 is specified\n");
-				ipv6 = 1;
-			}
+		case OPT_IPV6_NUM:
+			printf("ipv6 is specified\n");
+			ipv6 = 1;
+			break;
 #endif
 
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_NO_LTHREADS,
-					sizeof(CMD_LINE_OPT_NO_LTHREADS))) {
-				printf("l-threads model is disabled\n");
-				lthreads_on = 0;
-			}
+		case OPT_NO_LTHREADS_NUM:
+			printf("l-threads model is disabled\n");
+			lthreads_on = 0;
+			break;
 
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_PARSE_PTYPE,
-					sizeof(CMD_LINE_OPT_PARSE_PTYPE))) {
-				printf("software packet type parsing enabled\n");
-				parse_ptype_on = 1;
-			}
+		case OPT_PARSE_PTYPE_NUM:
+			printf("software packet type parsing enabled\n");
+			parse_ptype_on = 1;
+			break;
 
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_ENABLE_JUMBO,
-				sizeof(CMD_LINE_OPT_ENABLE_JUMBO))) {
-				struct option lenopts = {"max-pkt-len", required_argument, 0,
-						0};
-
-				printf("jumbo frame is enabled - disabling simple TX path\n");
-				port_conf.rxmode.offloads |=
-						DEV_RX_OFFLOAD_JUMBO_FRAME;
-				port_conf.txmode.offloads |=
-						DEV_TX_OFFLOAD_MULTI_SEGS;
-
-				/* if no max-pkt-len set, use the default value
-				 * RTE_ETHER_MAX_LEN
-				 */
-				if (0 == getopt_long(argc, argvopt, "", &lenopts,
-						&option_index)) {
-
-					ret = parse_max_pkt_len(optarg);
-					if ((ret < 64) || (ret > MAX_JUMBO_PKT_LEN)) {
-						printf("invalid packet length\n");
-						print_usage(prgname);
-						return -1;
-					}
-					port_conf.rxmode.max_rx_pkt_len = ret;
-				}
-				printf("set jumbo frame max packet length to %u\n",
-						(unsigned int)port_conf.rxmode.max_rx_pkt_len);
-			}
-#if (APP_LOOKUP_METHOD == APP_LOOKUP_EXACT_MATCH)
-			if (!strncmp(lgopts[option_index].name, CMD_LINE_OPT_HASH_ENTRY_NUM,
-				sizeof(CMD_LINE_OPT_HASH_ENTRY_NUM))) {
-				ret = parse_hash_entry_number(optarg);
-				if ((ret > 0) && (ret <= L3FWD_HASH_ENTRIES)) {
-					hash_entry_number = ret;
-				} else {
-					printf("invalid hash entry number\n");
+		case OPT_ENABLE_JUMBO_NUM:
+		{
+			struct option lenopts = {"max-pkt-len",
+					required_argument, 0, 0};
+
+			printf("jumbo frame is enabled - disabling simple TX path\n");
+			port_conf.rxmode.offloads |=
+					DEV_RX_OFFLOAD_JUMBO_FRAME;
+			port_conf.txmode.offloads |=
+					DEV_TX_OFFLOAD_MULTI_SEGS;
+
+			/* if no max-pkt-len set, use the default value
+			 * RTE_ETHER_MAX_LEN
+			 */
+			if (0 == getopt_long(argc, argvopt, "", &lenopts,
+					&option_index)) {
+
+				ret = parse_max_pkt_len(optarg);
+				if ((ret < 64) || (ret > MAX_JUMBO_PKT_LEN)) {
+					printf("invalid packet length\n");
 					print_usage(prgname);
 					return -1;
 				}
+				port_conf.rxmode.max_rx_pkt_len = ret;
 			}
-#endif
+			printf("set jumbo frame max packet length to %u\n",
+				(unsigned int)port_conf.rxmode.max_rx_pkt_len);
 			break;
+		}
+#if (APP_LOOKUP_METHOD == APP_LOOKUP_EXACT_MATCH)
+		case OPT_HASH_ENTRY_NUM_NUM:
+			ret = parse_hash_entry_number(optarg);
+			if ((ret > 0) && (ret <= L3FWD_HASH_ENTRIES)) {
+				hash_entry_number = ret;
+			} else {
+				printf("invalid hash entry number\n");
+				print_usage(prgname);
+				return -1;
+			}
+			break;
+#endif
 
 		default:
 			print_usage(prgname);
-- 
2.17.1


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

* [dpdk-dev] [PATCH 5/7] examples/qos_sched: enhance getopt_long usage
  2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (2 preceding siblings ...)
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 4/7] examples/performance-thread/l3fwd-thread: " Ibtisam Tariq
@ 2020-11-24 12:32 ` Ibtisam Tariq
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 6/7] examples/vhost: " Ibtisam Tariq
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-24 12:32 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia, cristian.dumitrescu,
	jasvinder.singh, john.mcnamara, reshma.pattan,
	konstantin.ananyev, marko.kovacevic
  Cc: dev, Ibtisam Tariq, stephen

Instead of using getopt_long return value, strcmp was used to
compare the input parameters with the struct option array. This
patch get rid of all those strcmp by directly binding each longopt
with an int enum. This is to improve readability and consistency in
all examples.

Bugzilla ID: 238
Cc: stephen@networkplumber.org

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
v3:
* Remove str_is function.

v2:
* Remove extra indentations.
* Remove extra block brackets in switch statement.
* Change enum names to start with OPT_ and remove KEYWORD from enum names.

v1:
* enhance getopt_long usage.
---
 examples/qos_sched/args.c | 149 +++++++++++++++++++++-----------------
 1 file changed, 83 insertions(+), 66 deletions(-)

diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
index c369ba9b4..10ca7bea6 100644
--- a/examples/qos_sched/args.c
+++ b/examples/qos_sched/args.c
@@ -81,10 +81,6 @@ app_usage(const char *prgname)
 		);
 }
 
-static inline int str_is(const char *str, const char *is)
-{
-	return strcmp(str, is) == 0;
-}
 
 /* returns core mask used by DPDK */
 static uint64_t
@@ -297,6 +293,25 @@ app_parse_burst_conf(const char *conf_str)
 	return 0;
 }
 
+enum {
+#define OPT_PFC "pfc"
+	OPT_PFC_NUM = 256,
+#define OPT_MNC "mnc"
+	OPT_MNC_NUM,
+#define OPT_RSZ "rsz"
+	OPT_RSZ_NUM,
+#define OPT_BSZ "bsz"
+	OPT_BSZ_NUM,
+#define OPT_MSZ "msz"
+	OPT_MSZ_NUM,
+#define OPT_RTH "rth"
+	OPT_RTH_NUM,
+#define OPT_TTH "tth"
+	OPT_TTH_NUM,
+#define OPT_CFG "cfg"
+	OPT_CFG_NUM,
+};
+
 /*
  * Parses the argument given in the command line of the application,
  * calculates mask for used cores and initializes EAL with calculated core mask
@@ -306,20 +321,19 @@ app_parse_args(int argc, char **argv)
 {
 	int opt, ret;
 	int option_index;
-	const char *optname;
 	char *prgname = argv[0];
 	uint32_t i, nb_lcores;
 
 	static struct option lgopts[] = {
-		{ "pfc", 1, 0, 0 },
-		{ "mnc", 1, 0, 0 },
-		{ "rsz", 1, 0, 0 },
-		{ "bsz", 1, 0, 0 },
-		{ "msz", 1, 0, 0 },
-		{ "rth", 1, 0, 0 },
-		{ "tth", 1, 0, 0 },
-		{ "cfg", 1, 0, 0 },
-		{ NULL,  0, 0, 0 }
+		{OPT_PFC, 1, NULL, OPT_PFC_NUM},
+		{OPT_MNC, 1, NULL, OPT_MNC_NUM},
+		{OPT_RSZ, 1, NULL, OPT_RSZ_NUM},
+		{OPT_BSZ, 1, NULL, OPT_BSZ_NUM},
+		{OPT_MSZ, 1, NULL, OPT_MSZ_NUM},
+		{OPT_RTH, 1, NULL, OPT_RTH_NUM},
+		{OPT_TTH, 1, NULL, OPT_TTH_NUM},
+		{OPT_CFG, 1, NULL, OPT_CFG_NUM},
+		{NULL,    0, 0,    0          }
 	};
 
 	/* initialize EAL first */
@@ -342,66 +356,69 @@ app_parse_args(int argc, char **argv)
 				interactive = 1;
 				break;
 			/* long options */
-			case 0:
-				optname = lgopts[option_index].name;
-				if (str_is(optname, "pfc")) {
-					ret = app_parse_flow_conf(optarg);
-					if (ret) {
-						RTE_LOG(ERR, APP, "Invalid pipe configuration %s\n", optarg);
-						return -1;
-					}
-					break;
-				}
-				if (str_is(optname, "mnc")) {
-					app_main_core = (uint32_t)atoi(optarg);
-					break;
-				}
-				if (str_is(optname, "rsz")) {
-					ret = app_parse_ring_conf(optarg);
-					if (ret) {
-						RTE_LOG(ERR, APP, "Invalid ring configuration %s\n", optarg);
-						return -1;
-					}
-					break;
+
+			case OPT_PFC_NUM:
+				ret = app_parse_flow_conf(optarg);
+				if (ret) {
+					RTE_LOG(ERR, APP, "Invalid pipe configuration %s\n",
+							optarg);
+					return -1;
 				}
-				if (str_is(optname, "bsz")) {
-					ret = app_parse_burst_conf(optarg);
-					if (ret) {
-						RTE_LOG(ERR, APP, "Invalid burst configuration %s\n", optarg);
-						return -1;
-					}
-					break;
+				break;
+
+			case OPT_MNC_NUM:
+				app_main_core = (uint32_t)atoi(optarg);
+				break;
+
+			case OPT_RSZ_NUM:
+				ret = app_parse_ring_conf(optarg);
+				if (ret) {
+					RTE_LOG(ERR, APP, "Invalid ring configuration %s\n",
+							optarg);
+					return -1;
 				}
-				if (str_is(optname, "msz")) {
-					mp_size = atoi(optarg);
-					if (mp_size <= 0) {
-						RTE_LOG(ERR, APP, "Invalid mempool size %s\n", optarg);
-						return -1;
-					}
-					break;
+				break;
+
+			case OPT_BSZ_NUM:
+				ret = app_parse_burst_conf(optarg);
+				if (ret) {
+					RTE_LOG(ERR, APP, "Invalid burst configuration %s\n",
+							optarg);
+					return -1;
 				}
-				if (str_is(optname, "rth")) {
-					ret = app_parse_rth_conf(optarg);
-					if (ret) {
-						RTE_LOG(ERR, APP, "Invalid RX threshold configuration %s\n", optarg);
-						return -1;
-					}
-					break;
+				break;
+
+			case OPT_MSZ_NUM:
+				mp_size = atoi(optarg);
+				if (mp_size <= 0) {
+					RTE_LOG(ERR, APP, "Invalid mempool size %s\n",
+							optarg);
+					return -1;
 				}
-				if (str_is(optname, "tth")) {
-					ret = app_parse_tth_conf(optarg);
-					if (ret) {
-						RTE_LOG(ERR, APP, "Invalid TX threshold configuration %s\n", optarg);
-						return -1;
-					}
-					break;
+				break;
+
+			case OPT_RTH_NUM:
+				ret = app_parse_rth_conf(optarg);
+				if (ret) {
+					RTE_LOG(ERR, APP, "Invalid RX threshold configuration %s\n",
+							optarg);
+					return -1;
 				}
-				if (str_is(optname, "cfg")) {
-					cfg_profile = optarg;
-					break;
+				break;
+
+			case OPT_TTH_NUM:
+				ret = app_parse_tth_conf(optarg);
+				if (ret) {
+					RTE_LOG(ERR, APP, "Invalid TX threshold configuration %s\n",
+							optarg);
+					return -1;
 				}
 				break;
 
+			case OPT_CFG_NUM:
+				cfg_profile = optarg;
+				break;
+
 			default:
 				app_usage(prgname);
 				return -1;
-- 
2.17.1


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

* [dpdk-dev] [PATCH 6/7] examples/vhost: enhance getopt_long usage
  2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (3 preceding siblings ...)
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 5/7] examples/qos_sched: " Ibtisam Tariq
@ 2020-11-24 12:32 ` Ibtisam Tariq
  2020-12-07 10:51   ` David Marchand
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 7/7] examples/vhost_crypto: " Ibtisam Tariq
  2020-12-07 10:50 ` [dpdk-dev] [PATCH 1/7] examples/fips_validation: " David Marchand
  6 siblings, 1 reply; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-24 12:32 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia, cristian.dumitrescu,
	jasvinder.singh, john.mcnamara, reshma.pattan,
	konstantin.ananyev, marko.kovacevic
  Cc: dev, Ibtisam Tariq, jiayu.hu, huawei.xie

Instead of using getopt_long return value, strcmp was used to
compare the input parameters with the struct option array. This
patch get rid of all those strcmp by directly binding each longopt
with an int enum. This is to improve readability and consistency in
all examples.

Bugzilla ID: 238
Cc: jiayu.hu@intel.com
Cc: huawei.xie@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
v3:
* None.

v2:
* Remove extra indentations.
* Remove extra block brackets in switch statement.
* Change enum names to start with OPT_ and remove KEYWORD from enum names.

v1:
* enhance getopt_long usage.
---
 examples/vhost/main.c | 283 ++++++++++++++++++++++--------------------
 1 file changed, 150 insertions(+), 133 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 8d8c3038b..ce8b64035 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -466,6 +466,33 @@ us_vhost_usage(const char *prgname)
 	       prgname);
 }
 
+enum {
+#define OPT_VM2VM               "vm2vm"
+	OPT_VM2VM_NUM = 256,
+#define OPT_RX_RETRY            "rx-retry"
+	OPT_RX_RETRY_NUM,
+#define OPT_RX_RETRY_DELAY      "rx-retry-delay"
+	OPT_RX_RETRY_DELAY_NUM,
+#define OPT_RX_RETRY_NUMB        "rx-retry-num"
+	OPT_RX_RETRY_NUMB_NUM,
+#define OPT_MERGEABLE           "mergeable"
+	OPT_MERGEABLE_NUM,
+#define OPT_STATS               "stats"
+	OPT_STATS_NUM,
+#define OPT_SOCKET_FILE         "socket-file"
+	OPT_SOCKET_FILE_NUM,
+#define OPT_TX_CSUM             "tx-csum"
+	OPT_TX_CSUM_NUM,
+#define OPT_TSO                 "tso"
+	OPT_TSO_NUM,
+#define OPT_CLIENT              "client"
+#define OPT_BUILTIN_NET_DRIVER  "builtin-net-driver"
+#define OPT_DMA_TYPE			"dma-type"
+	OPT_DMA_TYPE_NUM,
+#define OPT_DMAS				"dmas"
+	OPT_DMAS_NUM,
+};
+
 /*
  * Parse the arguments given in the command line of the application.
  */
@@ -477,19 +504,31 @@ us_vhost_parse_args(int argc, char **argv)
 	unsigned i;
 	const char *prgname = argv[0];
 	static struct option long_option[] = {
-		{"vm2vm", required_argument, NULL, 0},
-		{"rx-retry", required_argument, NULL, 0},
-		{"rx-retry-delay", required_argument, NULL, 0},
-		{"rx-retry-num", required_argument, NULL, 0},
-		{"mergeable", required_argument, NULL, 0},
-		{"stats", required_argument, NULL, 0},
-		{"socket-file", required_argument, NULL, 0},
-		{"tx-csum", required_argument, NULL, 0},
-		{"tso", required_argument, NULL, 0},
-		{"client", no_argument, &client_mode, 1},
-		{"builtin-net-driver", no_argument, &builtin_net_driver, 1},
-		{"dma-type", required_argument, NULL, 0},
-		{"dmas", required_argument, NULL, 0},
+		{OPT_VM2VM, required_argument,
+				NULL, OPT_VM2VM_NUM},
+		{OPT_RX_RETRY, required_argument,
+				NULL, OPT_RX_RETRY_NUM},
+		{OPT_RX_RETRY_DELAY, required_argument,
+				NULL, OPT_RX_RETRY_DELAY_NUM},
+		{OPT_RX_RETRY_NUMB, required_argument,
+				NULL, OPT_RX_RETRY_NUMB_NUM},
+		{OPT_MERGEABLE, required_argument,
+				NULL, OPT_MERGEABLE_NUM},
+		{OPT_STATS, required_argument,
+				NULL, OPT_STATS_NUM},
+		{OPT_SOCKET_FILE, required_argument,
+				NULL, OPT_SOCKET_FILE_NUM},
+		{OPT_TX_CSUM, required_argument,
+				NULL, OPT_TX_CSUM_NUM},
+		{OPT_TSO, required_argument,
+				NULL, OPT_TSO_NUM},
+		{OPT_CLIENT, no_argument, &client_mode, 1},
+		{OPT_BUILTIN_NET_DRIVER, no_argument,
+				&builtin_net_driver, 1},
+		{OPT_DMA_TYPE, required_argument,
+				NULL, OPT_DMA_TYPE_NUM},
+		{OPT_DMAS, required_argument,
+				NULL, OPT_DMAS_NUM},
 		{NULL, 0, 0, 0},
 	};
 
@@ -512,151 +551,129 @@ us_vhost_parse_args(int argc, char **argv)
 			vmdq_conf_default.rx_adv_conf.vmdq_rx_conf.rx_mode =
 				ETH_VMDQ_ACCEPT_BROADCAST |
 				ETH_VMDQ_ACCEPT_MULTICAST;
-
 			break;
 
-		case 0:
-			/* Enable/disable vm2vm comms. */
-			if (!strncmp(long_option[option_index].name, "vm2vm",
-				MAX_LONG_OPT_SZ)) {
-				ret = parse_num_opt(optarg, (VM2VM_LAST - 1));
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for "
-						"vm2vm [0|1|2]\n");
-					us_vhost_usage(prgname);
-					return -1;
-				} else {
-					vm2vm_mode = (vm2vm_type)ret;
-				}
+		case OPT_VM2VM_NUM:
+			ret = parse_num_opt(optarg, (VM2VM_LAST - 1));
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for "
+					"vm2vm [0|1|2]\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			vm2vm_mode = (vm2vm_type)ret;
+			break;
 
-			/* Enable/disable retries on RX. */
-			if (!strncmp(long_option[option_index].name, "rx-retry", MAX_LONG_OPT_SZ)) {
-				ret = parse_num_opt(optarg, 1);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for rx-retry [0|1]\n");
-					us_vhost_usage(prgname);
-					return -1;
-				} else {
-					enable_retry = ret;
-				}
+		case OPT_RX_RETRY_NUM:
+			ret = parse_num_opt(optarg, 1);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for rx-retry [0|1]\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			enable_retry = ret;
+			break;
 
-			/* Enable/disable TX checksum offload. */
-			if (!strncmp(long_option[option_index].name, "tx-csum", MAX_LONG_OPT_SZ)) {
-				ret = parse_num_opt(optarg, 1);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for tx-csum [0|1]\n");
-					us_vhost_usage(prgname);
-					return -1;
-				} else
-					enable_tx_csum = ret;
+		case OPT_TX_CSUM_NUM:
+			ret = parse_num_opt(optarg, 1);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for tx-csum [0|1]\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			enable_tx_csum = ret;
+			break;
 
-			/* Enable/disable TSO offload. */
-			if (!strncmp(long_option[option_index].name, "tso", MAX_LONG_OPT_SZ)) {
-				ret = parse_num_opt(optarg, 1);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for tso [0|1]\n");
-					us_vhost_usage(prgname);
-					return -1;
-				} else
-					enable_tso = ret;
+		case OPT_TSO_NUM:
+			ret = parse_num_opt(optarg, 1);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for tso [0|1]\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			enable_tso = ret;
+			break;
 
-			/* Specify the retries delay time (in useconds) on RX. */
-			if (!strncmp(long_option[option_index].name, "rx-retry-delay", MAX_LONG_OPT_SZ)) {
-				ret = parse_num_opt(optarg, INT32_MAX);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for rx-retry-delay [0-N]\n");
-					us_vhost_usage(prgname);
-					return -1;
-				} else {
-					burst_rx_delay_time = ret;
-				}
+		case OPT_RX_RETRY_DELAY_NUM:
+			ret = parse_num_opt(optarg, INT32_MAX);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for rx-retry-delay [0-N]\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			burst_rx_delay_time = ret;
+			break;
 
-			/* Specify the retries number on RX. */
-			if (!strncmp(long_option[option_index].name, "rx-retry-num", MAX_LONG_OPT_SZ)) {
-				ret = parse_num_opt(optarg, INT32_MAX);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for rx-retry-num [0-N]\n");
-					us_vhost_usage(prgname);
-					return -1;
-				} else {
-					burst_rx_retry_num = ret;
-				}
+		case OPT_RX_RETRY_NUMB_NUM:
+			ret = parse_num_opt(optarg, INT32_MAX);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for rx-retry-num [0-N]\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			burst_rx_retry_num = ret;
+			break;
 
-			/* Enable/disable RX mergeable buffers. */
-			if (!strncmp(long_option[option_index].name, "mergeable", MAX_LONG_OPT_SZ)) {
-				ret = parse_num_opt(optarg, 1);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for mergeable [0|1]\n");
-					us_vhost_usage(prgname);
-					return -1;
-				} else {
-					mergeable = !!ret;
-					if (ret) {
-						vmdq_conf_default.rxmode.offloads |=
-							DEV_RX_OFFLOAD_JUMBO_FRAME;
-						vmdq_conf_default.rxmode.max_rx_pkt_len
-							= JUMBO_FRAME_MAX_SIZE;
-					}
-				}
+		case OPT_MERGEABLE_NUM:
+			ret = parse_num_opt(optarg, 1);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for mergeable [0|1]\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
-
-			/* Enable/disable stats. */
-			if (!strncmp(long_option[option_index].name, "stats", MAX_LONG_OPT_SZ)) {
-				ret = parse_num_opt(optarg, INT32_MAX);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for stats [0..N]\n");
-					us_vhost_usage(prgname);
-					return -1;
-				} else {
-					enable_stats = ret;
-				}
+			mergeable = !!ret;
+			if (ret) {
+				vmdq_conf_default.rxmode.offloads |=
+					DEV_RX_OFFLOAD_JUMBO_FRAME;
+				vmdq_conf_default.rxmode.max_rx_pkt_len
+					= JUMBO_FRAME_MAX_SIZE;
 			}
+			break;
 
-			/* Set socket file path. */
-			if (!strncmp(long_option[option_index].name,
-						"socket-file", MAX_LONG_OPT_SZ)) {
-				if (us_vhost_parse_socket_path(optarg) == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-					"Invalid argument for socket name (Max %d characters)\n",
-					PATH_MAX);
-					us_vhost_usage(prgname);
-					return -1;
-				}
+		case OPT_STATS_NUM:
+			ret = parse_num_opt(optarg, INT32_MAX);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for stats [0..N]\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			enable_stats = ret;
+			break;
 
-			if (!strncmp(long_option[option_index].name,
-						"dma-type", MAX_LONG_OPT_SZ)) {
-				if (strlen(optarg) >= MAX_LONG_OPT_SZ) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Wrong DMA type\n");
-					us_vhost_usage(prgname);
-					return -1;
-				}
-				strcpy(dma_type, optarg);
+		/* Set socket file path. */
+		case OPT_SOCKET_FILE_NUM:
+			if (us_vhost_parse_socket_path(optarg) == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+				"Invalid argument for socket name (Max %d characters)\n",
+				PATH_MAX);
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			break;
 
-			if (!strncmp(long_option[option_index].name,
-						"dmas", MAX_LONG_OPT_SZ)) {
-				if (open_dma(optarg) == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Wrong DMA args\n");
-					us_vhost_usage(prgname);
-					return -1;
-				}
-				async_vhost_driver = 1;
+		case OPT_DMA_TYPE_NUM:
+			if (strlen(optarg) >= MAX_LONG_OPT_SZ) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Wrong DMA type\n");
+				us_vhost_usage(prgname);
+				return -1;
 			}
+			strcpy(dma_type, optarg);
+			break;
 
+		case OPT_DMAS_NUM:
+			if (open_dma(optarg) == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Wrong DMA args\n");
+				us_vhost_usage(prgname);
+				return -1;
+			}
+			async_vhost_driver = 1;
 			break;
 
-			/* Invalid option - print options. */
+		/* Invalid option - print options. */
 		default:
 			us_vhost_usage(prgname);
 			return -1;
-- 
2.17.1


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

* [dpdk-dev] [PATCH 7/7] examples/vhost_crypto: enhance getopt_long usage
  2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (4 preceding siblings ...)
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 6/7] examples/vhost: " Ibtisam Tariq
@ 2020-11-24 12:32 ` Ibtisam Tariq
  2020-12-07 10:50 ` [dpdk-dev] [PATCH 1/7] examples/fips_validation: " David Marchand
  6 siblings, 0 replies; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-24 12:32 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia, cristian.dumitrescu,
	jasvinder.singh, john.mcnamara, reshma.pattan,
	konstantin.ananyev, marko.kovacevic
  Cc: dev, Ibtisam Tariq, roy.fan.zhang

Instead of using getopt_long return value, strcmp was used to
compare the input parameters with the struct option array. This
patch get rid of all those strcmp by directly binding each longopt
with an int enum. This is to improve readability and consistency in
all examples.

Bugzilla ID: 238
Cc: roy.fan.zhang@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
v3:
* None.

v2:
* Remove extra indentations.
* Remove extra block brackets in switch statement.
* Change enum names to start with OPT_ and remove KEYWORD from enum names.
* Remove unused short options.

v1:
* enhance getopt_long usage.
---
 examples/vhost_crypto/main.c | 90 +++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 38 deletions(-)

diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
index 29c8f7228..d731751eb 100644
--- a/examples/vhost_crypto/main.c
+++ b/examples/vhost_crypto/main.c
@@ -62,10 +62,16 @@ struct vhost_crypto_options {
 	uint32_t guest_polling;
 } options;
 
-#define CONFIG_KEYWORD		"config"
-#define SOCKET_FILE_KEYWORD	"socket-file"
-#define ZERO_COPY_KEYWORD	"zero-copy"
-#define POLLING_KEYWORD		"guest-polling"
+enum {
+#define OPT_CONFIG		"config"
+	OPT_CONFIG_NUM = 256,
+#define OPT_SOCKET_FILE	"socket-file"
+	OPT_SOCKET_FILE_NUM,
+#define OPT_ZERO_COPY	"zero-copy"
+	OPT_ZERO_COPY_NUM,
+#define OPT_POLLING		"guest-polling"
+	OPT_POLLING_NUM,
+};
 
 #define NB_SOCKET_FIELDS	(2)
 
@@ -195,11 +201,11 @@ vhost_crypto_usage(const char *prgname)
 {
 	printf("%s [EAL options] --\n"
 		"  --%s <lcore>,SOCKET-FILE-PATH\n"
-		"  --%s (lcore,cdev_id,queue_id)[,(lcore,cdev_id,queue_id)]\n"
+		"  --%s (lcore,cdev_id,queue_id)[,(lcore,cdev_id,queue_id)]"
 		"  --%s: zero copy\n"
 		"  --%s: guest polling\n",
-		prgname, SOCKET_FILE_KEYWORD, CONFIG_KEYWORD,
-		ZERO_COPY_KEYWORD, POLLING_KEYWORD);
+		prgname, OPT_SOCKET_FILE, OPT_CONFIG,
+		OPT_ZERO_COPY, OPT_POLLING);
 }
 
 static int
@@ -210,48 +216,56 @@ vhost_crypto_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	struct option lgopts[] = {
-			{SOCKET_FILE_KEYWORD, required_argument, 0, 0},
-			{CONFIG_KEYWORD, required_argument, 0, 0},
-			{ZERO_COPY_KEYWORD, no_argument, 0, 0},
-			{POLLING_KEYWORD, no_argument, 0, 0},
-			{NULL, 0, 0, 0}
+		{OPT_SOCKET_FILE, required_argument,
+				NULL, OPT_SOCKET_FILE_NUM},
+		{OPT_CONFIG, required_argument,
+				NULL, OPT_CONFIG_NUM},
+		{OPT_ZERO_COPY, no_argument,
+				NULL, OPT_ZERO_COPY_NUM},
+		{OPT_POLLING, no_argument,
+				NULL, OPT_POLLING_NUM},
+		{NULL, 0, 0, 0}
 	};
 
 	argvopt = argv;
 
-	while ((opt = getopt_long(argc, argvopt, "s:",
+	while ((opt = getopt_long(argc, argvopt, "",
 				  lgopts, &option_index)) != EOF) {
 
+		if (opt == '?') {
+			vhost_crypto_usage(prgname);
+			return -1;
+		}
+
 		switch (opt) {
-		case 0:
-			if (strcmp(lgopts[option_index].name,
-					SOCKET_FILE_KEYWORD) == 0) {
-				ret = parse_socket_arg(optarg);
-				if (ret < 0) {
-					vhost_crypto_usage(prgname);
-					return ret;
-				}
-			} else if (strcmp(lgopts[option_index].name,
-					CONFIG_KEYWORD) == 0) {
-				ret = parse_config(optarg);
-				if (ret < 0) {
-					vhost_crypto_usage(prgname);
-					return ret;
-				}
-			} else if (strcmp(lgopts[option_index].name,
-					ZERO_COPY_KEYWORD) == 0) {
-				options.zero_copy =
-					RTE_VHOST_CRYPTO_ZERO_COPY_ENABLE;
-			} else if (strcmp(lgopts[option_index].name,
-					POLLING_KEYWORD) == 0) {
-				options.guest_polling = 1;
-			} else {
+		case OPT_SOCKET_FILE_NUM:
+			ret = parse_socket_arg(optarg);
+			if (ret < 0) {
 				vhost_crypto_usage(prgname);
-				return -EINVAL;
+				return ret;
+			}
+			break;
+
+		case OPT_CONFIG_NUM:
+			ret = parse_config(optarg);
+			if (ret < 0) {
+				vhost_crypto_usage(prgname);
+				return ret;
 			}
 			break;
+
+		case OPT_ZERO_COPY_NUM:
+			options.zero_copy =
+				RTE_VHOST_CRYPTO_ZERO_COPY_ENABLE;
+			break;
+
+		case OPT_POLLING_NUM:
+			options.guest_polling = 1;
+			break;
+
 		default:
-			return -1;
+			vhost_crypto_usage(prgname);
+			return -EINVAL;
 		}
 	}
 
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage
  2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (5 preceding siblings ...)
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 7/7] examples/vhost_crypto: " Ibtisam Tariq
@ 2020-12-07 10:50 ` David Marchand
  6 siblings, 0 replies; 13+ messages in thread
From: David Marchand @ 2020-12-07 10:50 UTC (permalink / raw)
  To: Ibtisam Tariq
  Cc: Maxime Coquelin, Xia, Chenbo, Cristian Dumitrescu, Singh,
	Jasvinder, Mcnamara, John, Pattan, Reshma, Ananyev, Konstantin,
	Kovacevic, Marko, dev

On Tue, Nov 24, 2020 at 1:33 PM Ibtisam Tariq <ibtisam.tariq@emumba.com> wrote:
>
> Instead of using getopt_long return value, strcmp was used to
> compare the input parameters with the struct option array. This
> patch get rid of all those strcmp by directly binding each longopt
> with an int enum. This is to improve readability and consistency in
> all examples.
>
> Bugzilla ID: 238
> Cc: marko.kovacevic@intel.com
>
> Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
> ---
> v3:
> * None.

We lost the version prefix in the patch title, please do not forget it
in the next revision.

[snip]

> diff --git a/examples/fips_validation/main.c b/examples/fips_validation/main.c
> index cad6bcb18..36ed4b546 100644
> --- a/examples/fips_validation/main.c
> +++ b/examples/fips_validation/main.c
> @@ -15,17 +15,26 @@
>  #include "fips_validation.h"
>  #include "fips_dev_self_test.h"
>
> -#define REQ_FILE_PATH_KEYWORD  "req-file"
> -#define RSP_FILE_PATH_KEYWORD  "rsp-file"
> -#define MBUF_DATAROOM_KEYWORD  "mbuf-dataroom"
> -#define FOLDER_KEYWORD         "path-is-folder"
> -#define CRYPTODEV_KEYWORD      "cryptodev"
> -#define CRYPTODEV_ID_KEYWORD   "cryptodev-id"
> -#define CRYPTODEV_ST_KEYWORD   "self-test"
> -#define CRYPTODEV_BK_ID_KEYWORD        "broken-test-id"
> -#define CRYPTODEV_BK_DIR_KEY   "broken-test-dir"
> -#define CRYPTODEV_ENC_KEYWORD  "enc"
> -#define CRYPTODEV_DEC_KEYWORD  "dec"
> +enum {
> +#define OPT_REQ_FILE_PATH      "req-file"
> +       OPT_REQ_FILE_PATH_NUM = 256,
> +#define OPT_RSP_FILE_PATH      "rsp-file"
> +       OPT_RSP_FILE_PATH_NUM,
> +#define OPT_MBUF_DATAROOM      "mbuf-dataroom"
> +       OPT_MBUF_DATAROOM_NUM,
> +#define OPT_FOLDER                 "path-is-folder"
> +       OPT_FOLDER_NUM,
> +#define OPT_CRYPTODEV      "cryptodev"
> +       OPT_CRYPTODEV_NUM,

Nit: could you realign those two strings?

> +#define OPT_CRYPTODEV_ID       "cryptodev-id"
> +       OPT_CRYPTODEV_ID_NUM,
> +#define OPT_CRYPTODEV_ST       "self-test"
> +       OPT_CRYPTODEV_ST_NUM,
> +#define OPT_CRYPTODEV_BK_ID    "broken-test-id"
> +       OPT_CRYPTODEV_BK_ID_NUM,
> +#define OPT_CRYPTODEV_BK_DIR_KEY       "broken-test-dir"
> +       OPT_CRYPTODEV_BK_DIR_KEY_NUM,
> +};

[snip]

> @@ -248,108 +266,113 @@ cryptodev_fips_validate_parse_args(int argc, char **argv)
>                 return -EINVAL;
>         }
>
> -       while ((opt = getopt_long(argc, argvopt, "s:",
> +       while ((opt = getopt_long(argc, argvopt, "",

Passing "s:" was a bug (since nothing was done with it).
But this was not an issue requiring a separate fix + backport from my pov.
Let's at least mention it in the commitlog.


>                                   lgopts, &option_index)) != EOF) {
>
> +               if (opt == '?') {
> +                       cryptodev_fips_validate_usage(prgname);
> +                       return -1;
> +               }

Why a separate check here?
The default: block below will handle an unknown option fine.

> +
>                 switch (opt) {

[snip]


> +
>                 default:
> -                       return -1;
> +                       cryptodev_fips_validate_usage(prgname);
> +                       return -EINVAL;
>                 }
>         }
>


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH 6/7] examples/vhost: enhance getopt_long usage
  2020-11-24 12:32 ` [dpdk-dev] [PATCH 6/7] examples/vhost: " Ibtisam Tariq
@ 2020-12-07 10:51   ` David Marchand
  2020-12-31  7:00     ` Ibtisam Tariq
  0 siblings, 1 reply; 13+ messages in thread
From: David Marchand @ 2020-12-07 10:51 UTC (permalink / raw)
  To: Ibtisam Tariq, Maxime Coquelin
  Cc: Xia, Chenbo, Cristian Dumitrescu, Singh, Jasvinder, Mcnamara,
	John, Pattan, Reshma, Ananyev, Konstantin, Kovacevic, Marko, dev,
	Jiayu Hu, huawei.xie

On Tue, Nov 24, 2020 at 1:35 PM Ibtisam Tariq <ibtisam.tariq@emumba.com> wrote:
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index 8d8c3038b..ce8b64035 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -466,6 +466,33 @@ us_vhost_usage(const char *prgname)
>                prgname);
>  }
>
> +enum {
> +#define OPT_VM2VM               "vm2vm"
> +       OPT_VM2VM_NUM = 256,
> +#define OPT_RX_RETRY            "rx-retry"
> +       OPT_RX_RETRY_NUM,
> +#define OPT_RX_RETRY_DELAY      "rx-retry-delay"
> +       OPT_RX_RETRY_DELAY_NUM,
> +#define OPT_RX_RETRY_NUMB        "rx-retry-num"
> +       OPT_RX_RETRY_NUMB_NUM,
> +#define OPT_MERGEABLE           "mergeable"
> +       OPT_MERGEABLE_NUM,
> +#define OPT_STATS               "stats"
> +       OPT_STATS_NUM,
> +#define OPT_SOCKET_FILE         "socket-file"
> +       OPT_SOCKET_FILE_NUM,
> +#define OPT_TX_CSUM             "tx-csum"
> +       OPT_TX_CSUM_NUM,
> +#define OPT_TSO                 "tso"
> +       OPT_TSO_NUM,
> +#define OPT_CLIENT              "client"
> +#define OPT_BUILTIN_NET_DRIVER  "builtin-net-driver"
> +#define OPT_DMA_TYPE                   "dma-type"
> +       OPT_DMA_TYPE_NUM,
> +#define OPT_DMAS                               "dmas"
> +       OPT_DMAS_NUM,

Nit: could you align those last two strings?


> +};
> +
>  /*
>   * Parse the arguments given in the command line of the application.
>   */
> @@ -477,19 +504,31 @@ us_vhost_parse_args(int argc, char **argv)
>         unsigned i;
>         const char *prgname = argv[0];
>         static struct option long_option[] = {
> -               {"vm2vm", required_argument, NULL, 0},
> -               {"rx-retry", required_argument, NULL, 0},
> -               {"rx-retry-delay", required_argument, NULL, 0},
> -               {"rx-retry-num", required_argument, NULL, 0},
> -               {"mergeable", required_argument, NULL, 0},
> -               {"stats", required_argument, NULL, 0},
> -               {"socket-file", required_argument, NULL, 0},
> -               {"tx-csum", required_argument, NULL, 0},
> -               {"tso", required_argument, NULL, 0},
> -               {"client", no_argument, &client_mode, 1},
> -               {"builtin-net-driver", no_argument, &builtin_net_driver, 1},
> -               {"dma-type", required_argument, NULL, 0},
> -               {"dmas", required_argument, NULL, 0},
> +               {OPT_VM2VM, required_argument,
> +                               NULL, OPT_VM2VM_NUM},
> +               {OPT_RX_RETRY, required_argument,
> +                               NULL, OPT_RX_RETRY_NUM},
> +               {OPT_RX_RETRY_DELAY, required_argument,
> +                               NULL, OPT_RX_RETRY_DELAY_NUM},
> +               {OPT_RX_RETRY_NUMB, required_argument,
> +                               NULL, OPT_RX_RETRY_NUMB_NUM},
> +               {OPT_MERGEABLE, required_argument,
> +                               NULL, OPT_MERGEABLE_NUM},
> +               {OPT_STATS, required_argument,
> +                               NULL, OPT_STATS_NUM},
> +               {OPT_SOCKET_FILE, required_argument,
> +                               NULL, OPT_SOCKET_FILE_NUM},
> +               {OPT_TX_CSUM, required_argument,
> +                               NULL, OPT_TX_CSUM_NUM},
> +               {OPT_TSO, required_argument,
> +                               NULL, OPT_TSO_NUM},
> +               {OPT_CLIENT, no_argument, &client_mode, 1},
> +               {OPT_BUILTIN_NET_DRIVER, no_argument,
> +                               &builtin_net_driver, 1},

We will have an issue here.

Quoting the manual:
"""
       flag   specifies  how  results are returned for a long option.
If flag is NULL, then getopt_long() returns val.  (For example, the
calling program may set val to the equivalent short option charac‐
              ter.)  Otherwise, getopt_long() returns 0, and flag
points to a variable which is set to val if the option is found, but
left unchanged if the option is not found.
"""

$ build/examples/dpdk-vhost --no-huge -m 1024 -l 0,1 -a 0:0.0 --
--builtin-net-driver --socket-file /tmp/sock0 -p 0x1
EAL: Detected 8 lcore(s)
EAL: Detected 1 NUMA nodes
EAL: Multi-process socket /run/user/114840/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
EAL: Probing VFIO support...
VHOST_CONFIG: build/examples/dpdk-vhost [EAL options] -- -p PORTMASK
        --vm2vm [0|1|2]
        --rx_retry [0|1] --mergeable [0|1] --stats [0-N]
        --socket-file <path>
        --nb-devices ND
        -p PORTMASK: Set mask for ports to be used by application
        --vm2vm [0|1|2]: disable/software(default)/hardware vm2vm comms
        --rx-retry [0|1]: disable/enable(default) retries on rx.
Enable retry if destintation queue is full
        --rx-retry-delay [0-N]: timeout(in usecond) between retries on
RX. This makes effect only if retries on rx enabled
        --rx-retry-num [0-N]: the number of retries on rx. This makes
effect only if retries on rx enabled
        --mergeable [0|1]: disable(default)/enable RX mergeable buffers
        --stats [0-N]: 0: Disable stats, N: Time in seconds to print stats
        --socket-file: The path of the socket file.
        --tx-csum [0|1] disable/enable TX checksum offload.
        --tso [0|1] disable/enable TCP segment offload.
        --client register a vhost-user socket as client mode.
        --dma-type register dma type for your vhost async driver. For
example "ioat" for now.
        --dmas register dma channel for specific vhost device.
EAL: Error - exiting with code: 1
  Cause: Invalid argument

This is because getopt_long returned 0.

We have two solutions:
- we keep this &builtin_net_driver flag use, but we must accept a 0
return value from getopt_long.
- we convert this option to the same scheme as the other and add a
OPT_BUILTIN_NET_DRIVER_NUM like other options in this example,

Maxime, opinion?


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH 6/7] examples/vhost: enhance getopt_long usage
  2020-12-07 10:51   ` David Marchand
@ 2020-12-31  7:00     ` Ibtisam Tariq
  0 siblings, 0 replies; 13+ messages in thread
From: Ibtisam Tariq @ 2020-12-31  7:00 UTC (permalink / raw)
  To: David Marchand
  Cc: Maxime Coquelin, Xia, Chenbo, Cristian Dumitrescu, Singh,
	Jasvinder, Mcnamara, John, Pattan, Reshma, Ananyev, Konstantin,
	Kovacevic, Marko, dev, Jiayu Hu, huawei.xie

Hi,
Sorry, for the late reply.

We have two solutions:
> - we keep this &builtin_net_driver flag use, but we must accept a 0
> return value from getopt_long.
> - we convert this option to the same scheme as the other and add a
> OPT_BUILTIN_NET_DRIVER_NUM like other options in this example,
>
What should I do for built_net_driver. The convention used in eal is the
2nd option.

On Mon, Dec 7, 2020 at 3:51 PM David Marchand <david.marchand@redhat.com>
wrote:

> On Tue, Nov 24, 2020 at 1:35 PM Ibtisam Tariq <ibtisam.tariq@emumba.com>
> wrote:
> > diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> > index 8d8c3038b..ce8b64035 100644
> > --- a/examples/vhost/main.c
> > +++ b/examples/vhost/main.c
> > @@ -466,6 +466,33 @@ us_vhost_usage(const char *prgname)
> >                prgname);
> >  }
> >
> > +enum {
> > +#define OPT_VM2VM               "vm2vm"
> > +       OPT_VM2VM_NUM = 256,
> > +#define OPT_RX_RETRY            "rx-retry"
> > +       OPT_RX_RETRY_NUM,
> > +#define OPT_RX_RETRY_DELAY      "rx-retry-delay"
> > +       OPT_RX_RETRY_DELAY_NUM,
> > +#define OPT_RX_RETRY_NUMB        "rx-retry-num"
> > +       OPT_RX_RETRY_NUMB_NUM,
> > +#define OPT_MERGEABLE           "mergeable"
> > +       OPT_MERGEABLE_NUM,
> > +#define OPT_STATS               "stats"
> > +       OPT_STATS_NUM,
> > +#define OPT_SOCKET_FILE         "socket-file"
> > +       OPT_SOCKET_FILE_NUM,
> > +#define OPT_TX_CSUM             "tx-csum"
> > +       OPT_TX_CSUM_NUM,
> > +#define OPT_TSO                 "tso"
> > +       OPT_TSO_NUM,
> > +#define OPT_CLIENT              "client"
> > +#define OPT_BUILTIN_NET_DRIVER  "builtin-net-driver"
> > +#define OPT_DMA_TYPE                   "dma-type"
> > +       OPT_DMA_TYPE_NUM,
> > +#define OPT_DMAS                               "dmas"
> > +       OPT_DMAS_NUM,
>
> Nit: could you align those last two strings?
>
>
> > +};
> > +
> >  /*
> >   * Parse the arguments given in the command line of the application.
> >   */
> > @@ -477,19 +504,31 @@ us_vhost_parse_args(int argc, char **argv)
> >         unsigned i;
> >         const char *prgname = argv[0];
> >         static struct option long_option[] = {
> > -               {"vm2vm", required_argument, NULL, 0},
> > -               {"rx-retry", required_argument, NULL, 0},
> > -               {"rx-retry-delay", required_argument, NULL, 0},
> > -               {"rx-retry-num", required_argument, NULL, 0},
> > -               {"mergeable", required_argument, NULL, 0},
> > -               {"stats", required_argument, NULL, 0},
> > -               {"socket-file", required_argument, NULL, 0},
> > -               {"tx-csum", required_argument, NULL, 0},
> > -               {"tso", required_argument, NULL, 0},
> > -               {"client", no_argument, &client_mode, 1},
> > -               {"builtin-net-driver", no_argument, &builtin_net_driver,
> 1},
> > -               {"dma-type", required_argument, NULL, 0},
> > -               {"dmas", required_argument, NULL, 0},
> > +               {OPT_VM2VM, required_argument,
> > +                               NULL, OPT_VM2VM_NUM},
> > +               {OPT_RX_RETRY, required_argument,
> > +                               NULL, OPT_RX_RETRY_NUM},
> > +               {OPT_RX_RETRY_DELAY, required_argument,
> > +                               NULL, OPT_RX_RETRY_DELAY_NUM},
> > +               {OPT_RX_RETRY_NUMB, required_argument,
> > +                               NULL, OPT_RX_RETRY_NUMB_NUM},
> > +               {OPT_MERGEABLE, required_argument,
> > +                               NULL, OPT_MERGEABLE_NUM},
> > +               {OPT_STATS, required_argument,
> > +                               NULL, OPT_STATS_NUM},
> > +               {OPT_SOCKET_FILE, required_argument,
> > +                               NULL, OPT_SOCKET_FILE_NUM},
> > +               {OPT_TX_CSUM, required_argument,
> > +                               NULL, OPT_TX_CSUM_NUM},
> > +               {OPT_TSO, required_argument,
> > +                               NULL, OPT_TSO_NUM},
> > +               {OPT_CLIENT, no_argument, &client_mode, 1},
> > +               {OPT_BUILTIN_NET_DRIVER, no_argument,
> > +                               &builtin_net_driver, 1},
>
> We will have an issue here.
>
> Quoting the manual:
> """
>        flag   specifies  how  results are returned for a long option.
> If flag is NULL, then getopt_long() returns val.  (For example, the
> calling program may set val to the equivalent short option charac‐
>               ter.)  Otherwise, getopt_long() returns 0, and flag
> points to a variable which is set to val if the option is found, but
> left unchanged if the option is not found.
> """
>
> $ build/examples/dpdk-vhost --no-huge -m 1024 -l 0,1 -a 0:0.0 --
> --builtin-net-driver --socket-file /tmp/sock0 -p 0x1
> EAL: Detected 8 lcore(s)
> EAL: Detected 1 NUMA nodes
> EAL: Multi-process socket /run/user/114840/dpdk/rte/mp_socket
> EAL: Selected IOVA mode 'VA'
> EAL: Probing VFIO support...
> VHOST_CONFIG: build/examples/dpdk-vhost [EAL options] -- -p PORTMASK
>         --vm2vm [0|1|2]
>         --rx_retry [0|1] --mergeable [0|1] --stats [0-N]
>         --socket-file <path>
>         --nb-devices ND
>         -p PORTMASK: Set mask for ports to be used by application
>         --vm2vm [0|1|2]: disable/software(default)/hardware vm2vm comms
>         --rx-retry [0|1]: disable/enable(default) retries on rx.
> Enable retry if destintation queue is full
>         --rx-retry-delay [0-N]: timeout(in usecond) between retries on
> RX. This makes effect only if retries on rx enabled
>         --rx-retry-num [0-N]: the number of retries on rx. This makes
> effect only if retries on rx enabled
>         --mergeable [0|1]: disable(default)/enable RX mergeable buffers
>         --stats [0-N]: 0: Disable stats, N: Time in seconds to print stats
>         --socket-file: The path of the socket file.
>         --tx-csum [0|1] disable/enable TX checksum offload.
>         --tso [0|1] disable/enable TCP segment offload.
>         --client register a vhost-user socket as client mode.
>         --dma-type register dma type for your vhost async driver. For
> example "ioat" for now.
>         --dmas register dma channel for specific vhost device.
> EAL: Error - exiting with code: 1
>   Cause: Invalid argument
>
> This is because getopt_long returned 0.
>
> We have two solutions:
> - we keep this &builtin_net_driver flag use, but we must accept a 0
> return value from getopt_long.
> - we convert this option to the same scheme as the other and add a
> OPT_BUILTIN_NET_DRIVER_NUM like other options in this example,
>
> Maxime, opinion?
>
>
> --
> David Marchand
>
>

-- 
- Ibtisam

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

* Re: [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: enhance getopt_long usage
  2020-11-13 12:26   ` Ananyev, Konstantin
@ 2020-11-17  7:52     ` Ibtisam Tariq
  0 siblings, 0 replies; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-17  7:52 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: maxime.coquelin, Xia, Chenbo, Dumitrescu, Cristian, Singh,
	Jasvinder, Mcnamara, John, Pattan, Reshma, Kovacevic, Marko, dev

Hi Ananyev,

Thank you.
Mix enum with corresponding defines for string literals, is the
convention of eal_options.h.
You can have a look at this
https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/eal_options.h.


On Fri, Nov 13, 2020 at 5:27 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
>
> Hi,
>
> >
> > Instead of using getopt_long return value, strcmp was used to
> > compare the input parameters with the struct option array. This
> > patch get rid of all those strcmp by directly binding each longopt
> > with an int enum. This is to improve readability and consistency in
> > all examples.
> >
> > Bugzilla ID: 238
> > Cc: konstantin.ananyev@intel.com
> >
> > Reported-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
> > ---
> > v2
> > * Remove extra indentations.
> > * Remove extra block brackets in switch statement.
> > * Change enum names to start with OPT_ and remove KEYWORD from enum names.
> >
> > v1
> > * enhance getopt_long usage
> > ---
> >  examples/l3fwd-acl/main.c | 219 +++++++++++++++++++-------------------
> >  1 file changed, 110 insertions(+), 109 deletions(-)
> >
> > diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c
> > index 961594f5f..5725963ad 100644
> > --- a/examples/l3fwd-acl/main.c
> > +++ b/examples/l3fwd-acl/main.c
> > @@ -195,13 +195,24 @@ send_single_packet(struct rte_mbuf *m, uint16_t port);
> >  #define ACL_LEAD_CHAR                ('@')
> >  #define ROUTE_LEAD_CHAR              ('R')
> >  #define COMMENT_LEAD_CHAR    ('#')
> > -#define OPTION_CONFIG                "config"
> > -#define OPTION_NONUMA                "no-numa"
> > -#define OPTION_ENBJMO                "enable-jumbo"
> > -#define OPTION_RULE_IPV4     "rule_ipv4"
> > -#define OPTION_RULE_IPV6     "rule_ipv6"
> > -#define OPTION_ALG           "alg"
> > -#define OPTION_ETH_DEST              "eth-dest"
> > +
> > +enum {
> > +#define OPT_CONFIG           "config"
> > +     OPT_CONFIG_NUM = 256,
>
> It is probably better not to mix enum values with corresponding defines
> for string literals. Looks really weird and hard to read (at least to me).
> Apart from that - LGTM.
>
> > +#define OPT_NONUMA           "no-numa"
> > +     OPT_NONUMA_NUM,
> > +#define OPT_ENBJMO           "enable-jumbo"
> > +     OPT_ENBJMO_NUM,
> > +#define OPT_RULE_IPV4        "rule_ipv4"
> > +     OPT_RULE_IPV4_NUM,
> > +#define OPT_RULE_IPV6        "rule_ipv6"
> > +     OPT_RULE_IPV6_NUM,
> > +#define OPT_ALG                  "alg"
> > +     OPT_ALG_NUM,
> > +#define OPT_ETH_DEST    "eth-dest"
> > +     OPT_ETH_DEST_NUM,
> > +};
> > +
> >  #define ACL_DENY_SIGNATURE   0xf0000000
> >  #define RTE_LOGTYPE_L3FWDACL RTE_LOGTYPE_USER3
> >  #define acl_log(format, ...) RTE_LOG(ERR, L3FWDACL, format, ##__VA_ARGS__)
> > @@ -1177,9 +1188,9 @@ static void
> >  dump_acl_config(void)
> >  {
> >       printf("ACL option are:\n");
> > -     printf(OPTION_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
> > -     printf(OPTION_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
> > -     printf(OPTION_ALG": %s\n", str_acl_alg(parm_config.alg));
> > +     printf(OPT_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
> > +     printf(OPT_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
> > +     printf(OPT_ALG": %s\n", str_acl_alg(parm_config.alg));
> >  }
> >
> >  static int
> > @@ -1608,27 +1619,27 @@ print_usage(const char *prgname)
> >
> >       usage_acl_alg(alg, sizeof(alg));
> >       printf("%s [EAL options] -- -p PORTMASK -P"
> > -             "--"OPTION_RULE_IPV4"=FILE"
> > -             "--"OPTION_RULE_IPV6"=FILE"
> > -             "  [--"OPTION_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
> > -             "  [--"OPTION_ENBJMO" [--max-pkt-len PKTLEN]]\n"
> > +             "--"OPT_RULE_IPV4"=FILE"
> > +             "--"OPT_RULE_IPV6"=FILE"
> > +             "  [--"OPT_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
> > +             "  [--"OPT_ENBJMO" [--max-pkt-len PKTLEN]]\n"
> >               "  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
> >               "  -P : enable promiscuous mode\n"
> > -             "  --"OPTION_CONFIG": (port,queue,lcore): "
> > +             "  --"OPT_CONFIG": (port,queue,lcore): "
> >               "rx queues configuration\n"
> > -             "  --"OPTION_NONUMA": optional, disable numa awareness\n"
> > -             "  --"OPTION_ENBJMO": enable jumbo frame"
> > +             "  --"OPT_NONUMA": optional, disable numa awareness\n"
> > +             "  --"OPT_ENBJMO": enable jumbo frame"
> >               " which max packet len is PKTLEN in decimal (64-9600)\n"
> > -             "  --"OPTION_RULE_IPV4"=FILE: specify the ipv4 rules entries "
> > +             "  --"OPT_RULE_IPV4"=FILE: specify the ipv4 rules entries "
> >               "file. "
> >               "Each rule occupy one line. "
> >               "2 kinds of rules are supported. "
> >               "One is ACL entry at while line leads with character '%c', "
> >               "another is route entry at while line leads with "
> >               "character '%c'.\n"
> > -             "  --"OPTION_RULE_IPV6"=FILE: specify the ipv6 rules "
> > +             "  --"OPT_RULE_IPV6"=FILE: specify the ipv6 rules "
> >               "entries file.\n"
> > -             "  --"OPTION_ALG": ACL classify method to use, one of: %s\n",
> > +             "  --"OPT_ALG": ACL classify method to use, one of: %s\n",
> >               prgname, ACL_LEAD_CHAR, ROUTE_LEAD_CHAR, alg);
> >  }
> >
> > @@ -1747,14 +1758,14 @@ parse_args(int argc, char **argv)
> >       int option_index;
> >       char *prgname = argv[0];
> >       static struct option lgopts[] = {
> > -             {OPTION_CONFIG, 1, 0, 0},
> > -             {OPTION_NONUMA, 0, 0, 0},
> > -             {OPTION_ENBJMO, 0, 0, 0},
> > -             {OPTION_RULE_IPV4, 1, 0, 0},
> > -             {OPTION_RULE_IPV6, 1, 0, 0},
> > -             {OPTION_ALG, 1, 0, 0},
> > -             {OPTION_ETH_DEST, 1, 0, 0},
> > -             {NULL, 0, 0, 0}
> > +             {OPT_CONFIG,    1, NULL, OPT_CONFIG_NUM    },
> > +             {OPT_NONUMA,    0, NULL, OPT_NONUMA_NUM    },
> > +             {OPT_ENBJMO,    0, NULL, OPT_ENBJMO_NUM    },
> > +             {OPT_RULE_IPV4, 1, NULL, OPT_RULE_IPV4_NUM },
> > +             {OPT_RULE_IPV6, 1, NULL, OPT_RULE_IPV6_NUM },
> > +             {OPT_ALG,       1, NULL, OPT_ALG_NUM       },
> > +             {OPT_ETH_DEST,  1, NULL, OPT_ETH_DEST_NUM  },
> > +             {NULL,          0, 0,    0                 }
> >       };
> >
> >       argvopt = argv;
> > @@ -1772,104 +1783,94 @@ parse_args(int argc, char **argv)
> >                               return -1;
> >                       }
> >                       break;
> > +
> >               case 'P':
> >                       printf("Promiscuous mode selected\n");
> >                       promiscuous_on = 1;
> >                       break;
> >
> >               /* long options */
> > -             case 0:
> > -                     if (!strncmp(lgopts[option_index].name,
> > -                                     OPTION_CONFIG,
> > -                                     sizeof(OPTION_CONFIG))) {
> > -                             ret = parse_config(optarg);
> > -                             if (ret) {
> > -                                     printf("invalid config\n");
> > -                                     print_usage(prgname);
> > -                                     return -1;
> > -                             }
> > -                     }
> > -
> > -                     if (!strncmp(lgopts[option_index].name,
> > -                                     OPTION_NONUMA,
> > -                                     sizeof(OPTION_NONUMA))) {
> > -                             printf("numa is disabled\n");
> > -                             numa_on = 0;
> > -                     }
> > -
> > -                     if (!strncmp(lgopts[option_index].name,
> > -                                     OPTION_ENBJMO, sizeof(OPTION_ENBJMO))) {
> > -                             struct option lenopts = {
> > -                                     "max-pkt-len",
> > -                                     required_argument,
> > -                                     0,
> > -                                     0
> > -                             };
> > -
> > -                             printf("jumbo frame is enabled\n");
> > -                             port_conf.rxmode.offloads |=
> > -                                             DEV_RX_OFFLOAD_JUMBO_FRAME;
> > -                             port_conf.txmode.offloads |=
> > -                                             DEV_TX_OFFLOAD_MULTI_SEGS;
> > -
> > -                             /*
> > -                              * if no max-pkt-len set, then use the
> > -                              * default value RTE_ETHER_MAX_LEN
> > -                              */
> > -                             if (0 == getopt_long(argc, argvopt, "",
> > -                                             &lenopts, &option_index)) {
> > -                                     ret = parse_max_pkt_len(optarg);
> > -                                     if ((ret < 64) ||
> > -                                             (ret > MAX_JUMBO_PKT_LEN)) {
> > -                                             printf("invalid packet "
> > -                                                     "length\n");
> > -                                             print_usage(prgname);
> > -                                             return -1;
> > -                                     }
> > -                                     port_conf.rxmode.max_rx_pkt_len = ret;
> > -                             }
> > -                             printf("set jumbo frame max packet length "
> > -                                     "to %u\n",
> > -                                     (unsigned int)
> > -                                     port_conf.rxmode.max_rx_pkt_len);
> > +             case OPT_CONFIG_NUM:
> > +                     ret = parse_config(optarg);
> > +                     if (ret) {
> > +                             printf("invalid config\n");
> > +                             print_usage(prgname);
> > +                             return -1;
> >                       }
> > +                     break;
> >
> > -                     if (!strncmp(lgopts[option_index].name,
> > -                                     OPTION_RULE_IPV4,
> > -                                     sizeof(OPTION_RULE_IPV4)))
> > -                             parm_config.rule_ipv4_name = optarg;
> > -
> > -                     if (!strncmp(lgopts[option_index].name,
> > -                                     OPTION_RULE_IPV6,
> > -                                     sizeof(OPTION_RULE_IPV6))) {
> > -                             parm_config.rule_ipv6_name = optarg;
> > -                     }
> > +             case OPT_NONUMA_NUM:
> > +                     printf("numa is disabled\n");
> > +                     numa_on = 0;
> > +                     break;
> >
> > -                     if (!strncmp(lgopts[option_index].name,
> > -                                     OPTION_ALG, sizeof(OPTION_ALG))) {
> > -                             parm_config.alg = parse_acl_alg(optarg);
> > -                             if (parm_config.alg ==
> > -                                             RTE_ACL_CLASSIFY_DEFAULT) {
> > -                                     printf("unknown %s value:\"%s\"\n",
> > -                                             OPTION_ALG, optarg);
> > +             case OPT_ENBJMO_NUM:
> > +             {
> > +                     struct option lenopts = {
> > +                             "max-pkt-len",
> > +                             required_argument,
> > +                             0,
> > +                             0
> > +                     };
> > +
> > +                     printf("jumbo frame is enabled\n");
> > +                     port_conf.rxmode.offloads |=
> > +                                     DEV_RX_OFFLOAD_JUMBO_FRAME;
> > +                     port_conf.txmode.offloads |=
> > +                                     DEV_TX_OFFLOAD_MULTI_SEGS;
> > +
> > +                     /*
> > +                      * if no max-pkt-len set, then use the
> > +                      * default value RTE_ETHER_MAX_LEN
> > +                      */
> > +                     if (0 == getopt_long(argc, argvopt, "",
> > +                                     &lenopts, &option_index)) {
> > +                             ret = parse_max_pkt_len(optarg);
> > +                             if ((ret < 64) ||
> > +                                     (ret > MAX_JUMBO_PKT_LEN)) {
> > +                                     printf("invalid packet "
> > +                                             "length\n");
> >                                       print_usage(prgname);
> >                                       return -1;
> >                               }
> > +                             port_conf.rxmode.max_rx_pkt_len = ret;
> >                       }
> > +                     printf("set jumbo frame max packet length "
> > +                             "to %u\n",
> > +                             (unsigned int)
> > +                             port_conf.rxmode.max_rx_pkt_len);
> > +                     break;
> > +             }
> > +             case OPT_RULE_IPV4_NUM:
> > +                     parm_config.rule_ipv4_name = optarg;
> > +                     break;
> >
> > -                     if (!strncmp(lgopts[option_index].name, OPTION_ETH_DEST,
> > -                                     sizeof(OPTION_ETH_DEST))) {
> > -                             const char *serr = parse_eth_dest(optarg);
> > -                             if (serr != NULL) {
> > -                                     printf("invalid %s value:\"%s\": %s\n",
> > -                                             OPTION_ETH_DEST, optarg, serr);
> > -                                     print_usage(prgname);
> > -                                     return -1;
> > -                             }
> > -                     }
> > +             case OPT_RULE_IPV6_NUM:
> > +                     parm_config.rule_ipv6_name = optarg;
> > +                     break;
> >
> > +             case OPT_ALG_NUM:
> > +                     parm_config.alg = parse_acl_alg(optarg);
> > +                     if (parm_config.alg ==
> > +                                     RTE_ACL_CLASSIFY_DEFAULT) {
> > +                             printf("unknown %s value:\"%s\"\n",
> > +                                     OPT_ALG, optarg);
> > +                             print_usage(prgname);
> > +                             return -1;
> > +                     }
> >                       break;
> >
> > +             case OPT_ETH_DEST_NUM:
> > +             {
> > +                     const char *serr = parse_eth_dest(optarg);
> > +                     if (serr != NULL) {
> > +                             printf("invalid %s value:\"%s\": %s\n",
> > +                                     OPT_ETH_DEST, optarg, serr);
> > +                             print_usage(prgname);
> > +                             return -1;
> > +                     }
> > +                     break;
> > +             }
> >               default:
> >                       print_usage(prgname);
> >                       return -1;
> > --
> > 2.17.1
>


-- 
- Ibtisam

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

* Re: [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: enhance getopt_long usage
  2020-11-11  8:15 ` [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: " Ibtisam Tariq
@ 2020-11-13 12:26   ` Ananyev, Konstantin
  2020-11-17  7:52     ` Ibtisam Tariq
  0 siblings, 1 reply; 13+ messages in thread
From: Ananyev, Konstantin @ 2020-11-13 12:26 UTC (permalink / raw)
  To: Ibtisam Tariq, maxime.coquelin, Xia, Chenbo, Dumitrescu,
	Cristian, Singh, Jasvinder, Mcnamara, John, Pattan, Reshma,
	Kovacevic, Marko
  Cc: dev

Hi, 

> 
> Instead of using getopt_long return value, strcmp was used to
> compare the input parameters with the struct option array. This
> patch get rid of all those strcmp by directly binding each longopt
> with an int enum. This is to improve readability and consistency in
> all examples.
> 
> Bugzilla ID: 238
> Cc: konstantin.ananyev@intel.com
> 
> Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
> ---
> v2
> * Remove extra indentations.
> * Remove extra block brackets in switch statement.
> * Change enum names to start with OPT_ and remove KEYWORD from enum names.
> 
> v1
> * enhance getopt_long usage
> ---
>  examples/l3fwd-acl/main.c | 219 +++++++++++++++++++-------------------
>  1 file changed, 110 insertions(+), 109 deletions(-)
> 
> diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c
> index 961594f5f..5725963ad 100644
> --- a/examples/l3fwd-acl/main.c
> +++ b/examples/l3fwd-acl/main.c
> @@ -195,13 +195,24 @@ send_single_packet(struct rte_mbuf *m, uint16_t port);
>  #define ACL_LEAD_CHAR		('@')
>  #define ROUTE_LEAD_CHAR		('R')
>  #define COMMENT_LEAD_CHAR	('#')
> -#define OPTION_CONFIG		"config"
> -#define OPTION_NONUMA		"no-numa"
> -#define OPTION_ENBJMO		"enable-jumbo"
> -#define OPTION_RULE_IPV4	"rule_ipv4"
> -#define OPTION_RULE_IPV6	"rule_ipv6"
> -#define OPTION_ALG		"alg"
> -#define OPTION_ETH_DEST		"eth-dest"
> +
> +enum {
> +#define OPT_CONFIG		"config"
> +	OPT_CONFIG_NUM = 256,

It is probably better not to mix enum values with corresponding defines
for string literals. Looks really weird and hard to read (at least to me).
Apart from that - LGTM.

> +#define OPT_NONUMA		"no-numa"
> +	OPT_NONUMA_NUM,
> +#define OPT_ENBJMO		"enable-jumbo"
> +	OPT_ENBJMO_NUM,
> +#define OPT_RULE_IPV4	"rule_ipv4"
> +	OPT_RULE_IPV4_NUM,
> +#define OPT_RULE_IPV6	"rule_ipv6"
> +	OPT_RULE_IPV6_NUM,
> +#define OPT_ALG		    "alg"
> +	OPT_ALG_NUM,
> +#define OPT_ETH_DEST    "eth-dest"
> +	OPT_ETH_DEST_NUM,
> +};
> +
>  #define ACL_DENY_SIGNATURE	0xf0000000
>  #define RTE_LOGTYPE_L3FWDACL	RTE_LOGTYPE_USER3
>  #define acl_log(format, ...)	RTE_LOG(ERR, L3FWDACL, format, ##__VA_ARGS__)
> @@ -1177,9 +1188,9 @@ static void
>  dump_acl_config(void)
>  {
>  	printf("ACL option are:\n");
> -	printf(OPTION_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
> -	printf(OPTION_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
> -	printf(OPTION_ALG": %s\n", str_acl_alg(parm_config.alg));
> +	printf(OPT_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
> +	printf(OPT_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
> +	printf(OPT_ALG": %s\n", str_acl_alg(parm_config.alg));
>  }
> 
>  static int
> @@ -1608,27 +1619,27 @@ print_usage(const char *prgname)
> 
>  	usage_acl_alg(alg, sizeof(alg));
>  	printf("%s [EAL options] -- -p PORTMASK -P"
> -		"--"OPTION_RULE_IPV4"=FILE"
> -		"--"OPTION_RULE_IPV6"=FILE"
> -		"  [--"OPTION_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
> -		"  [--"OPTION_ENBJMO" [--max-pkt-len PKTLEN]]\n"
> +		"--"OPT_RULE_IPV4"=FILE"
> +		"--"OPT_RULE_IPV6"=FILE"
> +		"  [--"OPT_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
> +		"  [--"OPT_ENBJMO" [--max-pkt-len PKTLEN]]\n"
>  		"  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
>  		"  -P : enable promiscuous mode\n"
> -		"  --"OPTION_CONFIG": (port,queue,lcore): "
> +		"  --"OPT_CONFIG": (port,queue,lcore): "
>  		"rx queues configuration\n"
> -		"  --"OPTION_NONUMA": optional, disable numa awareness\n"
> -		"  --"OPTION_ENBJMO": enable jumbo frame"
> +		"  --"OPT_NONUMA": optional, disable numa awareness\n"
> +		"  --"OPT_ENBJMO": enable jumbo frame"
>  		" which max packet len is PKTLEN in decimal (64-9600)\n"
> -		"  --"OPTION_RULE_IPV4"=FILE: specify the ipv4 rules entries "
> +		"  --"OPT_RULE_IPV4"=FILE: specify the ipv4 rules entries "
>  		"file. "
>  		"Each rule occupy one line. "
>  		"2 kinds of rules are supported. "
>  		"One is ACL entry at while line leads with character '%c', "
>  		"another is route entry at while line leads with "
>  		"character '%c'.\n"
> -		"  --"OPTION_RULE_IPV6"=FILE: specify the ipv6 rules "
> +		"  --"OPT_RULE_IPV6"=FILE: specify the ipv6 rules "
>  		"entries file.\n"
> -		"  --"OPTION_ALG": ACL classify method to use, one of: %s\n",
> +		"  --"OPT_ALG": ACL classify method to use, one of: %s\n",
>  		prgname, ACL_LEAD_CHAR, ROUTE_LEAD_CHAR, alg);
>  }
> 
> @@ -1747,14 +1758,14 @@ parse_args(int argc, char **argv)
>  	int option_index;
>  	char *prgname = argv[0];
>  	static struct option lgopts[] = {
> -		{OPTION_CONFIG, 1, 0, 0},
> -		{OPTION_NONUMA, 0, 0, 0},
> -		{OPTION_ENBJMO, 0, 0, 0},
> -		{OPTION_RULE_IPV4, 1, 0, 0},
> -		{OPTION_RULE_IPV6, 1, 0, 0},
> -		{OPTION_ALG, 1, 0, 0},
> -		{OPTION_ETH_DEST, 1, 0, 0},
> -		{NULL, 0, 0, 0}
> +		{OPT_CONFIG,    1, NULL, OPT_CONFIG_NUM    },
> +		{OPT_NONUMA,    0, NULL, OPT_NONUMA_NUM    },
> +		{OPT_ENBJMO,    0, NULL, OPT_ENBJMO_NUM    },
> +		{OPT_RULE_IPV4, 1, NULL, OPT_RULE_IPV4_NUM },
> +		{OPT_RULE_IPV6, 1, NULL, OPT_RULE_IPV6_NUM },
> +		{OPT_ALG,       1, NULL, OPT_ALG_NUM       },
> +		{OPT_ETH_DEST,  1, NULL, OPT_ETH_DEST_NUM  },
> +		{NULL,          0, 0,    0                 }
>  	};
> 
>  	argvopt = argv;
> @@ -1772,104 +1783,94 @@ parse_args(int argc, char **argv)
>  				return -1;
>  			}
>  			break;
> +
>  		case 'P':
>  			printf("Promiscuous mode selected\n");
>  			promiscuous_on = 1;
>  			break;
> 
>  		/* long options */
> -		case 0:
> -			if (!strncmp(lgopts[option_index].name,
> -					OPTION_CONFIG,
> -					sizeof(OPTION_CONFIG))) {
> -				ret = parse_config(optarg);
> -				if (ret) {
> -					printf("invalid config\n");
> -					print_usage(prgname);
> -					return -1;
> -				}
> -			}
> -
> -			if (!strncmp(lgopts[option_index].name,
> -					OPTION_NONUMA,
> -					sizeof(OPTION_NONUMA))) {
> -				printf("numa is disabled\n");
> -				numa_on = 0;
> -			}
> -
> -			if (!strncmp(lgopts[option_index].name,
> -					OPTION_ENBJMO, sizeof(OPTION_ENBJMO))) {
> -				struct option lenopts = {
> -					"max-pkt-len",
> -					required_argument,
> -					0,
> -					0
> -				};
> -
> -				printf("jumbo frame is enabled\n");
> -				port_conf.rxmode.offloads |=
> -						DEV_RX_OFFLOAD_JUMBO_FRAME;
> -				port_conf.txmode.offloads |=
> -						DEV_TX_OFFLOAD_MULTI_SEGS;
> -
> -				/*
> -				 * if no max-pkt-len set, then use the
> -				 * default value RTE_ETHER_MAX_LEN
> -				 */
> -				if (0 == getopt_long(argc, argvopt, "",
> -						&lenopts, &option_index)) {
> -					ret = parse_max_pkt_len(optarg);
> -					if ((ret < 64) ||
> -						(ret > MAX_JUMBO_PKT_LEN)) {
> -						printf("invalid packet "
> -							"length\n");
> -						print_usage(prgname);
> -						return -1;
> -					}
> -					port_conf.rxmode.max_rx_pkt_len = ret;
> -				}
> -				printf("set jumbo frame max packet length "
> -					"to %u\n",
> -					(unsigned int)
> -					port_conf.rxmode.max_rx_pkt_len);
> +		case OPT_CONFIG_NUM:
> +			ret = parse_config(optarg);
> +			if (ret) {
> +				printf("invalid config\n");
> +				print_usage(prgname);
> +				return -1;
>  			}
> +			break;
> 
> -			if (!strncmp(lgopts[option_index].name,
> -					OPTION_RULE_IPV4,
> -					sizeof(OPTION_RULE_IPV4)))
> -				parm_config.rule_ipv4_name = optarg;
> -
> -			if (!strncmp(lgopts[option_index].name,
> -					OPTION_RULE_IPV6,
> -					sizeof(OPTION_RULE_IPV6))) {
> -				parm_config.rule_ipv6_name = optarg;
> -			}
> +		case OPT_NONUMA_NUM:
> +			printf("numa is disabled\n");
> +			numa_on = 0;
> +			break;
> 
> -			if (!strncmp(lgopts[option_index].name,
> -					OPTION_ALG, sizeof(OPTION_ALG))) {
> -				parm_config.alg = parse_acl_alg(optarg);
> -				if (parm_config.alg ==
> -						RTE_ACL_CLASSIFY_DEFAULT) {
> -					printf("unknown %s value:\"%s\"\n",
> -						OPTION_ALG, optarg);
> +		case OPT_ENBJMO_NUM:
> +		{
> +			struct option lenopts = {
> +				"max-pkt-len",
> +				required_argument,
> +				0,
> +				0
> +			};
> +
> +			printf("jumbo frame is enabled\n");
> +			port_conf.rxmode.offloads |=
> +					DEV_RX_OFFLOAD_JUMBO_FRAME;
> +			port_conf.txmode.offloads |=
> +					DEV_TX_OFFLOAD_MULTI_SEGS;
> +
> +			/*
> +			 * if no max-pkt-len set, then use the
> +			 * default value RTE_ETHER_MAX_LEN
> +			 */
> +			if (0 == getopt_long(argc, argvopt, "",
> +					&lenopts, &option_index)) {
> +				ret = parse_max_pkt_len(optarg);
> +				if ((ret < 64) ||
> +					(ret > MAX_JUMBO_PKT_LEN)) {
> +					printf("invalid packet "
> +						"length\n");
>  					print_usage(prgname);
>  					return -1;
>  				}
> +				port_conf.rxmode.max_rx_pkt_len = ret;
>  			}
> +			printf("set jumbo frame max packet length "
> +				"to %u\n",
> +				(unsigned int)
> +				port_conf.rxmode.max_rx_pkt_len);
> +			break;
> +		}
> +		case OPT_RULE_IPV4_NUM:
> +			parm_config.rule_ipv4_name = optarg;
> +			break;
> 
> -			if (!strncmp(lgopts[option_index].name, OPTION_ETH_DEST,
> -					sizeof(OPTION_ETH_DEST))) {
> -				const char *serr = parse_eth_dest(optarg);
> -				if (serr != NULL) {
> -					printf("invalid %s value:\"%s\": %s\n",
> -						OPTION_ETH_DEST, optarg, serr);
> -					print_usage(prgname);
> -					return -1;
> -				}
> -			}
> +		case OPT_RULE_IPV6_NUM:
> +			parm_config.rule_ipv6_name = optarg;
> +			break;
> 
> +		case OPT_ALG_NUM:
> +			parm_config.alg = parse_acl_alg(optarg);
> +			if (parm_config.alg ==
> +					RTE_ACL_CLASSIFY_DEFAULT) {
> +				printf("unknown %s value:\"%s\"\n",
> +					OPT_ALG, optarg);
> +				print_usage(prgname);
> +				return -1;
> +			}
>  			break;
> 
> +		case OPT_ETH_DEST_NUM:
> +		{
> +			const char *serr = parse_eth_dest(optarg);
> +			if (serr != NULL) {
> +				printf("invalid %s value:\"%s\": %s\n",
> +					OPT_ETH_DEST, optarg, serr);
> +				print_usage(prgname);
> +				return -1;
> +			}
> +			break;
> +		}
>  		default:
>  			print_usage(prgname);
>  			return -1;
> --
> 2.17.1


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

* [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: enhance getopt_long usage
  2020-11-11  8:15 Ibtisam Tariq
@ 2020-11-11  8:15 ` Ibtisam Tariq
  2020-11-13 12:26   ` Ananyev, Konstantin
  0 siblings, 1 reply; 13+ messages in thread
From: Ibtisam Tariq @ 2020-11-11  8:15 UTC (permalink / raw)
  To: maxime.coquelin, chenbo.xia, cristian.dumitrescu,
	jasvinder.singh, john.mcnamara, reshma.pattan,
	konstantin.ananyev, marko.kovacevic
  Cc: dev, Ibtisam Tariq

Instead of using getopt_long return value, strcmp was used to
compare the input parameters with the struct option array. This
patch get rid of all those strcmp by directly binding each longopt
with an int enum. This is to improve readability and consistency in
all examples.

Bugzilla ID: 238
Cc: konstantin.ananyev@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
v2
* Remove extra indentations.
* Remove extra block brackets in switch statement.
* Change enum names to start with OPT_ and remove KEYWORD from enum names.

v1
* enhance getopt_long usage
---
 examples/l3fwd-acl/main.c | 219 +++++++++++++++++++-------------------
 1 file changed, 110 insertions(+), 109 deletions(-)

diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c
index 961594f5f..5725963ad 100644
--- a/examples/l3fwd-acl/main.c
+++ b/examples/l3fwd-acl/main.c
@@ -195,13 +195,24 @@ send_single_packet(struct rte_mbuf *m, uint16_t port);
 #define ACL_LEAD_CHAR		('@')
 #define ROUTE_LEAD_CHAR		('R')
 #define COMMENT_LEAD_CHAR	('#')
-#define OPTION_CONFIG		"config"
-#define OPTION_NONUMA		"no-numa"
-#define OPTION_ENBJMO		"enable-jumbo"
-#define OPTION_RULE_IPV4	"rule_ipv4"
-#define OPTION_RULE_IPV6	"rule_ipv6"
-#define OPTION_ALG		"alg"
-#define OPTION_ETH_DEST		"eth-dest"
+
+enum {
+#define OPT_CONFIG		"config"
+	OPT_CONFIG_NUM = 256,
+#define OPT_NONUMA		"no-numa"
+	OPT_NONUMA_NUM,
+#define OPT_ENBJMO		"enable-jumbo"
+	OPT_ENBJMO_NUM,
+#define OPT_RULE_IPV4	"rule_ipv4"
+	OPT_RULE_IPV4_NUM,
+#define OPT_RULE_IPV6	"rule_ipv6"
+	OPT_RULE_IPV6_NUM,
+#define OPT_ALG		    "alg"
+	OPT_ALG_NUM,
+#define OPT_ETH_DEST    "eth-dest"
+	OPT_ETH_DEST_NUM,
+};
+
 #define ACL_DENY_SIGNATURE	0xf0000000
 #define RTE_LOGTYPE_L3FWDACL	RTE_LOGTYPE_USER3
 #define acl_log(format, ...)	RTE_LOG(ERR, L3FWDACL, format, ##__VA_ARGS__)
@@ -1177,9 +1188,9 @@ static void
 dump_acl_config(void)
 {
 	printf("ACL option are:\n");
-	printf(OPTION_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
-	printf(OPTION_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
-	printf(OPTION_ALG": %s\n", str_acl_alg(parm_config.alg));
+	printf(OPT_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
+	printf(OPT_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
+	printf(OPT_ALG": %s\n", str_acl_alg(parm_config.alg));
 }
 
 static int
@@ -1608,27 +1619,27 @@ print_usage(const char *prgname)
 
 	usage_acl_alg(alg, sizeof(alg));
 	printf("%s [EAL options] -- -p PORTMASK -P"
-		"--"OPTION_RULE_IPV4"=FILE"
-		"--"OPTION_RULE_IPV6"=FILE"
-		"  [--"OPTION_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
-		"  [--"OPTION_ENBJMO" [--max-pkt-len PKTLEN]]\n"
+		"--"OPT_RULE_IPV4"=FILE"
+		"--"OPT_RULE_IPV6"=FILE"
+		"  [--"OPT_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
+		"  [--"OPT_ENBJMO" [--max-pkt-len PKTLEN]]\n"
 		"  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
 		"  -P : enable promiscuous mode\n"
-		"  --"OPTION_CONFIG": (port,queue,lcore): "
+		"  --"OPT_CONFIG": (port,queue,lcore): "
 		"rx queues configuration\n"
-		"  --"OPTION_NONUMA": optional, disable numa awareness\n"
-		"  --"OPTION_ENBJMO": enable jumbo frame"
+		"  --"OPT_NONUMA": optional, disable numa awareness\n"
+		"  --"OPT_ENBJMO": enable jumbo frame"
 		" which max packet len is PKTLEN in decimal (64-9600)\n"
-		"  --"OPTION_RULE_IPV4"=FILE: specify the ipv4 rules entries "
+		"  --"OPT_RULE_IPV4"=FILE: specify the ipv4 rules entries "
 		"file. "
 		"Each rule occupy one line. "
 		"2 kinds of rules are supported. "
 		"One is ACL entry at while line leads with character '%c', "
 		"another is route entry at while line leads with "
 		"character '%c'.\n"
-		"  --"OPTION_RULE_IPV6"=FILE: specify the ipv6 rules "
+		"  --"OPT_RULE_IPV6"=FILE: specify the ipv6 rules "
 		"entries file.\n"
-		"  --"OPTION_ALG": ACL classify method to use, one of: %s\n",
+		"  --"OPT_ALG": ACL classify method to use, one of: %s\n",
 		prgname, ACL_LEAD_CHAR, ROUTE_LEAD_CHAR, alg);
 }
 
@@ -1747,14 +1758,14 @@ parse_args(int argc, char **argv)
 	int option_index;
 	char *prgname = argv[0];
 	static struct option lgopts[] = {
-		{OPTION_CONFIG, 1, 0, 0},
-		{OPTION_NONUMA, 0, 0, 0},
-		{OPTION_ENBJMO, 0, 0, 0},
-		{OPTION_RULE_IPV4, 1, 0, 0},
-		{OPTION_RULE_IPV6, 1, 0, 0},
-		{OPTION_ALG, 1, 0, 0},
-		{OPTION_ETH_DEST, 1, 0, 0},
-		{NULL, 0, 0, 0}
+		{OPT_CONFIG,    1, NULL, OPT_CONFIG_NUM    },
+		{OPT_NONUMA,    0, NULL, OPT_NONUMA_NUM    },
+		{OPT_ENBJMO,    0, NULL, OPT_ENBJMO_NUM    },
+		{OPT_RULE_IPV4, 1, NULL, OPT_RULE_IPV4_NUM },
+		{OPT_RULE_IPV6, 1, NULL, OPT_RULE_IPV6_NUM },
+		{OPT_ALG,       1, NULL, OPT_ALG_NUM       },
+		{OPT_ETH_DEST,  1, NULL, OPT_ETH_DEST_NUM  },
+		{NULL,          0, 0,    0                 }
 	};
 
 	argvopt = argv;
@@ -1772,104 +1783,94 @@ parse_args(int argc, char **argv)
 				return -1;
 			}
 			break;
+
 		case 'P':
 			printf("Promiscuous mode selected\n");
 			promiscuous_on = 1;
 			break;
 
 		/* long options */
-		case 0:
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_CONFIG,
-					sizeof(OPTION_CONFIG))) {
-				ret = parse_config(optarg);
-				if (ret) {
-					printf("invalid config\n");
-					print_usage(prgname);
-					return -1;
-				}
-			}
-
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_NONUMA,
-					sizeof(OPTION_NONUMA))) {
-				printf("numa is disabled\n");
-				numa_on = 0;
-			}
-
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_ENBJMO, sizeof(OPTION_ENBJMO))) {
-				struct option lenopts = {
-					"max-pkt-len",
-					required_argument,
-					0,
-					0
-				};
-
-				printf("jumbo frame is enabled\n");
-				port_conf.rxmode.offloads |=
-						DEV_RX_OFFLOAD_JUMBO_FRAME;
-				port_conf.txmode.offloads |=
-						DEV_TX_OFFLOAD_MULTI_SEGS;
-
-				/*
-				 * if no max-pkt-len set, then use the
-				 * default value RTE_ETHER_MAX_LEN
-				 */
-				if (0 == getopt_long(argc, argvopt, "",
-						&lenopts, &option_index)) {
-					ret = parse_max_pkt_len(optarg);
-					if ((ret < 64) ||
-						(ret > MAX_JUMBO_PKT_LEN)) {
-						printf("invalid packet "
-							"length\n");
-						print_usage(prgname);
-						return -1;
-					}
-					port_conf.rxmode.max_rx_pkt_len = ret;
-				}
-				printf("set jumbo frame max packet length "
-					"to %u\n",
-					(unsigned int)
-					port_conf.rxmode.max_rx_pkt_len);
+		case OPT_CONFIG_NUM:
+			ret = parse_config(optarg);
+			if (ret) {
+				printf("invalid config\n");
+				print_usage(prgname);
+				return -1;
 			}
+			break;
 
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_RULE_IPV4,
-					sizeof(OPTION_RULE_IPV4)))
-				parm_config.rule_ipv4_name = optarg;
-
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_RULE_IPV6,
-					sizeof(OPTION_RULE_IPV6))) {
-				parm_config.rule_ipv6_name = optarg;
-			}
+		case OPT_NONUMA_NUM:
+			printf("numa is disabled\n");
+			numa_on = 0;
+			break;
 
-			if (!strncmp(lgopts[option_index].name,
-					OPTION_ALG, sizeof(OPTION_ALG))) {
-				parm_config.alg = parse_acl_alg(optarg);
-				if (parm_config.alg ==
-						RTE_ACL_CLASSIFY_DEFAULT) {
-					printf("unknown %s value:\"%s\"\n",
-						OPTION_ALG, optarg);
+		case OPT_ENBJMO_NUM:
+		{
+			struct option lenopts = {
+				"max-pkt-len",
+				required_argument,
+				0,
+				0
+			};
+
+			printf("jumbo frame is enabled\n");
+			port_conf.rxmode.offloads |=
+					DEV_RX_OFFLOAD_JUMBO_FRAME;
+			port_conf.txmode.offloads |=
+					DEV_TX_OFFLOAD_MULTI_SEGS;
+
+			/*
+			 * if no max-pkt-len set, then use the
+			 * default value RTE_ETHER_MAX_LEN
+			 */
+			if (0 == getopt_long(argc, argvopt, "",
+					&lenopts, &option_index)) {
+				ret = parse_max_pkt_len(optarg);
+				if ((ret < 64) ||
+					(ret > MAX_JUMBO_PKT_LEN)) {
+					printf("invalid packet "
+						"length\n");
 					print_usage(prgname);
 					return -1;
 				}
+				port_conf.rxmode.max_rx_pkt_len = ret;
 			}
+			printf("set jumbo frame max packet length "
+				"to %u\n",
+				(unsigned int)
+				port_conf.rxmode.max_rx_pkt_len);
+			break;
+		}
+		case OPT_RULE_IPV4_NUM:
+			parm_config.rule_ipv4_name = optarg;
+			break;
 
-			if (!strncmp(lgopts[option_index].name, OPTION_ETH_DEST,
-					sizeof(OPTION_ETH_DEST))) {
-				const char *serr = parse_eth_dest(optarg);
-				if (serr != NULL) {
-					printf("invalid %s value:\"%s\": %s\n",
-						OPTION_ETH_DEST, optarg, serr);
-					print_usage(prgname);
-					return -1;
-				}
-			}
+		case OPT_RULE_IPV6_NUM:
+			parm_config.rule_ipv6_name = optarg;
+			break;
 
+		case OPT_ALG_NUM:
+			parm_config.alg = parse_acl_alg(optarg);
+			if (parm_config.alg ==
+					RTE_ACL_CLASSIFY_DEFAULT) {
+				printf("unknown %s value:\"%s\"\n",
+					OPT_ALG, optarg);
+				print_usage(prgname);
+				return -1;
+			}
 			break;
 
+		case OPT_ETH_DEST_NUM:
+		{
+			const char *serr = parse_eth_dest(optarg);
+			if (serr != NULL) {
+				printf("invalid %s value:\"%s\": %s\n",
+					OPT_ETH_DEST, optarg, serr);
+				print_usage(prgname);
+				return -1;
+			}
+			break;
+		}
 		default:
 			print_usage(prgname);
 			return -1;
-- 
2.17.1


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

end of thread, other threads:[~2020-12-31  7:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
2020-11-24 12:32 ` [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: " Ibtisam Tariq
2020-11-24 12:32 ` [dpdk-dev] [PATCH 3/7] examples/packet_ordering: " Ibtisam Tariq
2020-11-24 12:32 ` [dpdk-dev] [PATCH 4/7] examples/performance-thread/l3fwd-thread: " Ibtisam Tariq
2020-11-24 12:32 ` [dpdk-dev] [PATCH 5/7] examples/qos_sched: " Ibtisam Tariq
2020-11-24 12:32 ` [dpdk-dev] [PATCH 6/7] examples/vhost: " Ibtisam Tariq
2020-12-07 10:51   ` David Marchand
2020-12-31  7:00     ` Ibtisam Tariq
2020-11-24 12:32 ` [dpdk-dev] [PATCH 7/7] examples/vhost_crypto: " Ibtisam Tariq
2020-12-07 10:50 ` [dpdk-dev] [PATCH 1/7] examples/fips_validation: " David Marchand
  -- strict thread matches above, loose matches on Subject: below --
2020-11-11  8:15 Ibtisam Tariq
2020-11-11  8:15 ` [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: " Ibtisam Tariq
2020-11-13 12:26   ` Ananyev, Konstantin
2020-11-17  7:52     ` Ibtisam Tariq

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