Re: [dpdk-dev] [PATCH v1 2/3] net/hyperv: implement core functionality

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Mon, Dec 18, 2017 at 10:26:29AM -0800, Stephen Hemminger wrote:
> On Mon, 18 Dec 2017 17:46:23 +0100
> Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> 
> > +static int
> > +ether_addr_from_str(struct ether_addr *eth_addr, const char *str)
> > +{
> > +	static const uint8_t conv[0x100] = {
> > +		['0'] = 0x80, ['1'] = 0x81, ['2'] = 0x82, ['3'] = 0x83,
> > +		['4'] = 0x84, ['5'] = 0x85, ['6'] = 0x86, ['7'] = 0x87,
> > +		['8'] = 0x88, ['9'] = 0x89, ['a'] = 0x8a, ['b'] = 0x8b,
> > +		['c'] = 0x8c, ['d'] = 0x8d, ['e'] = 0x8e, ['f'] = 0x8f,
> > +		['A'] = 0x8a, ['B'] = 0x8b, ['C'] = 0x8c, ['D'] = 0x8d,
> > +		['E'] = 0x8e, ['F'] = 0x8f, [':'] = 0x40, ['-'] = 0x40,
> > +		['\0'] = 0x60,
> > +	};
> > +	uint64_t addr = 0;
> > +	uint64_t buf = 0;
> > +	unsigned int i = 0;
> > +	unsigned int n = 0;
> > +	uint8_t tmp;
> > +
> > +	do {
> > +		tmp = conv[(int)*(str++)];
> > +		if (!tmp)
> > +			return -EINVAL;
> > +		if (tmp & 0x40) {
> > +			i += (i & 1) + (!i << 1);
> > +			addr = (addr << (i << 2)) | buf;
> > +			n += i;
> > +			buf = 0;
> > +			i = 0;
> > +		} else {
> > +			buf = (buf << 4) | (tmp & 0xf);
> > +			++i;
> > +		}
> > +	} while (!(tmp & 0x20));
> > +	if (n > 12)
> > +		return -EINVAL;
> > +	i = RTE_DIM(eth_addr->addr_bytes);
> > +	while (i) {
> > +		eth_addr->addr_bytes[--i] = addr & 0xff;
> > +		addr >>= 8;
> > +	}
> > +	return 0;
> > +}
> > +
> 
> 
> Why not ether_ntoa?

Good question. For the following reasons:

- I forgot about the existence of ether_ntoa() and didn't look it up seeing
  struct ether_addr is (re-)defined by rte_ether.h. What happens when one
  includes netinet/ether.h together with that file results in various
  conflicts that trigger a compilation error. This problem should be
  addressed first.

- ether_ntoa() returns a static buffer and is not reentrant, ether_ntoa_r()
  is but as a GNU extension, I'm not sure it exists on other OSes. Even if
  this driver is currently targeted at Linux, this is likely not the case
  for other DPDK code relying on rte_ether.h.

- I had ether_addr_from_str()'s code already ready and lying around for a
  future update in testpmd's flow command parser. No other MAC-48 conversion
  function I know of is as flexible as this version. The ability to omit ":"
  and entering partial addresses is a big plus IMO.

I think both can coexist on their own merits. Since rte_ether.h needs to be
fixed either way, how about I move this function in a separate commit and
address the conflict with netinet/ether.h while there?

-- 
Adrien Mazarguil
6WIND