DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage
@ 2020-10-29 12:53 Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 2/8] examples/l3fwd-acl: " Ibtisam Tariq
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-10-29 12:53 UTC (permalink / raw)
  To: marko.kovacevic, konstantin.ananyev, reshma.pattan,
	john.mcnamara, cristian.dumitrescu, jasvinder.singh, chenbo.xia,
	maxime.coquelin, xiaoyun.li
  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.

Bugzilla ID: 238
Fixes: 3d0fad56b74 ("examples/fips_validation: add crypto FIPS application"}
Cc: marko.kovacevic@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
 examples/fips_validation/main.c | 241 +++++++++++++++++++-------------
 1 file changed, 143 insertions(+), 98 deletions(-)

diff --git a/examples/fips_validation/main.c b/examples/fips_validation/main.c
index 07532c956..5fb76b421 100644
--- a/examples/fips_validation/main.c
+++ b/examples/fips_validation/main.c
@@ -15,17 +15,31 @@
 #include "fips_validation.h"
 #include "fips_dev_self_test.h"
 
+enum{
 #define REQ_FILE_PATH_KEYWORD	"req-file"
+	/* first long only option value must be >= 256, so that we won't
+	 * conflict with short options
+	 */
+	REQ_FILE_PATH_KEYWORD_NUM = 256,
 #define RSP_FILE_PATH_KEYWORD	"rsp-file"
+	RSP_FILE_PATH_KEYWORD_NUM,
 #define MBUF_DATAROOM_KEYWORD	"mbuf-dataroom"
+	MBUF_DATAROOM_KEYWORD_NUM,
 #define FOLDER_KEYWORD		"path-is-folder"
+	FOLDER_KEYWORD_NUM,
 #define CRYPTODEV_KEYWORD	"cryptodev"
+	CRYPTODEV_KEYWORD_NUM,
 #define CRYPTODEV_ID_KEYWORD	"cryptodev-id"
+	CRYPTODEV_ID_KEYWORD_NUM,
 #define CRYPTODEV_ST_KEYWORD	"self-test"
+	CRYPTODEV_ST_KEYWORD_NUM,
 #define CRYPTODEV_BK_ID_KEYWORD	"broken-test-id"
+	CRYPTODEV_BK_ID_KEYWORD_NUM,
 #define CRYPTODEV_BK_DIR_KEY	"broken-test-dir"
+	CRYPTODEV_BK_DIR_KEY_NUM,
 #define CRYPTODEV_ENC_KEYWORD	"enc"
 #define CRYPTODEV_DEC_KEYWORD	"dec"
+};
 
 struct fips_test_vector vec;
 struct fips_test_interim_info info;
@@ -226,15 +240,24 @@ 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},
+			{REQ_FILE_PATH_KEYWORD, required_argument,
+					NULL, REQ_FILE_PATH_KEYWORD_NUM},
+			{RSP_FILE_PATH_KEYWORD, required_argument,
+					NULL, RSP_FILE_PATH_KEYWORD_NUM},
+			{FOLDER_KEYWORD, no_argument,
+					NULL, FOLDER_KEYWORD_NUM},
+			{MBUF_DATAROOM_KEYWORD, required_argument,
+					NULL, MBUF_DATAROOM_KEYWORD_NUM},
+			{CRYPTODEV_KEYWORD, required_argument,
+					NULL, CRYPTODEV_KEYWORD_NUM},
+			{CRYPTODEV_ID_KEYWORD, required_argument,
+					NULL, CRYPTODEV_ID_KEYWORD_NUM},
+			{CRYPTODEV_ST_KEYWORD, no_argument,
+					NULL, CRYPTODEV_ST_KEYWORD_NUM},
+			{CRYPTODEV_BK_ID_KEYWORD, required_argument,
+					NULL, CRYPTODEV_BK_ID_KEYWORD_NUM},
+			{CRYPTODEV_BK_DIR_KEY, required_argument,
+					NULL, CRYPTODEV_BK_DIR_KEY_NUM},
 			{NULL, 0, 0, 0}
 	};
 
@@ -251,105 +274,127 @@ cryptodev_fips_validate_parse_args(int argc, char **argv)
 	while ((opt = getopt_long(argc, argvopt, "s:",
 				  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 REQ_FILE_PATH_KEYWORD_NUM:
+		{
+			env.req_path = optarg;
+			break;
+		}
+		case RSP_FILE_PATH_KEYWORD_NUM:
+		{
+			env.rsp_path = optarg;
+			break;
+		}
+		case FOLDER_KEYWORD_NUM:
+		{
+			env.is_path_folder = 1;
+			break;
+		}
+		case CRYPTODEV_KEYWORD_NUM:
+		{
+			ret = parse_cryptodev_arg(optarg);
+			if (ret < 0) {
+				cryptodev_fips_validate_usage(prgname);
+				return -EINVAL;
+			}
 
-				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;
-				}
+			break;
+		}
+		case CRYPTODEV_ID_KEYWORD_NUM:
+		{
+			ret = parse_cryptodev_id_arg(optarg);
+			if (ret < 0) {
+				cryptodev_fips_validate_usage(prgname);
+				return -EINVAL;
+			}
+			break;
+		}
+		case CRYPTODEV_ST_KEYWORD_NUM:
+		{
+			env.self_test = 1;
+			break;
+		}
+		case CRYPTODEV_BK_ID_KEYWORD_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 (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;
-				}
+			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 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 (data_room_size == 0 ||
-						data_room_size > UINT16_MAX) {
-					cryptodev_fips_validate_usage(prgname);
-					return -EINVAL;
-				}
+			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;
+			}
+			break;
+		}
+		case MBUF_DATAROOM_KEYWORD_NUM:
+		{
+			uint32_t data_room_size;
 
-				env.mbuf_data_room = data_room_size;
-			} else {
+			if (parser_read_uint32(&data_room_size,
+					optarg) < 0) {
 				cryptodev_fips_validate_usage(prgname);
 				return -EINVAL;
 			}
+
+			if (data_room_size == 0 ||
+					data_room_size > UINT16_MAX) {
+				cryptodev_fips_validate_usage(prgname);
+				return -EINVAL;
+			}
+
+			env.mbuf_data_room = data_room_size;
+
 			break;
+		}
 		default:
-			return -1;
+		{
+			cryptodev_fips_validate_usage(prgname);
+			return -EINVAL;
+		}
 		}
 	}
 
-- 
2.17.1


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

* [dpdk-dev] [PATCH 2/8] examples/l3fwd-acl: enhance getopt_long usage
  2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
@ 2020-10-29 12:53 ` Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 3/8] examples/packet_ordering: " Ibtisam Tariq
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-10-29 12:53 UTC (permalink / raw)
  To: marko.kovacevic, konstantin.ananyev, reshma.pattan,
	john.mcnamara, cristian.dumitrescu, jasvinder.singh, chenbo.xia,
	maxime.coquelin, xiaoyun.li
  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.

Bugzilla ID: 238
Fixes: 361b2e9559 ("acl: new sample l3fwd-acl")
Cc: konstantin.ananyev@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
 examples/l3fwd-acl/main.c | 189 +++++++++++++++++++-------------------
 1 file changed, 97 insertions(+), 92 deletions(-)

diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c
index 961594f5f..5c8de1fc0 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	('#')
+
+enum{
 #define OPTION_CONFIG		"config"
+	OPTION_CONFIG_NUM = 256,
 #define OPTION_NONUMA		"no-numa"
+	OPTION_NONUMA_NUM,
 #define OPTION_ENBJMO		"enable-jumbo"
+	OPTION_ENBJMO_NUM,
 #define OPTION_RULE_IPV4	"rule_ipv4"
+	OPTION_RULE_IPV4_NUM,
 #define OPTION_RULE_IPV6	"rule_ipv6"
+	OPTION_RULE_IPV6_NUM,
 #define OPTION_ALG		"alg"
+	OPTION_ALG_NUM,
 #define OPTION_ETH_DEST		"eth-dest"
+	OPTION_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__)
@@ -1747,13 +1758,13 @@ 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},
+		{OPTION_CONFIG, 1, NULL, OPTION_CONFIG_NUM},
+		{OPTION_NONUMA, 0, NULL, OPTION_NONUMA_NUM},
+		{OPTION_ENBJMO, 0, NULL, OPTION_ENBJMO_NUM},
+		{OPTION_RULE_IPV4, 1, NULL, OPTION_RULE_IPV4_NUM},
+		{OPTION_RULE_IPV6, 1, NULL, OPTION_RULE_IPV6_NUM},
+		{OPTION_ALG, 1, NULL, OPTION_ALG_NUM},
+		{OPTION_ETH_DEST, 1, NULL, OPTION_ETH_DEST_NUM},
 		{NULL, 0, 0, 0}
 	};
 
@@ -1778,98 +1789,92 @@ parse_args(int argc, char **argv)
 			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);
-			}
-
-			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 OPTION_CONFIG_NUM:
+		{
+			ret = parse_config(optarg);
+			if (ret) {
+				printf("invalid config\n");
+				print_usage(prgname);
+				return -1;
 			}
-
-			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);
+			break;
+		}
+		case OPTION_NONUMA_NUM:
+		{
+			printf("numa is disabled\n");
+			numa_on = 0;
+			break;
+		}
+		case OPTION_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;
 			}
-
-			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;
-				}
+			printf("set jumbo frame max packet length "
+				"to %u\n",
+				(unsigned int)
+				port_conf.rxmode.max_rx_pkt_len);
+			break;
+		}
+		case OPTION_RULE_IPV4_NUM:
+		{
+			parm_config.rule_ipv4_name = optarg;
+			break;
+		}
+		case OPTION_RULE_IPV6_NUM:
+		{
+			parm_config.rule_ipv6_name = optarg;
+			break;
+		}
+		case OPTION_ALG_NUM:
+		{
+			parm_config.alg = parse_acl_alg(optarg);
+			if (parm_config.alg ==
+					RTE_ACL_CLASSIFY_DEFAULT) {
+				printf("unknown %s value:\"%s\"\n",
+					OPTION_ALG, optarg);
+				print_usage(prgname);
+				return -1;
 			}
-
 			break;
-
+		}
+		case OPTION_ETH_DEST_NUM:
+		{
+			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;
+			}
+			break;
+		}
 		default:
 			print_usage(prgname);
 			return -1;
-- 
2.17.1


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

* [dpdk-dev] [PATCH 3/8] examples/packet_ordering: enhance getopt_long usage
  2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 2/8] examples/l3fwd-acl: " Ibtisam Tariq
@ 2020-10-29 12:53 ` Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 4/8] examples/performance-thread/l3fwd-thread: " Ibtisam Tariq
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-10-29 12:53 UTC (permalink / raw)
  To: marko.kovacevic, konstantin.ananyev, reshma.pattan,
	john.mcnamara, cristian.dumitrescu, jasvinder.singh, chenbo.xia,
	maxime.coquelin, xiaoyun.li
  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.

