Re: [dpdk-dev] [RFC] net: be more restrictive in ether_unformat_addr

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 18 Jul 2019 09:47:03 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:

> Hi,
> 
> I'm fine with a more strict version like you proposed here. I checked
> that the cmdline tests pass.
> 
> Few minor comments below.
> 
> On Wed, Jul 17, 2019 at 11:49:45AM -0700, Stephen Hemminger wrote:
> > The current code acts more like BSD ether_aton and allows leading zeros
> > which breaks the cmdline tests.
> > 
> > Change the code to be more restrictive and only allow the fully
> > expanded standard formats.
> > 
> > Fixes: 596d31092d32 ("net: add function to convert string to ethernet address")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> Can you add the bugzilla id ?
> https://bugs.dpdk.org/show_bug.cgi?id=324

Sure, will do.
I wish there was a one week delay during development and reserve Bugzilla
for stuff in released code.

> > +	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
> > +		int8_t x;
> > +
> > +		x = get_xdigit(*s++);
> > +		if (x < 0)
> > +			return false;
> > +		ea->addr_bytes[i] = x << 4;
> > +		x = get_xdigit(*s++);
> > +		if (x < 0)
> > +			return false;
> > +		ea->addr_bytes[i] |= x;  
> 
> Maybe we should say in the API doc that ether address can be modified
> even if parsing fails.
> 


No. Standard practice is that the state of returned data is undefined in any 
function that returns an error.

> > -	} else {
> > -		/* unknown format */
> > -		rte_errno = EINVAL;
> > +	if  (get_ether_addr6(s, ea) || get_ether_addr3(s, ea))
> > +		return 0;
> > +	else {
> > +		rte_errno = -EINVAL;
> >  		return -1;  
> 
> rte_errno should be positive

Will fix.