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.anan...@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.anan...@intel.com
> >
> > Reported-by: David Marchand <david.march...@redhat.com>
> > Signed-off-by: Ibtisam Tariq <ibtisam.ta...@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
