DPDK patches and discussions
 help / color / mirror / Atom feed
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
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

  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

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