Re: [dpdk-dev] [PATCH v5 3/5] examples/l3fwd: add FIB infrastructure

["Walsh, Conor" <conor.walsh@intel.com>]

<snip>
> 
> > @@ -310,7 +328,10 @@ print_usage(const char *prgname)
> >   		"                  Valid only if --mode=eventdev\n"
> >   		"  --event-eth-rxqs: Number of ethernet RX queues per
> device.\n"
> >   		"                    Default: 1\n"
> > -		"                    Valid only if --mode=eventdev\n\n",
> > +		"                    Valid only if --mode=eventdev\n"
> > +		"  --lookup: Select the lookup method\n"
> > +		"            Default: lpm\n"
> > +		"            Accepted: em (Exact Match), lpm (Longest Prefix
> Match), fib (First Information Base)\n\n",
> 
> Isn't it Forward Information Base?

Hi Anatoly,
It's Forwarding Information Base, I will correct this.

> 
> >   		prgname);
> >   }
> >
> > @@ -485,13 +506,32 @@ parse_event_eth_rx_queues(const char
> *eth_rx_queues)
> >   	evt_rsrc->eth_rx_queues = num_eth_rx_queues;
> >   }
> >
> > +static void
> > +parse_lookup(const char *optarg)
> > +{
> > +	if (lookup_mode != L3FWD_LOOKUP_DEFAULT) {
> > +		rte_exit(EXIT_FAILURE,
> > +				"Only one lookup mode is allowed at a
> time!\n");
> > +	}
> > +	if (!strcmp(optarg, "em"))
> > +		lookup_mode = L3FWD_LOOKUP_EM;
> > +	else if (!strcmp(optarg, "lpm"))
> > +		lookup_mode = L3FWD_LOOKUP_LPM;
> > +	else if (!strcmp(optarg, "fib"))
> > +		lookup_mode = L3FWD_LOOKUP_FIB;
> > +	else {
> > +		rte_exit(EXIT_FAILURE,
> > +				"Invalid --lookup option! Accepted options:
> em, lpm, fib\n");
> > +	}
> > +}
> > +
> 
> I don't think having rte_exit() calls inside a parsing function is good
> practice. The check at the beginning of the function can be done in
> optarg switch (like you have for E and L switches), while the latter can
> be replaced with a RTE_LOG(ERR, ...) and a return -1, which can be
> checked by the caller.

I will modify the patch in line with this comment too.

> 
> Once the above is fixed,
> 
> Acked-by: Anatoly Burako <anatoly.burakov@intel.com>

Thanks for your feedback and ack I will make the changes and push a v6 of the patchset.
Conor.

> 
> --
> Thanks,
> Anatoly