Bugzilla ID: 238
Fixes: 850f3733f8 ("examples/packet_ordering: new sample app")
Fixes: 016493307a ("examples/packet_ordering: add stats per worker thread")
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>
---
 examples/packet_ordering/main.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/examples/packet_ordering/main.c b/examples/packet_ordering/main.c
index a79d77a32..ac41d1a88 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 OPTION_DISABLE_REORDER "disable-reorder"
+	OPTION_DISABLE_REORDER_NUM = 256,
+#define OPTION_INSIGHT_WORKER "insight-worker"
+	OPTION_INSIGHT_WORKER_NUM,
+};
+
 unsigned int portmask;
 unsigned int disable_reorder;
 unsigned int insight_worker;
@@ -157,8 +164,8 @@ 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},
+		{OPTION_DISABLE_REORDER, 0, NULL, OPTION_DISABLE_REORDER_NUM},
+		{OPTION_INSIGHT_WORKER, 0, NULL, OPTION_INSIGHT_WORKER_NUM},
 		{NULL, 0, 0, 0}
 	};
 
@@ -177,17 +184,18 @@ parse_args(int argc, char **argv)
 			}
 			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 OPTION_DISABLE_REORDER_NUM:
+		{
+			printf("reorder disabled\n");
+			disable_reorder = 1;
 			break;
+		}
+		case OPTION_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] 17+ messages in thread

* [dpdk-dev] [PATCH 4/8] examples/performance-thread/l3fwd-thread: enhance getopt_long usage
  2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 2/8] examples/l3fwd-acl: " Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 3/8] examples/packet_ordering: " Ibtisam Tariq
@ 2020-10-29 12:53 ` Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 5/8] examples/qos_sched: " Ibtisam Tariq
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-10-29 12:53 UTC (permalink / raw)
  To: marko.kovacevic, konstantin.ananyev, reshma.pattan,
	john.mcnamara, cristian.dumitrescu, jasvinder.singh, chenbo.xia,
	maxime.coquelin, xiaoyun.li
  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.

Bugzilla ID: 238
Fixes: d48415e1fe ("examples/performance-thread: add l3fwd-thread app")
Cc: ian.betts@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
 .../performance-thread/l3fwd-thread/main.c    | 221 ++++++++++--------
 1 file changed, 121 insertions(+), 100 deletions(-)

diff --git a/examples/performance-thread/l3fwd-thread/main.c b/examples/performance-thread/l3fwd-thread/main.c
index e96076f29..3de33438f 100644
--- a/examples/performance-thread/l3fwd-thread/main.c
+++ b/examples/performance-thread/l3fwd-thread/main.c
@@ -2858,18 +2858,28 @@ parse_eth_dest(const char *optarg)
 		dest[c] = peer_addr[c];
 	*(uint64_t *)(val_eth + portid) = dest_eth_addr[portid];
 }
-
+enum {
 #define CMD_LINE_OPT_RX_CONFIG "rx"
+	CMD_LINE_OPT_RX_CONFIG_NUM = 256,
 #define CMD_LINE_OPT_TX_CONFIG "tx"
+	CMD_LINE_OPT_TX_CONFIG_NUM,
 #define CMD_LINE_OPT_STAT_LCORE "stat-lcore"
+	CMD_LINE_OPT_STAT_LCORE_NUM,
 #define CMD_LINE_OPT_ETH_DEST "eth-dest"
+	CMD_LINE_OPT_ETH_DEST_NUM,
 #define CMD_LINE_OPT_NO_NUMA "no-numa"
+	CMD_LINE_OPT_NO_NUMA_NUM,
 #define CMD_LINE_OPT_IPV6 "ipv6"
+	CMD_LINE_OPT_IPV6_NUM,
 #define CMD_LINE_OPT_ENABLE_JUMBO "enable-jumbo"
+	CMD_LINE_OPT_ENABLE_JUMBO_NUM,
 #define CMD_LINE_OPT_HASH_ENTRY_NUM "hash-entry-num"
+	CMD_LINE_OPT_HASH_ENTRY_NUM_NUM,
 #define CMD_LINE_OPT_NO_LTHREADS "no-lthreads"
+	CMD_LINE_OPT_NO_LTHREADS_NUM,
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
-
+	CMD_LINE_OPT_PARSE_PTYPE_NUM,
+};
 /* Parse the argument given in the command line of the application */
 static int
 parse_args(int argc, char **argv)
@@ -2879,16 +2889,26 @@ 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},
+		{CMD_LINE_OPT_RX_CONFIG, 1,
+				NULL, CMD_LINE_OPT_RX_CONFIG_NUM},
+		{CMD_LINE_OPT_TX_CONFIG, 1,
+				NULL, CMD_LINE_OPT_TX_CONFIG_NUM},
+		{CMD_LINE_OPT_STAT_LCORE, 1,
+				NULL, CMD_LINE_OPT_STAT_LCORE_NUM},
+		{CMD_LINE_OPT_ETH_DEST, 1,
+				NULL, CMD_LINE_OPT_ETH_DEST_NUM},
+		{CMD_LINE_OPT_NO_NUMA, 0,
+				NULL, CMD_LINE_OPT_NO_NUMA_NUM},
+		{CMD_LINE_OPT_IPV6, 0,
+				NULL, CMD_LINE_OPT_IPV6_NUM},
+		{CMD_LINE_OPT_ENABLE_JUMBO, 0,
+				NULL, CMD_LINE_OPT_ENABLE_JUMBO_NUM},
+		{CMD_LINE_OPT_HASH_ENTRY_NUM, 1,
+				NULL, CMD_LINE_OPT_HASH_ENTRY_NUM_NUM},
+		{CMD_LINE_OPT_NO_LTHREADS, 0,
+				NULL, CMD_LINE_OPT_NO_LTHREADS_NUM},
+		{CMD_LINE_OPT_PARSE_PTYPE, 0,
+				NULL, CMD_LINE_OPT_PARSE_PTYPE_NUM},
 		{NULL, 0, 0, 0}
 	};
 
@@ -2913,107 +2933,108 @@ parse_args(int argc, char **argv)
 			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 CMD_LINE_OPT_RX_CONFIG_NUM:
+		{
+			ret = parse_rx_config(optarg);
+			if (ret) {
+				printf("invalid rx-config\n");
+				print_usage(prgname);
+				return -1;
 			}
-
-			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;
-				}
+			break;
+		}
+		case CMD_LINE_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 CMD_LINE_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);
-
-			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 CMD_LINE_OPT_ETH_DEST_NUM:
+		{
+			parse_eth_dest(optarg);
+			break;
+		}
+		case CMD_LINE_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 CMD_LINE_OPT_IPV6_NUM:
+		{
+			printf("ipv6 is specified\n");
+			ipv6 = 1;
+			break;
+		}
 #endif
+		case CMD_LINE_OPT_NO_LTHREADS_NUM:
+		{
+			printf("l-threads model is disabled\n");
+			lthreads_on = 0;
+			break;
+		}
+		case CMD_LINE_OPT_PARSE_PTYPE_NUM:
+		{
+			printf("software packet type parsing enabled\n");
+			parse_ptype_on = 1;
+			break;
+		}
+		case CMD_LINE_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)) {
 
-			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;
-			}
-
-			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;
-			}
-
-			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");
+				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 CMD_LINE_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);
 			return -1;
-- 
2.17.1


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

* [dpdk-dev] [PATCH 5/8] examples/qos_sched: enhance getopt_long usage
  2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (2 preceding siblings ...)
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 4/8] examples/performance-thread/l3fwd-thread: " Ibtisam Tariq
@ 2020-10-29 12:53 ` Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 6/8] examples/vhost: " Ibtisam Tariq
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-10-29 12:53 UTC (permalink / raw)
  To: marko.kovacevic, konstantin.ananyev, reshma.pattan,
	john.mcnamara, cristian.dumitrescu, jasvinder.singh, chenbo.xia,
	maxime.coquelin, xiaoyun.li
  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.

Bugzilla ID: 238
Fixes: de3cfa2c98 ("sched: initial import")
Fixes: cb056611a8 ("eal: rename lcore master and slave")
Cc: stephen@networkplumber.org

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
 examples/qos_sched/args.c | 161 +++++++++++++++++++++++---------------
 1 file changed, 99 insertions(+), 62 deletions(-)

diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
index c62719623..8411a87b5 100644
--- a/examples/qos_sched/args.c
+++ b/examples/qos_sched/args.c
@@ -297,6 +297,25 @@ app_parse_burst_conf(const char *conf_str)
 	return 0;
 }
 
+enum {
+#define CMD_LINE_OPT_PFC "pfc"
+	CMD_LINE_OPT_PFC_NUM = 256,
+#define CMD_LINE_OPT_MNC "mnc"
+	CMD_LINE_OPT_MNC_NUM,
+#define CMD_LINE_OPT_RSZ "rsz"
+	CMD_LINE_OPT_RSZ_NUM,
+#define CMD_LINE_OPT_BSZ "bsz"
+	CMD_LINE_OPT_BSZ_NUM,
+#define CMD_LINE_OPT_MSZ "msz"
+	CMD_LINE_OPT_MSZ_NUM,
+#define CMD_LINE_OPT_RTH "rth"
+	CMD_LINE_OPT_RTH_NUM,
+#define CMD_LINE_OPT_TTH "tth"
+	CMD_LINE_OPT_TTH_NUM,
+#define CMD_LINE_OPT_CFG "cfg"
+	CMD_LINE_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,19 +325,26 @@ 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 },
+		{ CMD_LINE_OPT_PFC, 1,
+				NULL, CMD_LINE_OPT_PFC_NUM },
+		{ CMD_LINE_OPT_MNC, 1,
+				NULL, CMD_LINE_OPT_MNC_NUM},
+		{ CMD_LINE_OPT_RSZ, 1,
+				NULL, CMD_LINE_OPT_RSZ_NUM},
+		{ CMD_LINE_OPT_BSZ, 1,
+				NULL, CMD_LINE_OPT_BSZ_NUM},
+		{ CMD_LINE_OPT_MSZ, 1,
+				NULL, CMD_LINE_OPT_MSZ_NUM},
+		{ CMD_LINE_OPT_RTH, 1,
+				NULL, CMD_LINE_OPT_RTH_NUM},
+		{ CMD_LINE_OPT_TTH, 1,
+				NULL, CMD_LINE_OPT_TTH_NUM},
+		{ CMD_LINE_OPT_CFG, 1,
+				NULL, CMD_LINE_OPT_CFG_NUM},
 		{ NULL,  0, 0, 0 }
 	};
 
