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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 15-Mar-21 11:34 AM, Conor Walsh wrote:
> The purpose of this commit is to add the necessary function calls
> and supporting infrastructure to allow the Forwarding Information Base
> (FIB) library to be integrated into the l3fwd sample app.
> Instead of adding an individual flag for FIB, a new flag '--lookup' has
> been added that allows the user to select their desired lookup method.
> The flags '-E' and '-L' have been retained for backwards compatibility.
> 
> Signed-off-by: Conor Walsh <conor.walsh@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Acked-by: Vladimir Medvedkin <vladimir.medvedkin@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?

>   		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.

Once the above is fixed,

Acked-by: Anatoly Burako <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly