From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B4C11A04DB; Tue, 17 Nov 2020 08:52:21 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CBE8237B0; Tue, 17 Nov 2020 08:52:19 +0100 (CET) Received: from mail-lf1-f68.google.com (mail-lf1-f68.google.com [209.85.167.68]) by dpdk.org (Postfix) with ESMTP id 1F285F3E for ; Tue, 17 Nov 2020 08:52:17 +0100 (CET) Received: by mail-lf1-f68.google.com with SMTP id u18so28780240lfd.9 for ; Mon, 16 Nov 2020 23:52:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=emumba-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=gPmlWXEVRZbsqabWE5EnjuHsY6yntVAVP3jlHTqrOa8=; b=jFeTDJkQ+At3BpAotBIqTzr7JDA4fRWOSw03qA27SB6wlLp6OY4gE8pQW3vwQIcc1g 5z2BZURSTl4MtFpc9jrbynuEM+BBDh1/HINCuzCmsYh5Fu+kLZKeq/rxl4YyD8BcMH43 cLrJzZ5V1evSYnPBOcge20TclJN4acfQFl0avG272AHqTA7fPhA+t+fTsc+Pfs1c+SBR jGQzEkRm+r/ZBV+dBH41O0EknizpMB+z+iBu5Pjkp/beblcEVwdaEtzHC+HcnNrGSwSI kUkKNv1XXLQBUBYrWgtCRc8P3cq8Dw7nGSvkgLzjaFxP49cEDGkHZ//SnoqdBXFbYRXL /YAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gPmlWXEVRZbsqabWE5EnjuHsY6yntVAVP3jlHTqrOa8=; b=cJOT3X48TcfWwEbK9oztkQ9MPaI4LGWLlt2NoJv07pAlVC9S/hBC6EM+/IO56HQFDm +4tzMGlujfI0WmIEKwpiTs6dWdnrUbSbe5RFK1Wkk0S70gTJ/Ll/hRn1c3cwXP/cVR57 Qy9kA8ANg2Mm8JtUt4xXDi/h8mm3NcEKpTsKsUCwZ/HUAHnQ5XkjCfc9oDNzqq1/AgRY dCF61HWcsHOOZhTiv2W1nNR8x8eZmYPysHQKWGSs2FvKszLYMM7haeg4JY4qo1fK/A72 d2p15dTd/YeKZ3eJIY3M6aIU2fnOQJGjTK3fEZwaR8X3mAR2bvjn0/KVugBc02OgyPGK r8Bg== X-Gm-Message-State: AOAM531SPbEeV0w683s4I2kB4mpReeUd76cMzFWik1yChtafbLCu/B67 5rUZtZ5EG8eIue3NuO8Kb147nAkQqPtCKH/1H/mV2Q== X-Google-Smtp-Source: ABdhPJzyQc/Djl2OSgpBFD5eEGdlYKpjsmPoh2KQ4NDXi5iipudoD7qaoDsrQu7JpQZtd8b5OdPnlrIR5rUfHH6O2Bo= X-Received: by 2002:ac2:4d46:: with SMTP id 6mr1427408lfp.204.1605599535571; Mon, 16 Nov 2020 23:52:15 -0800 (PST) MIME-Version: 1.0 References: <20201111081507.19913-1-ibtisam.tariq@emumba.com> <20201111081507.19913-2-ibtisam.tariq@emumba.com> In-Reply-To: From: Ibtisam Tariq Date: Tue, 17 Nov 2020 12:52:03 +0500 Message-ID: To: "Ananyev, Konstantin" Cc: "maxime.coquelin@redhat.com" , "Xia, Chenbo" , "Dumitrescu, Cristian" , "Singh, Jasvinder" , "Mcnamara, John" , "Pattan, Reshma" , "Kovacevic, Marko" , "dev@dpdk.org" Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH 2/7] examples/l3fwd-acl: enhance getopt_long usage X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Ananyev, Thank you. Mix enum with corresponding defines for string literals, is the convention of eal_options.h. You can have a look at this https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/eal_options.h. On Fri, Nov 13, 2020 at 5:27 PM Ananyev, Konstantin 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 > > Signed-off-by: Ibtisam Tariq > > --- > > 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