Stephen Hemminger <step...@networkplumber.org> writes:
> On Tue, 16 Jul 2019 01:17:56 +0000
> "Li, WenjieX A" <wenjiex.a...@intel.com> wrote:
>
>> Hi Stephen,
>>
>>
>>
>> This DPDK patch makes cmdline_autotest failed. List the details as below.
>>
>> Could you please help to resolve it?
>>
>> Thank you!
>> Test Setup
>> Steps to reproduce
>> List the steps to reproduce the issue.
>> ./x86_64-native-linuxapp-gcc/app/test -n 1 -c 0xff
>> testpmd> cmdline_autotest
>> Show the output from the previous commands.
>> Testind parsing ethernet addresses...
>> Error: parsing 01:23:45:67:89:A succeeded!
>> Test Failed
>> Expected Result
>> Explain what is the expected result in text or as an example output:
>> Testind parsing ethernet addresses...
>> Testind parsing port lists...
>> Testind parsing numbers...
>> Testing parsing IP addresses...
>> Testing parsing strings...
>> Testing circular buffer...
>> Testing library functions...
>> Test OK
>>
>>
>>
>> BR,
>>
>> Wenjie
>>
>
> There are two possible solutions. Make the ether_unformat_addr function
> more complex and more restrictive. The new version corresponds to the FreeBSD
> behavior.
Not exactly. The new code accepts 2 forms (X:X:X:X:X:X which is
FreeBSD, as well as X:X:X which is not). Because of this, there is a
higher likelihood of ambiguity. I want to submit a patch to fix this
ambiguity (but got pulled into a customer bug). Maybe someone else can?
I think it's best to make the code slightly more complex. I originally
submitted a change just validating the string length[0], which is probably
too restrictive.
Maybe a better version would be something like the original (rewritten
to ensure it supports things like XX:X:XX:X:XX:X format, also):
{
unsigned long o[ETHER_ADDR_LEN];
char *end, *input = buf;
size_t i = 0;
do {
errno = 0;
o[i] = strtoul(input, &end, 16);
if (errno != 0 || end == input || (end && *end != ':' && *end != 0))
return ERROR;
input = end+1;
} while((input - buf < buflen) && ++i < ARRAY_SIZE(o) && *end != 0);
if (*end != 0)
return ERROR;
if (i == 6) /*x:x:x:x:x:x*/
else if (i == 3) /*x:x:x*/
else return ERROR
}
WDYT?
> The other possibility is just remove the test case.
I don't like this approach - right now its possible that a user (doing
c/p) can submit an ipv6 address which will be accepted as valid. I
prefer to be validating the form. Even if XX:X:X:XX:X:X is okay,
XXXX:XXXX:XX::X shouldn't be. There's a limit to how liberal we should
be when accepting input.
> I am leaning towards the latter as the least work.
Least work isn't always the right thing.
0: http://mails.dpdk.org/archives/dev/2019-July/137827.html
