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

[Olivier Matz <olivier.matz@6wind.com>]

On Wed, Jul 17, 2019 at 10:04:10AM -0400, Aaron Conole wrote:
> 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 will have a look at it.


> 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