@@ -342,66 +368,77 @@ 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 CMD_LINE_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 CMD_LINE_OPT_MNC_NUM:
+			{
+				app_main_core = (uint32_t)atoi(optarg);
+				break;
+			}
+			case CMD_LINE_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 CMD_LINE_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 CMD_LINE_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 CMD_LINE_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 CMD_LINE_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 CMD_LINE_OPT_CFG_NUM:
+			{
+				cfg_profile = optarg;
+				break;
+			}
 			default:
 				app_usage(prgname);
 				return -1;
-- 
2.17.1


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

* [dpdk-dev] [PATCH 6/8] examples/vhost: enhance getopt_long usage
  2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (3 preceding siblings ...)
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 5/8] examples/qos_sched: " Ibtisam Tariq
@ 2020-10-29 12:53 ` Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 7/8] examples/vhost_crypto: " Ibtisam Tariq
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-10-29 12:53 UTC (permalink / raw)
  To: marko.kovacevic, konstantin.ananyev, reshma.pattan,
	john.mcnamara, cristian.dumitrescu, jasvinder.singh, chenbo.xia,
	maxime.coquelin, xiaoyun.li
  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.

Bugzilla ID: 238
Fixes: d19533e86f ("examples/vhost: copy old vhost example")
Fixes: bde19a4d4b ("examples/vhost: rename --dev-basename to --socket-file")
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>
---
 examples/vhost/main.c | 264 +++++++++++++++++++++++-------------------
 1 file changed, 143 insertions(+), 121 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index faa482245..8056cac7a 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -450,6 +450,29 @@ us_vhost_usage(const char *prgname)
 	       prgname);
 }
 
+enum {
+#define CMD_LINE_OPT_VM2VM               "vm2vm"
+	CMD_LINE_OPT_VM2VM_NUM = 256,
+#define CMD_LINE_OPT_RX_RETRY            "rx-retry"
+	CMD_LINE_OPT_RX_RETRY_NUM,
+#define CMD_LINE_OPT_RX_RETRY_DELAY      "rx-retry-delay"
+	CMD_LINE_OPT_RX_RETRY_DELAY_NUM,
+#define CMD_LINE_OPT_RX_RETRY_NUMB        "rx-retry-num"
+	CMD_LINE_OPT_RX_RETRY_NUMB_NUM,
+#define CMD_LINE_OPT_MERGEABLE           "mergeable"
+	CMD_LINE_OPT_MERGEABLE_NUM,
+#define CMD_LINE_OPT_STATS               "stats"
+	CMD_LINE_OPT_STATS_NUM,
+#define CMD_LINE_OPT_SOCKET_FILE         "socket-file"
+	CMD_LINE_OPT_SOCKET_FILE_NUM,
+#define CMD_LINE_OPT_TX_CSUM             "tx-csum"
+	CMD_LINE_OPT_TX_CSUM_NUM,
+#define CMD_LINE_OPT_TSO                 "tso"
+	CMD_LINE_OPT_TSO_NUM,
+#define CMD_LINE_OPT_CLIENT              "client"
+#define CMD_LINE_OPT_BUILTIN_NET_DRIVER  "builtin-net-driver"
+};
+
 /*
  * Parse the arguments given in the command line of the application.
  */
@@ -461,17 +484,27 @@ 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},
+		{CMD_LINE_OPT_VM2VM, required_argument,
+				NULL, CMD_LINE_OPT_VM2VM_NUM},
+		{CMD_LINE_OPT_RX_RETRY, required_argument,
+				NULL, CMD_LINE_OPT_RX_RETRY_NUM},
+		{CMD_LINE_OPT_RX_RETRY_DELAY, required_argument,
+				NULL, CMD_LINE_OPT_RX_RETRY_DELAY_NUM},
+		{CMD_LINE_OPT_RX_RETRY_NUMB, required_argument,
+				NULL, CMD_LINE_OPT_RX_RETRY_NUMB_NUM},
+		{CMD_LINE_OPT_MERGEABLE, required_argument,
+				NULL, CMD_LINE_OPT_MERGEABLE_NUM},
+		{CMD_LINE_OPT_STATS, required_argument,
+				NULL, CMD_LINE_OPT_STATS_NUM},
+		{CMD_LINE_OPT_SOCKET_FILE, required_argument,
+				NULL, CMD_LINE_OPT_SOCKET_FILE_NUM},
+		{CMD_LINE_OPT_TX_CSUM, required_argument,
+				NULL, CMD_LINE_OPT_TX_CSUM_NUM},
+		{CMD_LINE_OPT_TSO, required_argument,
+				NULL, CMD_LINE_OPT_TSO_NUM},
+		{CMD_LINE_OPT_CLIENT, no_argument, &client_mode, 1},
+		{CMD_LINE_OPT_BUILTIN_NET_DRIVER, no_argument,
+				&builtin_net_driver, 1},
 		{NULL, 0, 0, 0},
 	};
 
@@ -497,126 +530,115 @@ us_vhost_parse_args(int argc, char **argv)
 
 			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 CMD_LINE_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;
 			}
-
-			/* 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;
-				}
+			vm2vm_mode = (vm2vm_type)ret;
+			break;
+		}
+		case CMD_LINE_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/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;
+			enable_retry = ret;
+			break;
+		}
+		case CMD_LINE_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/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;
+			enable_tx_csum = ret;
+			break;
+		}
+		case CMD_LINE_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;
 			}
-
-			/* 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;
-				}
+			enable_tso = ret;
+			break;
+		}
+		case CMD_LINE_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;
 			}
-
-			/* 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;
-				}
+			burst_rx_delay_time = ret;
+			break;
+		}
+		case CMD_LINE_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;
 			}
-
-			/* 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;
-					}
-				}
+			burst_rx_retry_num = ret;
+			break;
+		}
+		case CMD_LINE_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;
 			}
-
-			/* 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;
-				}
+			break;
+		}
+		case CMD_LINE_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;
-
-			/* Invalid option - print options. */
+		}
+		case CMD_LINE_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;
+		}
+		/* Invalid option - print options. */
 		default:
 			us_vhost_usage(prgname);
 			return -1;
-- 
2.17.1


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

* [dpdk-dev] [PATCH 7/8] examples/vhost_crypto: enhance getopt_long usage
  2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (4 preceding siblings ...)
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 6/8] examples/vhost: " Ibtisam Tariq
@ 2020-10-29 12:53 ` Ibtisam Tariq
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 8/8] examples/tep_termination: " Ibtisam Tariq
  2020-10-29 22:07 ` [dpdk-dev] [PATCH 1/8] examples/fips_validation: " David Marchand
  7 siblings, 0 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-10-29 12:53 UTC (permalink / raw)
  To: marko.kovacevic, konstantin.ananyev, reshma.pattan,
	john.mcnamara, cristian.dumitrescu, jasvinder.singh, chenbo.xia,
	maxime.coquelin, xiaoyun.li
  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.

Bugzilla ID: 238
Fixes: f5188211c7 ("examples/vhost_crypto: add sample application")
Cc: roy.fan.zhang@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
 examples/vhost_crypto/main.c | 78 ++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 29 deletions(-)

diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
index 11ad49159..c68b409b3 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;
 
+enum {
 #define CONFIG_KEYWORD		"config"
+	CONFIG_KEYWORD_NUM = 256,
 #define SOCKET_FILE_KEYWORD	"socket-file"
+	SOCKET_FILE_KEYWORD_NUM,
 #define ZERO_COPY_KEYWORD	"zero-copy"
+	ZERO_COPY_KEYWORD_NUM,
 #define POLLING_KEYWORD		"guest-polling"
+	POLLING_KEYWORD_NUM,
+};
 
 #define NB_SOCKET_FIELDS	(2)
 
@@ -210,10 +216,14 @@ 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},
+			{SOCKET_FILE_KEYWORD, required_argument,
+					NULL, SOCKET_FILE_KEYWORD_NUM},
+			{CONFIG_KEYWORD, required_argument,
+					NULL, CONFIG_KEYWORD_NUM},
+			{ZERO_COPY_KEYWORD, no_argument,
+					NULL, ZERO_COPY_KEYWORD_NUM},
+			{POLLING_KEYWORD, no_argument,
+					NULL, POLLING_KEYWORD_NUM},
 			{NULL, 0, 0, 0}
 	};
 
@@ -222,36 +232,46 @@ vhost_crypto_parse_args(int argc, char **argv)
 	while ((opt = getopt_long(argc, argvopt, "s:",
 				  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 SOCKET_FILE_KEYWORD_NUM:
+		{
+			ret = parse_socket_arg(optarg);
+			if (ret < 0) {
 				vhost_crypto_usage(prgname);
-				return -EINVAL;
+				return ret;
 			}
 			break;
+		}
+		case CONFIG_KEYWORD_NUM:
+		{
+			ret = parse_config(optarg);
+			if (ret < 0) {
+				vhost_crypto_usage(prgname);
+				return ret;
+			}
+			break;
+		}
+		case ZERO_COPY_KEYWORD_NUM:
+		{
+			options.zero_copy =
+				RTE_VHOST_CRYPTO_ZERO_COPY_ENABLE;
+			break;
+		}
+		case POLLING_KEYWORD_NUM:
+		{
+			options.guest_polling = 1;
+			break;
+		}
 		default:
-			return -1;
+		{
+			vhost_crypto_usage(prgname);
+			return -EINVAL;
+		}
 		}
 	}
 
-- 
2.17.1


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

* [dpdk-dev] [PATCH 8/8] examples/tep_termination: enhance getopt_long usage
  2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (5 preceding siblings ...)
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 7/8] examples/vhost_crypto: " Ibtisam Tariq
@ 2020-10-29 12:53 ` Ibtisam Tariq
  2020-10-29 13:16   ` David Marchand
  2020-10-29 22:07 ` [dpdk-dev] [PATCH 1/8] examples/fips_validation: " David Marchand
  7 siblings, 1 reply; 17+ messages in thread
