Re: [dpdk-dev] [PATCH v5 6/8] cmdline: use new ethernet address parser

[Aaron Conole <aconole@redhat.com>]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Tue, 16 Jul 2019 01:17:56 +0000
> "Li, WenjieX A" <wenjiex.a.li@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