On Thu, 18 Jul 2019 09:47:03 +0200
Olivier Matz <olivier.m...@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 <step...@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.