From: Ibtisam Tariq @ 2020-10-29 12:53 UTC (permalink / raw)
  To: marko.kovacevic, konstantin.ananyev, reshma.pattan,
	john.mcnamara, cristian.dumitrescu, jasvinder.singh, chenbo.xia,
	maxime.coquelin, xiaoyun.li
  Cc: dev, Ibtisam Tariq, jijiang.liu

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.

Bugzilla ID: 238
Fixes: a50245ede7 ("examples/tep_term: initialize VXLAN sample")
Fixes: 2bb43bd435 ("examples/tep_term: add TSO offload configuration")
Fixes: 39c6daca9b ("examples/tep_term: add UDP tunneling port configuration")
Fixes: 9b96dd2609 ("examples/tep_term: add inner checksum Tx offload configuration")
Fixes: e627e8843d ("examples/tep_term: add tunnel filter type configuration")
Fixes: c6a0fb5f54 ("examples/tep_term: add encap/decap configuration")
Cc: jijiang.liu@intel.com

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
---
 examples/tep_termination/main.c | 341 ++++++++++++++++----------------
 1 file changed, 173 insertions(+), 168 deletions(-)

diff --git a/examples/tep_termination/main.c b/examples/tep_termination/main.c
index 15bf8bbf7..089d8cc7a 100644
--- a/examples/tep_termination/main.c
+++ b/examples/tep_termination/main.c
@@ -75,18 +75,32 @@
 /* Used to compare MAC addresses. */
 #define MAC_ADDR_CMP 0xFFFFFFFFFFFFULL
 
+enum {
 #define CMD_LINE_OPT_NB_DEVICES "nb-devices"
+	CMD_LINE_OPT_NB_DEVICES_NUM = 256,
 #define CMD_LINE_OPT_UDP_PORT "udp-port"
+	CMD_LINE_OPT_UDP_PORT_NUM,
 #define CMD_LINE_OPT_TX_CHECKSUM "tx-checksum"
+	CMD_LINE_OPT_TX_CHECKSUM_NUM,
 #define CMD_LINE_OPT_TSO_SEGSZ "tso-segsz"
+	CMD_LINE_OPT_TSO_SEGSZ_NUM,
 #define CMD_LINE_OPT_FILTER_TYPE "filter-type"
+	CMD_LINE_OPT_FILTER_TYPE_NUM,
 #define CMD_LINE_OPT_ENCAP "encap"
+	CMD_LINE_OPT_ENCAP_NUM,
 #define CMD_LINE_OPT_DECAP "decap"
+	CMD_LINE_OPT_DECAP_NUM,
 #define CMD_LINE_OPT_RX_RETRY "rx-retry"
+	CMD_LINE_OPT_RX_RETRY_NUM,
 #define CMD_LINE_OPT_RX_RETRY_DELAY "rx-retry-delay"
-#define CMD_LINE_OPT_RX_RETRY_NUM "rx-retry-num"
+	CMD_LINE_OPT_RX_RETRY_DELAY_NUM,
+#define CMD_LINE_OPT_RX_RETRY_NUMB "rx-retry-num"
+	CMD_LINE_OPT_RX_RETRY_NUMB_NUM,
 #define CMD_LINE_OPT_STATS "stats"
+	CMD_LINE_OPT_STATS_NUM,
 #define CMD_LINE_OPT_DEV_BASENAME "dev-basename"
+	CMD_LINE_OPT_DEV_BASENAME_NUM,
+};
 
 /* mask of enabled ports */
 static uint32_t enabled_port_mask;
@@ -268,18 +282,30 @@ tep_termination_parse_args(int argc, char **argv)
 	unsigned i;
 	const char *prgname = argv[0];
 	static struct option long_option[] = {
-		{CMD_LINE_OPT_NB_DEVICES, required_argument, NULL, 0},
-		{CMD_LINE_OPT_UDP_PORT, required_argument, NULL, 0},
-		{CMD_LINE_OPT_TX_CHECKSUM, required_argument, NULL, 0},
-		{CMD_LINE_OPT_TSO_SEGSZ, required_argument, NULL, 0},
-		{CMD_LINE_OPT_DECAP, required_argument, NULL, 0},
-		{CMD_LINE_OPT_ENCAP, required_argument, NULL, 0},
-		{CMD_LINE_OPT_FILTER_TYPE, required_argument, NULL, 0},
-		{CMD_LINE_OPT_RX_RETRY, required_argument, NULL, 0},
-		{CMD_LINE_OPT_RX_RETRY_DELAY, required_argument, NULL, 0},
-		{CMD_LINE_OPT_RX_RETRY_NUM, required_argument, NULL, 0},
-		{CMD_LINE_OPT_STATS, required_argument, NULL, 0},
-		{CMD_LINE_OPT_DEV_BASENAME, required_argument, NULL, 0},
+		{CMD_LINE_OPT_NB_DEVICES, required_argument,
+				NULL, CMD_LINE_OPT_NB_DEVICES_NUM},
+		{CMD_LINE_OPT_UDP_PORT, required_argument,
+				NULL, CMD_LINE_OPT_UDP_PORT_NUM},
+		{CMD_LINE_OPT_TX_CHECKSUM, required_argument,
+				NULL, CMD_LINE_OPT_TX_CHECKSUM_NUM},
+		{CMD_LINE_OPT_TSO_SEGSZ, required_argument,
+				NULL, CMD_LINE_OPT_TSO_SEGSZ_NUM},
+		{CMD_LINE_OPT_DECAP, required_argument,
+				NULL, CMD_LINE_OPT_DECAP_NUM},
+		{CMD_LINE_OPT_ENCAP, required_argument,
+				NULL, CMD_LINE_OPT_ENCAP_NUM},
+		{CMD_LINE_OPT_FILTER_TYPE, required_argument,
+				NULL, CMD_LINE_OPT_FILTER_TYPE_NUM},
+		{CMD_LINE_OPT_RX_RETRY, required_argument,
+				NULL, CMD_LINE_OPT_RX_RETRY_NUM},
+		{CMD_LINE_OPT_RX_RETRY_DELAY, required_argument,
+				NULL, CMD_LINE_OPT_RX_RETRY_DELAY_NUM},
+		{CMD_LINE_OPT_RX_RETRY_NUMB, required_argument,
+				NULL, CMD_LINE_OPT_RX_RETRY_NUMB_NUM},
+		{CMD_LINE_OPT_STATS, required_argument,
+				NULL, CMD_LINE_OPT_STATS_NUM},
+		{CMD_LINE_OPT_DEV_BASENAME, required_argument,
+				NULL, CMD_LINE_OPT_DEV_BASENAME_NUM},
 		{NULL, 0, 0, 0},
 	};
 
@@ -297,174 +323,153 @@ tep_termination_parse_args(int argc, char **argv)
 				return -1;
 			}
 			break;
-		case 0:
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_NB_DEVICES,
-				sizeof(CMD_LINE_OPT_NB_DEVICES))) {
-				ret = parse_num_opt(optarg, MAX_DEVICES);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-					"Invalid argument for nb-devices [0-%d]\n",
-					MAX_DEVICES);
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					nb_devices = ret;
-			}
 
-			/* Enable/disable retries on RX. */
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_RX_RETRY,
-				sizeof(CMD_LINE_OPT_RX_RETRY))) {
-				ret = parse_num_opt(optarg, 1);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for rx-retry [0|1]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					enable_retry = ret;
+		case CMD_LINE_OPT_NB_DEVICES_NUM:
+		{
+			ret = parse_num_opt(optarg, MAX_DEVICES);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+				"Invalid argument for nb-devices [0-%d]\n",
+				MAX_DEVICES);
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_TSO_SEGSZ,
-				sizeof(CMD_LINE_OPT_TSO_SEGSZ))) {
-				ret = parse_num_opt(optarg, INT16_MAX);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for TCP segment size [0-N]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					tso_segsz = ret;
+			nb_devices = ret;
+			break;
+		}
+		case CMD_LINE_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");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			if (!strncmp(long_option[option_index].name,
-					CMD_LINE_OPT_UDP_PORT,
-					sizeof(CMD_LINE_OPT_UDP_PORT))) {
-				ret = parse_num_opt(optarg, INT16_MAX);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for UDP port [0-N]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					udp_port = ret;
+			enable_retry = ret;
+			break;
+		}
+		case CMD_LINE_OPT_TSO_SEGSZ_NUM:
+		{
+			ret = parse_num_opt(optarg, INT16_MAX);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for TCP segment size [0-N]\n");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			/* Specify the retries delay time (in useconds) on RX.*/
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_RX_RETRY_DELAY,
-				sizeof(CMD_LINE_OPT_RX_RETRY_DELAY))) {
-				ret = parse_num_opt(optarg, INT32_MAX);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for rx-retry-delay [0-N]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					burst_rx_delay_time = ret;
+			tso_segsz = ret;
+			break;
+		}
+		case CMD_LINE_OPT_UDP_PORT_NUM:
+		{
+			ret = parse_num_opt(optarg, INT16_MAX);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for UDP port [0-N]\n");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			/* Specify the retries number on RX. */
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_RX_RETRY_NUM,
-				sizeof(CMD_LINE_OPT_RX_RETRY_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");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					burst_rx_retry_num = ret;
+			udp_port = ret;
+			break;
+		}
+		case CMD_LINE_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");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_TX_CHECKSUM,
-				sizeof(CMD_LINE_OPT_TX_CHECKSUM))) {
-				ret = parse_num_opt(optarg, 1);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for tx-checksum [0|1]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					tx_checksum = ret;
+			burst_rx_delay_time = ret;
+			break;
+		}
+		case CMD_LINE_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");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			if (!strncmp(long_option[option_index].name,
-					CMD_LINE_OPT_FILTER_TYPE,
-					sizeof(CMD_LINE_OPT_FILTER_TYPE))) {
-				ret = parse_num_opt(optarg, 3);
-				if ((ret == -1) || (ret == 0)) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for filter type [1-3]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					filter_idx = ret - 1;
+			burst_rx_retry_num = ret;
+			break;
+		}
+		case CMD_LINE_OPT_TX_CHECKSUM_NUM:
+		{
+			ret = parse_num_opt(optarg, 1);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for tx-checksum [0|1]\n");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			/* Enable/disable encapsulation on RX. */
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_DECAP,
-				sizeof(CMD_LINE_OPT_DECAP))) {
-				ret = parse_num_opt(optarg, 1);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for decap [0|1]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					rx_decap = ret;
+			tx_checksum = ret;
+			break;
+		}
+		case CMD_LINE_OPT_FILTER_TYPE_NUM:
+		{
+			ret = parse_num_opt(optarg, 3);
+			if ((ret == -1) || (ret == 0)) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for filter type [1-3]\n");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			/* Enable/disable encapsulation on TX. */
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_ENCAP,
-				sizeof(CMD_LINE_OPT_ENCAP))) {
-				ret = parse_num_opt(optarg, 1);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for encap [0|1]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					tx_encap = ret;
+			filter_idx = ret - 1;
+			break;
+		}
+		case CMD_LINE_OPT_DECAP_NUM:
+		{
+			ret = parse_num_opt(optarg, 1);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for decap [0|1]\n");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			/* Enable/disable stats. */
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_STATS,
-				sizeof(CMD_LINE_OPT_STATS))) {
-				ret = parse_num_opt(optarg, INT32_MAX);
-				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-							"Invalid argument for stats [0..N]\n");
-					tep_termination_usage(prgname);
-					return -1;
-				} else
-					enable_stats = ret;
+			rx_decap = ret;
+			break;
+		}
+		case CMD_LINE_OPT_ENCAP_NUM:
+		{
+			ret = parse_num_opt(optarg, 1);
+			if (ret == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for encap [0|1]\n");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
-			/* Set character device basename. */
-			if (!strncmp(long_option[option_index].name,
-				CMD_LINE_OPT_DEV_BASENAME,
-				sizeof(CMD_LINE_OPT_DEV_BASENAME))) {
-				if (us_vhost_parse_basename(optarg) == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG,
-						"Invalid argument for character "
-						"device basename (Max %d characters)\n",
-						MAX_BASENAME_SZ);
-					tep_termination_usage(prgname);
-					return -1;
-				}
+			tx_encap = ret;
+			break;
+		}
+		case CMD_LINE_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");
+				tep_termination_usage(prgname);
+				return -1;
 			}
