From: Ibtisam Tariq <ibtisam.tariq@emumba.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Cc: "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>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: enhance getopt_long usage
Date: Tue, 17 Nov 2020 12:52:03 +0500 [thread overview]
Message-ID: <CA+8bGBugu75F56=eprWSnfYqbSLnV6vPNRq=smo61-sGweX13Q@mail.gmail.com> (raw)
In-Reply-To: <DM6PR11MB33087F0BC5BEF4426C74C27E9AE60@DM6PR11MB3308.namprd11.prod.outlook.com>
Hi Ananyev,
Thank you.
Mix enum with corresponding defines for string literals, is the
convention of eal_options.h.
You can have a look at this
https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/eal_options.h.
On Fri, Nov 13, 2020 at 5:27 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
>
> Hi,
>
> >
> > Instead of using getopt_long return value, strcmp was used to
> > compare the input parameters with the struct option array. This
> > patch get rid of all those strcmp by directly binding each longopt
> > with an int enum. This is to improve readability and consistency in
> > all examples.
> >
> > Bugzilla ID: 238
> > Cc: konstantin.ananyev@intel.com
> >
> > Reported-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Ibtisam Tariq <ibtisam.tariq@emumba.com>
> > ---
> > v2
> > * Remove extra indentations.
> > * Remove extra block brackets in switch statement.
> > * Change enum names to start with OPT_ and remove KEYWORD from enum names.
> >
> > v1
> > * enhance getopt_long usage
> > ---
> > examples/l3fwd-acl/main.c | 219 +++++++++++++++++++-------------------
> > 1 file changed, 110 insertions(+), 109 deletions(-)
> >
> > diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c
> > index 961594f5f..5725963ad 100644
> > --- a/examples/l3fwd-acl/main.c
> > +++ b/examples/l3fwd-acl/main.c
> > @@ -195,13 +195,24 @@ send_single_packet(struct rte_mbuf *m, uint16_t port);
> > #define ACL_LEAD_CHAR ('@')
> > #define ROUTE_LEAD_CHAR ('R')
> > #define COMMENT_LEAD_CHAR ('#')
> > -#define OPTION_CONFIG "config"
> > -#define OPTION_NONUMA "no-numa"
> > -#define OPTION_ENBJMO "enable-jumbo"
> > -#define OPTION_RULE_IPV4 "rule_ipv4"
> > -#define OPTION_RULE_IPV6 "rule_ipv6"
> > -#define OPTION_ALG "alg"
> > -#define OPTION_ETH_DEST "eth-dest"
> > +
> > +enum {
> > +#define OPT_CONFIG "config"
> > + OPT_CONFIG_NUM = 256,
>
> It is probably better not to mix enum values with corresponding defines
> for string literals. Looks really weird and hard to read (at least to me).
> Apart from that - LGTM.
>
> > +#define OPT_NONUMA "no-numa"
> > + OPT_NONUMA_NUM,
> > +#define OPT_ENBJMO "enable-jumbo"
> > + OPT_ENBJMO_NUM,
> > +#define OPT_RULE_IPV4 "rule_ipv4"
> > + OPT_RULE_IPV4_NUM,
> > +#define OPT_RULE_IPV6 "rule_ipv6"
> > + OPT_RULE_IPV6_NUM,
> > +#define OPT_ALG "alg"
> > + OPT_ALG_NUM,
> > +#define OPT_ETH_DEST "eth-dest"
> > + OPT_ETH_DEST_NUM,
> > +};
> > +
> > #define ACL_DENY_SIGNATURE 0xf0000000
> > #define RTE_LOGTYPE_L3FWDACL RTE_LOGTYPE_USER3
> > #define acl_log(format, ...) RTE_LOG(ERR, L3FWDACL, format, ##__VA_ARGS__)
> > @@ -1177,9 +1188,9 @@ static void
> > dump_acl_config(void)
> > {
> > printf("ACL option are:\n");
> > - printf(OPTION_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
> > - printf(OPTION_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
> > - printf(OPTION_ALG": %s\n", str_acl_alg(parm_config.alg));
> > + printf(OPT_RULE_IPV4": %s\n", parm_config.rule_ipv4_name);
> > + printf(OPT_RULE_IPV6": %s\n", parm_config.rule_ipv6_name);
> > + printf(OPT_ALG": %s\n", str_acl_alg(parm_config.alg));
> > }
> >
> > static int
> > @@ -1608,27 +1619,27 @@ print_usage(const char *prgname)
> >
> > usage_acl_alg(alg, sizeof(alg));
> > printf("%s [EAL options] -- -p PORTMASK -P"
> > - "--"OPTION_RULE_IPV4"=FILE"
> > - "--"OPTION_RULE_IPV6"=FILE"
> > - " [--"OPTION_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
> > - " [--"OPTION_ENBJMO" [--max-pkt-len PKTLEN]]\n"
> > + "--"OPT_RULE_IPV4"=FILE"
> > + "--"OPT_RULE_IPV6"=FILE"
> > + " [--"OPT_CONFIG" (port,queue,lcore)[,(port,queue,lcore]]"
> > + " [--"OPT_ENBJMO" [--max-pkt-len PKTLEN]]\n"
> > " -p PORTMASK: hexadecimal bitmask of ports to configure\n"
> > " -P : enable promiscuous mode\n"
> > - " --"OPTION_CONFIG": (port,queue,lcore): "
> > + " --"OPT_CONFIG": (port,queue,lcore): "
> > "rx queues configuration\n"
> > - " --"OPTION_NONUMA": optional, disable numa awareness\n"
> > - " --"OPTION_ENBJMO": enable jumbo frame"
> > + " --"OPT_NONUMA": optional, disable numa awareness\n"
> > + " --"OPT_ENBJMO": enable jumbo frame"
> > " which max packet len is PKTLEN in decimal (64-9600)\n"
> > - " --"OPTION_RULE_IPV4"=FILE: specify the ipv4 rules entries "
> > + " --"OPT_RULE_IPV4"=FILE: specify the ipv4 rules entries "
> > "file. "
> > "Each rule occupy one line. "
> > "2 kinds of rules are supported. "
> > "One is ACL entry at while line leads with character '%c', "
> > "another is route entry at while line leads with "
> > "character '%c'.\n"
> > - " --"OPTION_RULE_IPV6"=FILE: specify the ipv6 rules "
> > + " --"OPT_RULE_IPV6"=FILE: specify the ipv6 rules "
> > "entries file.\n"
> > - " --"OPTION_ALG": ACL classify method to use, one of: %s\n",
> > + " --"OPT_ALG": ACL classify method to use, one of: %s\n",
> > prgname, ACL_LEAD_CHAR, ROUTE_LEAD_CHAR, alg);
> > }
> >
> > @@ -1747,14 +1758,14 @@ parse_args(int argc, char **argv)
> > int option_index;
> > char *prgname = argv[0];
> > static struct option lgopts[] = {
> > - {OPTION_CONFIG, 1, 0, 0},
> > - {OPTION_NONUMA, 0, 0, 0},
> > - {OPTION_ENBJMO, 0, 0, 0},
> > - {OPTION_RULE_IPV4, 1, 0, 0},
> > - {OPTION_RULE_IPV6, 1, 0, 0},
> > - {OPTION_ALG, 1, 0, 0},
> > - {OPTION_ETH_DEST, 1, 0, 0},
> > - {NULL, 0, 0, 0}
> > + {OPT_CONFIG, 1, NULL, OPT_CONFIG_NUM },
> > + {OPT_NONUMA, 0, NULL, OPT_NONUMA_NUM },
> > + {OPT_ENBJMO, 0, NULL, OPT_ENBJMO_NUM },
> > + {OPT_RULE_IPV4, 1, NULL, OPT_RULE_IPV4_NUM },
> > + {OPT_RULE_IPV6, 1, NULL, OPT_RULE_IPV6_NUM },
> > + {OPT_ALG, 1, NULL, OPT_ALG_NUM },
> > + {OPT_ETH_DEST, 1, NULL, OPT_ETH_DEST_NUM },
> > + {NULL, 0, 0, 0 }
> > };
> >
> > argvopt = argv;
> > @@ -1772,104 +1783,94 @@ parse_args(int argc, char **argv)
> > return -1;
> > }
> > break;
> > +
> > case 'P':
> > printf("Promiscuous mode selected\n");
> > promiscuous_on = 1;
> > break;
> >
> > /* long options */
> > - case 0:
> > - if (!strncmp(lgopts[option_index].name,
> > - OPTION_CONFIG,
> > - sizeof(OPTION_CONFIG))) {
> > - ret = parse_config(optarg);
> > - if (ret) {
> > - printf("invalid config\n");
> > - print_usage(prgname);
> > - return -1;
> > - }
> > - }
> > -
> > - if (!strncmp(lgopts[option_index].name,
> > - OPTION_NONUMA,
> > - sizeof(OPTION_NONUMA))) {
> > - printf("numa is disabled\n");
> > - numa_on = 0;
> > - }
> > -
> > - if (!strncmp(lgopts[option_index].name,
> > - OPTION_ENBJMO, sizeof(OPTION_ENBJMO))) {
> > - struct option lenopts = {
> > - "max-pkt-len",
> > - required_argument,
> > - 0,
> > - 0
> > - };
> > -
> > - printf("jumbo frame is enabled\n");
> > - port_conf.rxmode.offloads |=
> > - DEV_RX_OFFLOAD_JUMBO_FRAME;
> > - port_conf.txmode.offloads |=
> > - DEV_TX_OFFLOAD_MULTI_SEGS;
> > -
> > - /*
> > - * if no max-pkt-len set, then use the
> > - * default value RTE_ETHER_MAX_LEN
> > - */
> > - if (0 == getopt_long(argc, argvopt, "",
> > - &lenopts, &option_index)) {
> > - ret = parse_max_pkt_len(optarg);
> > - if ((ret < 64) ||
> > - (ret > MAX_JUMBO_PKT_LEN)) {
> > - printf("invalid packet "
> > - "length\n");
> > - print_usage(prgname);
> > - return -1;
> > - }
> > - port_conf.rxmode.max_rx_pkt_len = ret;
> > - }
> > - printf("set jumbo frame max packet length "
> > - "to %u\n",
> > - (unsigned int)
> > - port_conf.rxmode.max_rx_pkt_len);
> > + case OPT_CONFIG_NUM:
> > + ret = parse_config(optarg);
> > + if (ret) {
> > + printf("invalid config\n");
> > + print_usage(prgname);
> > + return -1;
> > }
> > + break;
> >
> > - if (!strncmp(lgopts[option_index].name,
> > - OPTION_RULE_IPV4,
> > - sizeof(OPTION_RULE_IPV4)))
> > - parm_config.rule_ipv4_name = optarg;
> > -
> > - if (!strncmp(lgopts[option_index].name,
> > - OPTION_RULE_IPV6,
> > - sizeof(OPTION_RULE_IPV6))) {
> > - parm_config.rule_ipv6_name = optarg;
> > - }
> > + case OPT_NONUMA_NUM:
> > + printf("numa is disabled\n");
> > + numa_on = 0;
> > + break;
> >
> > - if (!strncmp(lgopts[option_index].name,
> > - OPTION_ALG, sizeof(OPTION_ALG))) {
> > - parm_config.alg = parse_acl_alg(optarg);
> > - if (parm_config.alg ==
> > - RTE_ACL_CLASSIFY_DEFAULT) {
> > - printf("unknown %s value:\"%s\"\n",
> > - OPTION_ALG, optarg);
> > + case OPT_ENBJMO_NUM:
> > + {
> > + struct option lenopts = {
> > + "max-pkt-len",
> > + required_argument,
> > + 0,
> > + 0
> > + };
> > +
> > + printf("jumbo frame is enabled\n");
> > + port_conf.rxmode.offloads |=
> > + DEV_RX_OFFLOAD_JUMBO_FRAME;
> > + port_conf.txmode.offloads |=
> > + DEV_TX_OFFLOAD_MULTI_SEGS;
> > +
> > + /*
> > + * if no max-pkt-len set, then use the
> > + * default value RTE_ETHER_MAX_LEN
> > + */
> > + if (0 == getopt_long(argc, argvopt, "",
> > + &lenopts, &option_index)) {
> > + ret = parse_max_pkt_len(optarg);
> > + if ((ret < 64) ||
> > + (ret > MAX_JUMBO_PKT_LEN)) {
> > + printf("invalid packet "
> > + "length\n");
> > print_usage(prgname);
> > return -1;
> > }
> > + port_conf.rxmode.max_rx_pkt_len = ret;
> > }
> > + printf("set jumbo frame max packet length "
> > + "to %u\n",
> > + (unsigned int)
> > + port_conf.rxmode.max_rx_pkt_len);
> > + break;
> > + }
> > + case OPT_RULE_IPV4_NUM:
> > + parm_config.rule_ipv4_name = optarg;
> > + break;
> >
> > - if (!strncmp(lgopts[option_index].name, OPTION_ETH_DEST,
> > - sizeof(OPTION_ETH_DEST))) {
> > - const char *serr = parse_eth_dest(optarg);
> > - if (serr != NULL) {
> > - printf("invalid %s value:\"%s\": %s\n",
> > - OPTION_ETH_DEST, optarg, serr);
> > - print_usage(prgname);
> > - return -1;
> > - }
> > - }
> > + case OPT_RULE_IPV6_NUM:
> > + parm_config.rule_ipv6_name = optarg;
> > + break;
> >
> > + case OPT_ALG_NUM:
> > + parm_config.alg = parse_acl_alg(optarg);
> > + if (parm_config.alg ==
> > + RTE_ACL_CLASSIFY_DEFAULT) {
> > + printf("unknown %s value:\"%s\"\n",
> > + OPT_ALG, optarg);
> > + print_usage(prgname);
> > + return -1;
> > + }
> > break;
> >
> > + case OPT_ETH_DEST_NUM:
> > + {
> > + const char *serr = parse_eth_dest(optarg);
> > + if (serr != NULL) {
> > + printf("invalid %s value:\"%s\": %s\n",
> > + OPT_ETH_DEST, optarg, serr);
> > + print_usage(prgname);
> > + return -1;
> > + }
> > + break;
> > + }
> > default:
> > print_usage(prgname);
> > return -1;
> > --
> > 2.17.1
>
--
- Ibtisam
next prev parent reply other threads:[~2020-11-17 7:52 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
2020-11-17 7:52 ` Ibtisam Tariq [this message]
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='CA+8bGBugu75F56=eprWSnfYqbSLnV6vPNRq=smo61-sGweX13Q@mail.gmail.com' \
--to=ibtisam.tariq@emumba.com \
--cc=chenbo.xia@intel.com \
--cc=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
--cc=jasvinder.singh@intel.com \
--cc=john.mcnamara@intel.com \
--cc=konstantin.ananyev@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).