[v5,6/8] cmdline: use new ethernet address parser

  Now that there is a version of ether_aton in rte_ether, it can
be used by the cmdline ethernet address parser.

Note: ether_aton_r can not be used in cmdline because
the old code would accept either bytes XX:XX:XX:XX:XX:XX
or words XXXX:XXXX:XXXX and we need to keep compatiablity.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/Makefile                                 |  2 +-
 lib/librte_cmdline/Makefile                  |  3 +-
 lib/librte_cmdline/cmdline_parse_etheraddr.c | 71 +++-----------------
 lib/librte_cmdline/meson.build               |  6 +-
 lib/meson.build                              |  2 +-
 5 files changed, 15 insertions(+), 69 deletions(-)
  

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.

The other possibility is just remove the test case.

I am leaning towards the latter as the least work.
  

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
  

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