-
+			enable_stats = ret;
 			break;
-
-			/* Invalid option - print options. */
+		}
+		case CMD_LINE_OPT_DEV_BASENAME_NUM:
+		{
+			if (us_vhost_parse_basename(optarg) == -1) {
+				RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for character "
+					"device basename (Max %d characters)\n",
+					MAX_BASENAME_SZ);
+				tep_termination_usage(prgname);
+				return -1;
+			}
+			break;
+		}
+		/* Invalid option - print options. */
 		default:
 			tep_termination_usage(prgname);
 			return -1;
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH 8/8] examples/tep_termination: enhance getopt_long usage
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 8/8] examples/tep_termination: " Ibtisam Tariq
@ 2020-10-29 13:16   ` David Marchand
  2020-11-02  8:18     ` Ibtisam Tariq
  0 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2020-10-29 13:16 UTC (permalink / raw)
  To: Ibtisam Tariq
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev, jijiang.liu

Hello Ibtisam,

On Thu, Oct 29, 2020 at 1:58 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.
>
> Bugzilla ID: 238
> Fixes: a50245ede7 ("examples/tep_term: initialize VXLAN sample")
> Fixes: 2bb43bd435 ("examples/tep_term: add TSO offload configuration")
> Fixes: 39c6daca9b ("examples/tep_term: add UDP tunneling port configuration")
> Fixes: 9b96dd2609 ("examples/tep_term: add inner checksum Tx offload configuration")
> Fixes: e627e8843d ("examples/tep_term: add tunnel filter type configuration")
> Fixes: c6a0fb5f54 ("examples/tep_term: add encap/decap configuration")
> Cc: jijiang.liu@intel.com
>
> Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>

This patch can be dropped.
Thomas just merged this example removal.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage
  2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
                   ` (6 preceding siblings ...)
  2020-10-29 12:53 ` [dpdk-dev] [PATCH 8/8] examples/tep_termination: " Ibtisam Tariq
@ 2020-10-29 22:07 ` David Marchand
  2020-11-02  8:32   ` Ibtisam Tariq
  7 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2020-10-29 22:07 UTC (permalink / raw)
  To: Ibtisam Tariq
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev

Hello Ibtisam,

On Thu, Oct 29, 2020 at 1:55 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.
>
> Bugzilla ID: 238
> Fixes: 3d0fad56b74 ("examples/fips_validation: add crypto FIPS application"}

I consider this bz as an enhancement, for better readability /
consistency in the project code.
This is not a bugfix, and I would not flag the patches with a Fixes: tag.


> Cc: marko.kovacevic@intel.com
>
> Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
> ---
>  examples/fips_validation/main.c | 241 +++++++++++++++++++-------------
>  1 file changed, 143 insertions(+), 98 deletions(-)
>
> diff --git a/examples/fips_validation/main.c b/examples/fips_validation/main.c
> index 07532c956..5fb76b421 100644
> --- a/examples/fips_validation/main.c
> +++ b/examples/fips_validation/main.c
> @@ -15,17 +15,31 @@
>  #include "fips_validation.h"
>  #include "fips_dev_self_test.h"
>
> +enum{

Missing space.


The _KEYWORD suffix gives no info and can be dropped in all those
defines / enums.

>  #define REQ_FILE_PATH_KEYWORD  "req-file"
> +       /* first long only option value must be >= 256, so that we won't
> +        * conflict with short options
> +        */

This comment is copied from the EAL header, but there is no mapping to
a short option in this example.
You can drop it.

> +       REQ_FILE_PATH_KEYWORD_NUM = 256,
>  #define RSP_FILE_PATH_KEYWORD  "rsp-file"
> +       RSP_FILE_PATH_KEYWORD_NUM,
>  #define MBUF_DATAROOM_KEYWORD  "mbuf-dataroom"
> +       MBUF_DATAROOM_KEYWORD_NUM,
>  #define FOLDER_KEYWORD         "path-is-folder"
> +       FOLDER_KEYWORD_NUM,
>  #define CRYPTODEV_KEYWORD      "cryptodev"
> +       CRYPTODEV_KEYWORD_NUM,
>  #define CRYPTODEV_ID_KEYWORD   "cryptodev-id"
> +       CRYPTODEV_ID_KEYWORD_NUM,
>  #define CRYPTODEV_ST_KEYWORD   "self-test"
> +       CRYPTODEV_ST_KEYWORD_NUM,
>  #define CRYPTODEV_BK_ID_KEYWORD        "broken-test-id"
> +       CRYPTODEV_BK_ID_KEYWORD_NUM,
>  #define CRYPTODEV_BK_DIR_KEY   "broken-test-dir"
> +       CRYPTODEV_BK_DIR_KEY_NUM,


Those next two defines have nothing to do with getopt options and they
are only used once.
You can directly replace them as their fixed string later in the
parsing code, and drop the defines.


>  #define CRYPTODEV_ENC_KEYWORD  "enc"
>  #define CRYPTODEV_DEC_KEYWORD  "dec"
> +};
>
>  struct fips_test_vector vec;
>  struct fips_test_interim_info info;
> @@ -226,15 +240,24 @@ 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},

Single indent is enough.


> +                       {REQ_FILE_PATH_KEYWORD, required_argument,
> +                                       NULL, REQ_FILE_PATH_KEYWORD_NUM},
> +                       {RSP_FILE_PATH_KEYWORD, required_argument,
> +                                       NULL, RSP_FILE_PATH_KEYWORD_NUM},
> +                       {FOLDER_KEYWORD, no_argument,
> +                                       NULL, FOLDER_KEYWORD_NUM},
> +                       {MBUF_DATAROOM_KEYWORD, required_argument,
> +                                       NULL, MBUF_DATAROOM_KEYWORD_NUM},
> +                       {CRYPTODEV_KEYWORD, required_argument,
> +                                       NULL, CRYPTODEV_KEYWORD_NUM},
> +                       {CRYPTODEV_ID_KEYWORD, required_argument,
> +                                       NULL, CRYPTODEV_ID_KEYWORD_NUM},
> +                       {CRYPTODEV_ST_KEYWORD, no_argument,
> +                                       NULL, CRYPTODEV_ST_KEYWORD_NUM},
> +                       {CRYPTODEV_BK_ID_KEYWORD, required_argument,
> +                                       NULL, CRYPTODEV_BK_ID_KEYWORD_NUM},
> +                       {CRYPTODEV_BK_DIR_KEY, required_argument,
> +                                       NULL, CRYPTODEV_BK_DIR_KEY_NUM},
>                         {NULL, 0, 0, 0}
>         };
>
> @@ -251,105 +274,127 @@ cryptodev_fips_validate_parse_args(int argc, char **argv)
>         while ((opt = getopt_long(argc, argvopt, "s:",
>                                   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 REQ_FILE_PATH_KEYWORD_NUM:
> +               {

Unless you need a temp variable, there is no need for a block for each
case: statement.
Simply:
    case REQ_FILE_PATH_NUM:
        env.req_path = optarg;
        break;

> +                       env.req_path = optarg;
> +                       break;
> +               }
> +               case RSP_FILE_PATH_KEYWORD_NUM:
> +               {
> +                       env.rsp_path = optarg;
> +                       break;
> +               }
> +               case FOLDER_KEYWORD_NUM:
> +               {
> +                       env.is_path_folder = 1;
> +                       break;
> +               }
> +               case CRYPTODEV_KEYWORD_NUM:
> +               {
> +                       ret = parse_cryptodev_arg(optarg);
> +                       if (ret < 0) {
> +                               cryptodev_fips_validate_usage(prgname);
> +                               return -EINVAL;
> +                       }
>
> -                               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;
> -                               }
> +                       break;
> +               }
> +               case CRYPTODEV_ID_KEYWORD_NUM:
> +               {
> +                       ret = parse_cryptodev_id_arg(optarg);
> +                       if (ret < 0) {
> +                               cryptodev_fips_validate_usage(prgname);
> +                               return -EINVAL;
> +                       }
> +                       break;
> +               }
> +               case CRYPTODEV_ST_KEYWORD_NUM:
> +               {
> +                       env.self_test = 1;
> +                       break;
> +               }
> +               case CRYPTODEV_BK_ID_KEYWORD_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 (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;
> -                               }
> +                       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 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 (data_room_size == 0 ||
> -                                               data_room_size > UINT16_MAX) {
> -                                       cryptodev_fips_validate_usage(prgname);
> -                                       return -EINVAL;
> -                               }
> +                       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;
> +                       }
> +                       break;
> +               }
> +               case MBUF_DATAROOM_KEYWORD_NUM:
> +               {
> +                       uint32_t data_room_size;

Here, I don't think we need a temp storage.
If the value is invalid, the parsing and then init will fail.
You can directly pass &env.mbuf_data_room to parser_read_uint32 and
check its value.


>
> -                               env.mbuf_data_room = data_room_size;
> -                       } else {
> +                       if (parser_read_uint32(&data_room_size,
> +                                       optarg) < 0) {
>                                 cryptodev_fips_validate_usage(prgname);
>                                 return -EINVAL;
>                         }
> +
> +                       if (data_room_size == 0 ||
> +                                       data_room_size > UINT16_MAX) {
> +                               cryptodev_fips_validate_usage(prgname);
> +                               return -EINVAL;
> +                       }
> +
> +                       env.mbuf_data_room = data_room_size;
> +
>                         break;
> +               }
>                 default:
> -                       return -1;
> +               {
> +                       cryptodev_fips_validate_usage(prgname);
> +                       return -EINVAL;
> +               }
>                 }
>         }
>
> --
> 2.17.1
>

