DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Ibtisam Tariq <ibtisam.tariq@emumba.com>,
	"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
	"Xia, Chenbo" <chenbo.xia@intel.com>,
	"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>,
	"Singh, Jasvinder" <jasvinder.singh@intel.com>,
	"Mcnamara, John" <john.mcnamara@intel.com>,
	"Pattan, Reshma" <reshma.pattan@intel.com>,
	"Kovacevic, Marko" <marko.kovacevic@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: enhance getopt_long usage
Date: Fri, 13 Nov 2020 12:26:51 +0000
Message-ID: <DM6PR11MB33087F0BC5BEF4426C74C27E9AE60@DM6PR11MB3308.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201111081507.19913-2-ibtisam.tariq@emumba.com>

Hi, 

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

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

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


  reply	other threads:[~2020-11-13 12:29 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-11  8:15 [dpdk-dev] [PATCH 1/7] examples/fips_validation: " Ibtisam Tariq
2020-11-11  8:15 ` [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: " Ibtisam Tariq
2020-11-13 12:26   ` Ananyev, Konstantin [this message]
2020-11-17  7:52     ` Ibtisam Tariq
2020-11-11  8:15 ` [dpdk-dev] [PATCH 3/7] examples/packet_ordering: " Ibtisam Tariq
2020-11-11  8:15 ` [dpdk-dev] [PATCH 4/7] examples/performance-thread/l3fwd-thread: " Ibtisam Tariq
2020-11-11  8:15 ` [dpdk-dev] [PATCH 5/7] examples/qos_sched: " Ibtisam Tariq
2020-11-14 11:12   ` David Marchand
2020-11-17  7:42     ` Ibtisam Tariq
2020-11-11  8:15 ` [dpdk-dev] [PATCH 6/7] examples/vhost: " Ibtisam Tariq
2020-11-11  8:15 ` [dpdk-dev] [PATCH 7/7] examples/vhost_crypto: " Ibtisam Tariq
2020-11-24 12:32 [dpdk-dev] [PATCH 1/7] examples/fips_validation: " Ibtisam Tariq
2020-11-24 12:32 ` [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: " Ibtisam Tariq

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM6PR11MB33087F0BC5BEF4426C74C27E9AE60@DM6PR11MB3308.namprd11.prod.outlook.com \
    --to=konstantin.ananyev@intel.com \
    --cc=chenbo.xia@intel.com \
    --cc=cristian.dumitrescu@intel.com \
    --cc=dev@dpdk.org \
    --cc=ibtisam.tariq@emumba.com \
    --cc=jasvinder.singh@intel.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=reshma.pattan@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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