Re: [dpdk-dev] [PATCH v2] app/testpmd: report invalid command line parameter

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/19/2019 7:18 PM, Pallavi Kadam wrote:
> 
> On 11/18/2019 8:18 AM, Ferruh Yigit wrote:
>> On 11/18/2019 3:37 PM, David Marchand wrote:
>>> We currently do not check that a non option string has been passed to
>>> testpmd.
>>>
>>> Example:
>>> $ ./master/app/testpmd --no-huge -m 512 --vdev net_null0 \
>>> 	--vdev net_null1 -- -i nb-cores=2 --total-num-mbuf 2048
>>> [...]
>>> testpmd> show config fwd
>>> io packet forwarding - ports=2 - cores=1 - streams=2 - NUMA support
>>> enabled, MP allocation mode: native
>>> Logical Core 1 (socket 0) forwards packets on 2 streams:
>>>   RX P=0/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:01
>>>   RX P=1/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00
>>>
>>> Here nb-cores=2 is just ignored, while the (probably sleepy) user did not
>>> notice this.
>>>
>>> Validate that all strings passed to testpmd are part of a known option.
>>>
>>> After this patch:
>>> $ ./master/app/testpmd --no-huge -m 512 --vdev net_null0 \
>>> 	--vdev net_null1 -- -i nb-cores=2 --total-num-mbuf 2048
>>> [...]
>>> Invalid parameter: nb-cores=2
>>> EAL: Error - exiting with code: 1
>>>   Cause: Command line incorrect
>>>
>>> While at it, when passing an unknown option, print the string that gets
>>> refused by getopt_long to help the user.
>>>
>>> Fixes: af75078fece3 ("first public release")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>> ---
>>> This seems a bit dangerous to take this kind of change this late.
>>> Some "working fine" scripts might now report failures from testpmd because
>>> of garbage in the command line.
>>>
>>> Sending the patch anyway to see what others think about it.
>>>
>>> Changelog since v1:
>>> - fixed example in commitlog,
>>>
>>> ---
>>>  app/test-pmd/parameters.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>>> index deca7a6828..2e7a504415 100644
>>> --- a/app/test-pmd/parameters.c
>>> +++ b/app/test-pmd/parameters.c
>>> @@ -1363,12 +1363,19 @@ launch_args_parse(int argc, char** argv)
>>>  			break;
>>>  		default:
>>>  			usage(argv[0]);
>>> +			printf("Invalid option: %s\n", argv[optind]);
>>>  			rte_exit(EXIT_FAILURE,
>>>  				 "Command line is incomplete or incorrect\n");
>>>  			break;
>>>  		}
>>>  	}
>>>  
>>> +	if (optind != argc) {
>> I hope 'optind' works as expected [1] in Windows too (Anatoly verified the
>> FreeBSD one).
>>
>> @Pallavi, @Ranjit, Can you please confirm it?
>>
>> [1]
>> https://linux.die.net/man/3/optind
>> "
>> If there are no more option characters, getopt() returns -1. Then optind is the
>> index in argv of the first argv-element that is not an option.
>> "
> 
> Verified on Windows. So far this change does not affect Windows code.
> Also, Windows does not support testpmd app yet.
> 

Applied to dpdk-next-net/master, thanks.