I did not look too much at the rest of the series, but I guess most of
those comments apply to other patches.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH 8/8] examples/tep_termination: enhance getopt_long usage
  2020-10-29 13:16   ` David Marchand
@ 2020-11-02  8:18     ` Ibtisam Tariq
  0 siblings, 0 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-11-02  8:18 UTC (permalink / raw)
  To: David Marchand
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev, jijiang.liu

Hi David,

Thank you for the update.

On Thu, Oct 29, 2020 at 6:16 PM David Marchand <david.marchand@redhat.com>
wrote:

> Hello Ibtisam,
>
> On Thu, Oct 29, 2020 at 1:58 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.
> >
> > Bugzilla ID: 238
> > Fixes: a50245ede7 ("examples/tep_term: initialize VXLAN sample")
> > Fixes: 2bb43bd435 ("examples/tep_term: add TSO offload configuration")
> > Fixes: 39c6daca9b ("examples/tep_term: add UDP tunneling port
> configuration")
> > Fixes: 9b96dd2609 ("examples/tep_term: add inner checksum Tx offload
> configuration")
> > Fixes: e627e8843d ("examples/tep_term: add tunnel filter type
> configuration")
> > Fixes: c6a0fb5f54 ("examples/tep_term: add encap/decap configuration")
> > Cc: jijiang.liu@intel.com
> >
> > Reported-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
>
> This patch can be dropped.
> Thomas just merged this example removal.
>
>
> --
> David Marchand
>
>

-- 
- Ibtisam

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

* Re: [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage
  2020-10-29 22:07 ` [dpdk-dev] [PATCH 1/8] examples/fips_validation: " David Marchand
@ 2020-11-02  8:32   ` Ibtisam Tariq
  2020-11-04 10:00     ` Ibtisam Tariq
  0 siblings, 1 reply; 17+ messages in thread
From: Ibtisam Tariq @ 2020-11-02  8:32 UTC (permalink / raw)
  To: David Marchand
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev

Hi David,

Thank you for reviewing the patch. I will submit the v2 of the patchset
with new updates.

On Fri, Oct 30, 2020 at 3:07 AM David Marchand <david.marchand@redhat.com>
wrote:

> Hello Ibtisam,
>
> On Thu, Oct 29, 2020 at 1:55 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.
> >
> > Bugzilla ID: 238
> > Fixes: 3d0fad56b74 ("examples/fips_validation: add crypto FIPS
> application"}
>
> I consider this bz as an enhancement, for better readability /
> consistency in the project code.
> This is not a bugfix, and I would not flag the patches with a Fixes: tag.
>
>
> > Cc: marko.kovacevic@intel.com
> >
> > Reported-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
> > ---
> >  examples/fips_validation/main.c | 241 +++++++++++++++++++-------------
> >  1 file changed, 143 insertions(+), 98 deletions(-)
> >
> > diff --git a/examples/fips_validation/main.c
> b/examples/fips_validation/main.c
> > index 07532c956..5fb76b421 100644
> > --- a/examples/fips_validation/main.c
> > +++ b/examples/fips_validation/main.c
> > @@ -15,17 +15,31 @@
> >  #include "fips_validation.h"
> >  #include "fips_dev_self_test.h"
> >
> > +enum{
>
> Missing space.
>
>
> The _KEYWORD suffix gives no info and can be dropped in all those
> defines / enums.
>
> >  #define REQ_FILE_PATH_KEYWORD  "req-file"
> > +       /* first long only option value must be >= 256, so that we won't
> > +        * conflict with short options
> > +        */
>
> This comment is copied from the EAL header, but there is no mapping to
> a short option in this example.
> You can drop it.
>
> > +       REQ_FILE_PATH_KEYWORD_NUM = 256,
> >  #define RSP_FILE_PATH_KEYWORD  "rsp-file"
> > +       RSP_FILE_PATH_KEYWORD_NUM,
> >  #define MBUF_DATAROOM_KEYWORD  "mbuf-dataroom"
> > +       MBUF_DATAROOM_KEYWORD_NUM,
> >  #define FOLDER_KEYWORD         "path-is-folder"
> > +       FOLDER_KEYWORD_NUM,
> >  #define CRYPTODEV_KEYWORD      "cryptodev"
> > +       CRYPTODEV_KEYWORD_NUM,
> >  #define CRYPTODEV_ID_KEYWORD   "cryptodev-id"
> > +       CRYPTODEV_ID_KEYWORD_NUM,
> >  #define CRYPTODEV_ST_KEYWORD   "self-test"
> > +       CRYPTODEV_ST_KEYWORD_NUM,
> >  #define CRYPTODEV_BK_ID_KEYWORD        "broken-test-id"
> > +       CRYPTODEV_BK_ID_KEYWORD_NUM,
> >  #define CRYPTODEV_BK_DIR_KEY   "broken-test-dir"
> > +       CRYPTODEV_BK_DIR_KEY_NUM,
>
>
> Those next two defines have nothing to do with getopt options and they
> are only used once.
> You can directly replace them as their fixed string later in the
> parsing code, and drop the defines.
>
>
> >  #define CRYPTODEV_ENC_KEYWORD  "enc"
> >  #define CRYPTODEV_DEC_KEYWORD  "dec"
> > +};
> >
> >  struct fips_test_vector vec;
> >  struct fips_test_interim_info info;
> > @@ -226,15 +240,24 @@ 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},
>
> Single indent is enough.
>
>
> > +                       {REQ_FILE_PATH_KEYWORD, required_argument,
> > +                                       NULL, REQ_FILE_PATH_KEYWORD_NUM},
> > +                       {RSP_FILE_PATH_KEYWORD, required_argument,
> > +                                       NULL, RSP_FILE_PATH_KEYWORD_NUM},
> > +                       {FOLDER_KEYWORD, no_argument,
> > +                                       NULL, FOLDER_KEYWORD_NUM},
> > +                       {MBUF_DATAROOM_KEYWORD, required_argument,
> > +                                       NULL, MBUF_DATAROOM_KEYWORD_NUM},
> > +                       {CRYPTODEV_KEYWORD, required_argument,
> > +                                       NULL, CRYPTODEV_KEYWORD_NUM},
> > +                       {CRYPTODEV_ID_KEYWORD, required_argument,
> > +                                       NULL, CRYPTODEV_ID_KEYWORD_NUM},
> > +                       {CRYPTODEV_ST_KEYWORD, no_argument,
> > +                                       NULL, CRYPTODEV_ST_KEYWORD_NUM},
> > +                       {CRYPTODEV_BK_ID_KEYWORD, required_argument,
> > +                                       NULL,
> CRYPTODEV_BK_ID_KEYWORD_NUM},
> > +                       {CRYPTODEV_BK_DIR_KEY, required_argument,
> > +                                       NULL, CRYPTODEV_BK_DIR_KEY_NUM},
> >                         {NULL, 0, 0, 0}
> >         };
> >
> > @@ -251,105 +274,127 @@ cryptodev_fips_validate_parse_args(int argc,
> char **argv)
> >         while ((opt = getopt_long(argc, argvopt, "s:",
> >                                   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 REQ_FILE_PATH_KEYWORD_NUM:
> > +               {
>
> Unless you need a temp variable, there is no need for a block for each
> case: statement.
> Simply:
>     case REQ_FILE_PATH_NUM:
>         env.req_path = optarg;
>         break;
>
> > +                       env.req_path = optarg;
> > +                       break;
> > +               }
> > +               case RSP_FILE_PATH_KEYWORD_NUM:
> > +               {
> > +                       env.rsp_path = optarg;
> > +                       break;
> > +               }
> > +               case FOLDER_KEYWORD_NUM:
> > +               {
> > +                       env.is_path_folder = 1;
> > +                       break;
> > +               }
> > +               case CRYPTODEV_KEYWORD_NUM:
> > +               {
> > +                       ret = parse_cryptodev_arg(optarg);
> > +                       if (ret < 0) {
> > +                               cryptodev_fips_validate_usage(prgname);
> > +                               return -EINVAL;
> > +                       }
> >
> > -                               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;
> > -                               }
> > +                       break;
> > +               }
> > +               case CRYPTODEV_ID_KEYWORD_NUM:
> > +               {
> > +                       ret = parse_cryptodev_id_arg(optarg);
> > +                       if (ret < 0) {
> > +                               cryptodev_fips_validate_usage(prgname);
> > +                               return -EINVAL;
> > +                       }
> > +                       break;
> > +               }
> > +               case CRYPTODEV_ST_KEYWORD_NUM:
> > +               {
> > +                       env.self_test = 1;
> > +                       break;
> > +               }
> > +               case CRYPTODEV_BK_ID_KEYWORD_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 (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;
> > -                               }
> > +                       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 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 (data_room_size == 0 ||
> > -                                               data_room_size >
> UINT16_MAX) {
> > -
>  cryptodev_fips_validate_usage(prgname);
> > -                                       return -EINVAL;
> > -                               }
> > +                       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;
> > +                       }
> > +                       break;
> > +               }
> > +               case MBUF_DATAROOM_KEYWORD_NUM:
> > +               {
> > +                       uint32_t data_room_size;
>
> Here, I don't think we need a temp storage.
> If the value is invalid, the parsing and then init will fail.
> You can directly pass &env.mbuf_data_room to parser_read_uint32 and
> check its value.
>
>
> >
> > -                               env.mbuf_data_room = data_room_size;
> > -                       } else {
> > +                       if (parser_read_uint32(&data_room_size,
> > +                                       optarg) < 0) {
> >                                 cryptodev_fips_validate_usage(prgname);
> >                                 return -EINVAL;
> >                         }
> > +
> > +                       if (data_room_size == 0 ||
> > +                                       data_room_size > UINT16_MAX) {
> > +                               cryptodev_fips_validate_usage(prgname);
> > +                               return -EINVAL;
> > +                       }
> > +
> > +                       env.mbuf_data_room = data_room_size;
> > +
> >                         break;
> > +               }
> >                 default:
> > -                       return -1;
> > +               {
> > +                       cryptodev_fips_validate_usage(prgname);
> > +                       return -EINVAL;
> > +               }
> >                 }
> >         }
> >
> > --
> > 2.17.1
> >
>
> I did not look too much at the rest of the series, but I guess most of
> those comments apply to other patches.
>
>
> --
> David Marchand
>
>

-- 
- Ibtisam

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

* Re: [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage
  2020-11-02  8:32   ` Ibtisam Tariq
@ 2020-11-04 10:00     ` Ibtisam Tariq
  2020-11-05  8:59       ` David Marchand
  0 siblings, 1 reply; 17+ messages in thread
From: Ibtisam Tariq @ 2020-11-04 10:00 UTC (permalink / raw)
  To: David Marchand
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev

Hello David,

In reference to this comment
> +               case MBUF_DATAROOM_KEYWORD_NUM:
> +               {
> +                       uint32_t data_room_size;

Here, I don't think we need a temp storage.
If the value is invalid, the parsing and then init will fail.
You can directly pass &env.mbuf_data_room to parser_read_uint32 and
check its value.



>
> -                               env.mbuf_data_room = data_room_size;
> -                       } else {
> +                       if (parser_read_uint32(&data_room_size,
> +                                       optarg) < 0) {
>                                 cryptodev_fips_validate_usage(prgname);
>                                 return -EINVAL;
>                         }
> +
> +                       if (data_room_size == 0 ||
> +                                       data_room_size > UINT16_MAX) {
> +                               cryptodev_fips_validate_usage(prgname);
> +                               return -EINVAL;
> +                       }
> +
> +                       env.mbuf_data_room = data_room_size;
> +
>                         break;
> +               }

The type of env.mbuf_data_room is uint16_t and the temp variable type
is uint32_t. In my opinion, the temp variable size is bigger than
env.mbuf_data_room to handle overflow value.

-- 
- Ibtisam

On Mon, Nov 2, 2020 at 1:32 PM Ibtisam Tariq <ibtisam.tariq@emumba.com> wrote:
>
> Hi David,
>
> Thank you for reviewing the patch. I will submit the v2 of the patchset with new updates.
>
> On Fri, Oct 30, 2020 at 3:07 AM David Marchand <david.marchand@redhat.com> wrote:
>>
>> Hello Ibtisam,
>>
>> On Thu, Oct 29, 2020 at 1:55 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.
>> >
>> > Bugzilla ID: 238
>> > Fixes: 3d0fad56b74 ("examples/fips_validation: add crypto FIPS application"}
>>
>> I consider this bz as an enhancement, for better readability /
>> consistency in the project code.
>> This is not a bugfix, and I would not flag the patches with a Fixes: tag.
>>
>>
>> > Cc: marko.kovacevic@intel.com
>> >
>> > Reported-by: David Marchand <david.marchand@redhat.com>
>> > Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
>> > ---
>> >  examples/fips_validation/main.c | 241 +++++++++++++++++++-------------
>> >  1 file changed, 143 insertions(+), 98 deletions(-)
>> >
>> > diff --git a/examples/fips_validation/main.c b/examples/fips_validation/main.c
>> > index 07532c956..5fb76b421 100644
>> > --- a/examples/fips_validation/main.c
>> > +++ b/examples/fips_validation/main.c
>> > @@ -15,17 +15,31 @@
>> >  #include "fips_validation.h"
>> >  #include "fips_dev_self_test.h"
>> >
>> > +enum{
>>
>> Missing space.
>>
>>
>> The _KEYWORD suffix gives no info and can be dropped in all those
>> defines / enums.
>>
>> >  #define REQ_FILE_PATH_KEYWORD  "req-file"
>> > +       /* first long only option value must be >= 256, so that we won't
>> > +        * conflict with short options
>> > +        */
>>
>> This comment is copied from the EAL header, but there is no mapping to
>> a short option in this example.
>> You can drop it.
>>
>> > +       REQ_FILE_PATH_KEYWORD_NUM = 256,
>> >  #define RSP_FILE_PATH_KEYWORD  "rsp-file"
>> > +       RSP_FILE_PATH_KEYWORD_NUM,
>> >  #define MBUF_DATAROOM_KEYWORD  "mbuf-dataroom"
>> > +       MBUF_DATAROOM_KEYWORD_NUM,
>> >  #define FOLDER_KEYWORD         "path-is-folder"
>> > +       FOLDER_KEYWORD_NUM,
>> >  #define CRYPTODEV_KEYWORD      "cryptodev"
>> > +       CRYPTODEV_KEYWORD_NUM,
>> >  #define CRYPTODEV_ID_KEYWORD   "cryptodev-id"
>> > +       CRYPTODEV_ID_KEYWORD_NUM,
>> >  #define CRYPTODEV_ST_KEYWORD   "self-test"
>> > +       CRYPTODEV_ST_KEYWORD_NUM,
>> >  #define CRYPTODEV_BK_ID_KEYWORD        "broken-test-id"
>> > +       CRYPTODEV_BK_ID_KEYWORD_NUM,
>> >  #define CRYPTODEV_BK_DIR_KEY   "broken-test-dir"
>> > +       CRYPTODEV_BK_DIR_KEY_NUM,
>>
>>
>> Those next two defines have nothing to do with getopt options and they
>> are only used once.
>> You can directly replace them as their fixed string later in the
>> parsing code, and drop the defines.
>>
>>
>> >  #define CRYPTODEV_ENC_KEYWORD  "enc"
>> >  #define CRYPTODEV_DEC_KEYWORD  "dec"
>> > +};
>> >
>> >  struct fips_test_vector vec;
>> >  struct fips_test_interim_info info;
>> > @@ -226,15 +240,24 @@ 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},
>>
>> Single indent is enough.
>>
>>
>> > +                       {REQ_FILE_PATH_KEYWORD, required_argument,
>> > +                                       NULL, REQ_FILE_PATH_KEYWORD_NUM},
>> > +                       {RSP_FILE_PATH_KEYWORD, required_argument,
>> > +                                       NULL, RSP_FILE_PATH_KEYWORD_NUM},
>> > +                       {FOLDER_KEYWORD, no_argument,
>> > +                                       NULL, FOLDER_KEYWORD_NUM},
>> > +                       {MBUF_DATAROOM_KEYWORD, required_argument,
>> > +                                       NULL, MBUF_DATAROOM_KEYWORD_NUM},
>> > +                       {CRYPTODEV_KEYWORD, required_argument,
>> > +                                       NULL, CRYPTODEV_KEYWORD_NUM},
>> > +                       {CRYPTODEV_ID_KEYWORD, required_argument,
>> > +                                       NULL, CRYPTODEV_ID_KEYWORD_NUM},
>> > +                       {CRYPTODEV_ST_KEYWORD, no_argument,
>> > +                                       NULL, CRYPTODEV_ST_KEYWORD_NUM},
>> > +                       {CRYPTODEV_BK_ID_KEYWORD, required_argument,
>> > +                                       NULL, CRYPTODEV_BK_ID_KEYWORD_NUM},
>> > +                       {CRYPTODEV_BK_DIR_KEY, required_argument,
>> > +                                       NULL, CRYPTODEV_BK_DIR_KEY_NUM},
>> >                         {NULL, 0, 0, 0}
>> >         };
>> >
>> > @@ -251,105 +274,127 @@ cryptodev_fips_validate_parse_args(int argc, char **argv)
>> >         while ((opt = getopt_long(argc, argvopt, "s:",
>> >                                   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 REQ_FILE_PATH_KEYWORD_NUM:
>> > +               {
>>
>> Unless you need a temp variable, there is no need for a block for each
>> case: statement.
>> Simply:
>>     case REQ_FILE_PATH_NUM:
>>         env.req_path = optarg;
>>         break;
>>
>> > +                       env.req_path = optarg;
>> > +                       break;
>> > +               }
>> > +               case RSP_FILE_PATH_KEYWORD_NUM:
>> > +               {
>> > +                       env.rsp_path = optarg;
>> > +                       break;
>> > +               }
>> > +               case FOLDER_KEYWORD_NUM:
>> > +               {
>> > +                       env.is_path_folder = 1;
>> > +                       break;
>> > +               }
>> > +               case CRYPTODEV_KEYWORD_NUM:
>> > +               {
>> > +                       ret = parse_cryptodev_arg(optarg);
>> > +                       if (ret < 0) {
>> > +                               cryptodev_fips_validate_usage(prgname);
>> > +                               return -EINVAL;
>> > +                       }
>> >
>> > -                               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;
>> > -                               }
>> > +                       break;
>> > +               }
>> > +               case CRYPTODEV_ID_KEYWORD_NUM:
>> > +               {
>> > +                       ret = parse_cryptodev_id_arg(optarg);
>> > +                       if (ret < 0) {
>> > +                               cryptodev_fips_validate_usage(prgname);
>> > +                               return -EINVAL;
>> > +                       }
>> > +                       break;
>> > +               }
>> > +               case CRYPTODEV_ST_KEYWORD_NUM:
>> > +               {
>> > +                       env.self_test = 1;
>> > +                       break;
>> > +               }
>> > +               case CRYPTODEV_BK_ID_KEYWORD_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 (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;
>> > -                               }
>> > +                       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 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 (data_room_size == 0 ||
>> > -                                               data_room_size > UINT16_MAX) {
>> > -                                       cryptodev_fips_validate_usage(prgname);
>> > -                                       return -EINVAL;
>> > -                               }
>> > +                       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;
>> > +                       }
>> > +                       break;
>> > +               }
>> > +               case MBUF_DATAROOM_KEYWORD_NUM:
>> > +               {
>> > +                       uint32_t data_room_size;
>>
>> Here, I don't think we need a temp storage.
>> If the value is invalid, the parsing and then init will fail.
>> You can directly pass &env.mbuf_data_room to parser_read_uint32 and
>> check its value.
>>
>>
>> >
>> > -                               env.mbuf_data_room = data_room_size;
>> > -                       } else {
>> > +                       if (parser_read_uint32(&data_room_size,
>> > +                                       optarg) < 0) {
>> >                                 cryptodev_fips_validate_usage(prgname);
>> >                                 return -EINVAL;
>> >                         }
>> > +
>> > +                       if (data_room_size == 0 ||
>> > +                                       data_room_size > UINT16_MAX) {
>> > +                               cryptodev_fips_validate_usage(prgname);
>> > +                               return -EINVAL;
>> > +                       }
>> > +
>> > +                       env.mbuf_data_room = data_room_size;
>> > +
>> >                         break;
>> > +               }
>> >                 default:
>> > -                       return -1;
>> > +               {
>> > +                       cryptodev_fips_validate_usage(prgname);
>> > +                       return -EINVAL;
>> > +               }
>> >                 }
>> >         }
>> >
>> > --
>> > 2.17.1
>> >
>>
>> I did not look too much at the rest of the series, but I guess most of
>> those comments apply to other patches.
>>
>>
>> --
>> David Marchand
>>
>
>
> --
> - Ibtisam
>


-- 
- Ibtisam

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

* Re: [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage
  2020-11-04 10:00     ` Ibtisam Tariq
@ 2020-11-05  8:59       ` David Marchand
  2020-11-10  6:10         ` Ibtisam Tariq
  0 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2020-11-05  8:59 UTC (permalink / raw)
  To: Ibtisam Tariq
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev

Hello Ibtisam,

On Wed, Nov 4, 2020 at 11:00 AM Ibtisam Tariq <ibtisam.tariq@emumba.com> wrote:
> > +               case MBUF_DATAROOM_KEYWORD_NUM:
> > +               {
> > +                       uint32_t data_room_size;
>
> Here, I don't think we need a temp storage.
> If the value is invalid, the parsing and then init will fail.
> You can directly pass &env.mbuf_data_room to parser_read_uint32 and
> check its value.
>
>
>
> >
> > -                               env.mbuf_data_room = data_room_size;
> > -                       } else {
> > +                       if (parser_read_uint32(&data_room_size,
> > +                                       optarg) < 0) {
> >                                 cryptodev_fips_validate_usage(prgname);
> >                                 return -EINVAL;
> >                         }
> > +
> > +                       if (data_room_size == 0 ||
> > +                                       data_room_size > UINT16_MAX) {
> > +                               cryptodev_fips_validate_usage(prgname);
> > +                               return -EINVAL;
> > +                       }
> > +
> > +                       env.mbuf_data_room = data_room_size;
> > +
> >                         break;
> > +               }
>
> The type of env.mbuf_data_room is uint16_t and the temp variable type
> is uint32_t. In my opinion, the temp variable size is bigger than
> env.mbuf_data_room to handle overflow value.

All of it could be moved to a read_uint16 parser.
WDYT?


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage
  2020-11-05  8:59       ` David Marchand
@ 2020-11-10  6:10         ` Ibtisam Tariq
  2020-11-10  8:23           ` David Marchand
  0 siblings, 1 reply; 17+ messages in thread
From: Ibtisam Tariq @ 2020-11-10  6:10 UTC (permalink / raw)
  To: David Marchand
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev

Hello David,

IMHO, it cannot be moved to read_uint16 parser.
If we do, we can't verify that the user input value is greater than
UINT16 MAX or not on the overflow data.
> > +                       if (data_room_size == 0 ||
> > +                                       data_room_size > UINT16_MAX) {
> > +                               cryptodev_fips_validate_usage(prgname);
> > +                               return -EINVAL;
> > +                       }

The temp variable:data_room_size is necessary to check the overflow of
the command line argument.

On Thu, Nov 5, 2020 at 1:59 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Hello Ibtisam,
>
> On Wed, Nov 4, 2020 at 11:00 AM Ibtisam Tariq <ibtisam.tariq@emumba.com> wrote:
> > > +               case MBUF_DATAROOM_KEYWORD_NUM:
> > > +               {
> > > +                       uint32_t data_room_size;
> >
> > Here, I don't think we need a temp storage.
> > If the value is invalid, the parsing and then init will fail.
> > You can directly pass &env.mbuf_data_room to parser_read_uint32 and
> > check its value.
> >
> >
> >
> > >
> > > -                               env.mbuf_data_room = data_room_size;
> > > -                       } else {
> > > +                       if (parser_read_uint32(&data_room_size,
> > > +                                       optarg) < 0) {
> > >                                 cryptodev_fips_validate_usage(prgname);
> > >                                 return -EINVAL;
> > >                         }
> > > +
> > > +                       if (data_room_size == 0 ||
> > > +                                       data_room_size > UINT16_MAX) {
> > > +                               cryptodev_fips_validate_usage(prgname);
> > > +                               return -EINVAL;
> > > +                       }
> > > +
> > > +                       env.mbuf_data_room = data_room_size;
> > > +
> > >                         break;
> > > +               }
> >
> > The type of env.mbuf_data_room is uint16_t and the temp variable type
> > is uint32_t. In my opinion, the temp variable size is bigger than
> > env.mbuf_data_room to handle overflow value.
>
> All of it could be moved to a read_uint16 parser.
> WDYT?
>
>
> --
> David Marchand
>


-- 
- Ibtisam

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

* Re: [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage
  2020-11-10  6:10         ` Ibtisam Tariq
@ 2020-11-10  8:23           ` David Marchand
  2020-11-10  9:03             ` Ibtisam Tariq
  0 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2020-11-10  8:23 UTC (permalink / raw)
  To: Ibtisam Tariq
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev

On Tue, Nov 10, 2020 at 7:10 AM Ibtisam Tariq <ibtisam.tariq@emumba.com> wrote:
> IMHO, it cannot be moved to read_uint16 parser.
> If we do, we can't verify that the user input value is greater than
> UINT16 MAX or not on the overflow data.
> > > +                       if (data_room_size == 0 ||
> > > +                                       data_room_size > UINT16_MAX) {
> > > +                               cryptodev_fips_validate_usage(prgname);
> > > +                               return -EINVAL;
> > > +                       }
>
> The temp variable:data_room_size is necessary to check the overflow of
> the command line argument.

The overflow check can go to a new read_uint16 parser, like what is
done in other parsers in this example.

int
parser_read_uint32(uint32_t *value, char *p)
{
        uint64_t val = 0;
        int ret = parser_read_uint64(&val, p);
        if (ret < 0)
                return ret;
        if (val > UINT32_MAX)
                return -EINVAL;
        *value = val;
        return 0;
}

The parser_read_uint16 caller can do any additional check, here test
for 0 value.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage
  2020-11-10  8:23           ` David Marchand
@ 2020-11-10  9:03             ` Ibtisam Tariq
  0 siblings, 0 replies; 17+ messages in thread
From: Ibtisam Tariq @ 2020-11-10  9:03 UTC (permalink / raw)
  To: David Marchand
  Cc: Kovacevic, Marko, Ananyev, Konstantin, Pattan, Reshma, Mcnamara,
	John, Cristian Dumitrescu, Singh, Jasvinder, Xia, Chenbo,
	Maxime Coquelin, Xiaoyun Li, dev

Thanks for explaining. I got it.
I will submit the patches with new updates.

On Tue, Nov 10, 2020 at 1:23 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Tue, Nov 10, 2020 at 7:10 AM Ibtisam Tariq <ibtisam.tariq@emumba.com> wrote:
> > IMHO, it cannot be moved to read_uint16 parser.
> > If we do, we can't verify that the user input value is greater than
> > UINT16 MAX or not on the overflow data.
> > > > +                       if (data_room_size == 0 ||
> > > > +                                       data_room_size > UINT16_MAX) {
> > > > +                               cryptodev_fips_validate_usage(prgname);
> > > > +                               return -EINVAL;
> > > > +                       }
> >
> > The temp variable:data_room_size is necessary to check the overflow of
> > the command line argument.
>
> The overflow check can go to a new read_uint16 parser, like what is
> done in other parsers in this example.
>
> int
> parser_read_uint32(uint32_t *value, char *p)
> {
>         uint64_t val = 0;
>         int ret = parser_read_uint64(&val, p);
>         if (ret < 0)
>                 return ret;
>         if (val > UINT32_MAX)
>                 return -EINVAL;
>         *value = val;
>         return 0;
> }
>
> The parser_read_uint16 caller can do any additional check, here test
> for 0 value.
>
>
> --
> David Marchand
>


-- 
- Ibtisam

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

end of thread, other threads:[~2020-11-10  9:03 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-29 12:53 [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage Ibtisam Tariq
2020-10-29 12:53 ` [dpdk-dev] [PATCH 2/8] examples/l3fwd-acl: " Ibtisam Tariq
2020-10-29 12:53 ` [dpdk-dev] [PATCH 3/8] examples/packet_ordering: " Ibtisam Tariq
2020-10-29 12:53 ` [dpdk-dev] [PATCH 4/8] examples/performance-thread/l3fwd-thread: " Ibtisam Tariq
2020-10-29 12:53 ` [dpdk-dev] [PATCH 5/8] examples/qos_sched: " Ibtisam Tariq
2020-10-29 12:53 ` [dpdk-dev] [PATCH 6/8] examples/vhost: " Ibtisam Tariq
2020-10-29 12:53 ` [dpdk-dev] [PATCH 7/8] examples/vhost_crypto: " Ibtisam Tariq
2020-10-29 12:53 ` [dpdk-dev] [PATCH 8/8] examples/tep_termination: " Ibtisam Tariq
2020-10-29 13:16   ` David Marchand
2020-11-02  8:18     ` Ibtisam Tariq
2020-10-29 22:07 ` [dpdk-dev] [PATCH 1/8] examples/fips_validation: " David Marchand
2020-11-02  8:32   ` Ibtisam Tariq
2020-11-04 10:00     ` Ibtisam Tariq
2020-11-05  8:59       ` David Marchand
2020-11-10  6:10         ` Ibtisam Tariq
2020-11-10  8:23           ` David Marchand
2020-11-10  9:03             ` Ibtisam Tariq

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git