[RFC] rte_ether_unformat: accept more inputs

[RFC] rte_ether_unformat: accept more inputs

[Stephen Hemminger]

This updates rte_ether_addr_unformat() to accept more types
of input. There have been requests to handle Windows and other formats.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
Marking this as RFC until unit tests are added.

 lib/net/rte_ether.c | 78 +++++++++++++++++++++++++++++++++++----------
 lib/net/rte_ether.h |  6 ++--
 2 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/lib/net/rte_ether.c b/lib/net/rte_ether.c
index 66d9a9d0699a..5250353eb162 100644
--- a/lib/net/rte_ether.c
+++ b/lib/net/rte_ether.c
@@ -38,7 +38,8 @@ static int8_t get_xdigit(char ch)
 }
 
 /* Convert 00:11:22:33:44:55 to ethernet address */
-static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea)
+static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea,
+			    const char sep)
 {
 	const char *s = s0;
 	int i;
@@ -50,14 +51,17 @@ static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea)
 		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;
+		ea->addr_bytes[i] = x;
+		if (*s != sep) {
+			x = get_xdigit(*s++);
+			if (x < 0)
+				return false;
+			ea->addr_bytes[i] <<= 4;
+			ea->addr_bytes[i] |= x;
+		}
 
 		if (i < RTE_ETHER_ADDR_LEN - 1 &&
-		    *s++ != ':')
+		    *s++ != sep)
 			return false;
 	}
 
@@ -66,7 +70,8 @@ static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea)
 }
 
 /* Convert 0011:2233:4455 to ethernet address */
-static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
+static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea,
+			    const char sep)
 {
 	int i, j;
 
@@ -80,12 +85,14 @@ static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
 			if (x < 0)
 				return false;
 			w = (w << 4) | x;
+			if (*s == sep)
+				break;
 		}
 		ea->addr_bytes[i] = w >> 8;
 		ea->addr_bytes[i + 1] = w & 0xff;
 
 		if (i < RTE_ETHER_ADDR_LEN - 2 &&
-		    *s++ != ':')
+		    *s++ != sep)
 			return false;
 	}
 
@@ -93,17 +100,56 @@ static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
 }
 
 /*
- * Like ether_aton_r but can handle either
- * XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
- * and is more restrictive.
+ * Scan input to see if seperated by dash, colon or period
+ * Returns seperator and number of matches
+ * If seperators are mixed will return
+ */
+static unsigned int get_ether_sep(const char *s, char *sep)
+{
+	const char seperators[] = "-:.";
+	unsigned int count = 0;
+	const char *cp;
+
+	cp = strpbrk(s, seperators);
+	if (cp == NULL)
+		return 0;
+
+	*sep = *cp;
+	do {
+		++count;
+		/* find next instance of seperator */
+		cp = strchr(cp + 1, *sep);
+	} while (cp != NULL);
+
+	return count;
+}
+
+/*
+ * Be libreal in accepting a wide variety of notational formats
+ * for MAC address including:
+ *  - Linux format six groups of hexadecimal digits seperated by colon
+ *  - Windows format six groups seperated by hyphen
+ *  - two groups hexadecimal digits
  */
 int
 rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea)
 {
-	if (get_ether_addr6(s, ea))
-		return 0;
-	if (get_ether_addr3(s, ea))
-		return 0;
+	unsigned int count;
+	char sep = '\0';
+
+	count = get_ether_sep(s, &sep);
+	switch (count) {
+	case 5:	/* i.e 01:23:45:67:89:AB */
+		if (get_ether_addr6(s, ea, sep))
+			return 0;
+		break;
+	case 2: /* i.e 0123.4567.89AB */
+		if (get_ether_addr3(s, ea, sep))
+			return 0;
+		break;
+	default:
+		break;
+	}
 
 	rte_errno = EINVAL;
 	return -1;
diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index b35c72c7b0e0..e9a4ba9b5860 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -254,8 +254,10 @@ rte_ether_format_addr(char *buf, uint16_t size,
  *
  * @param str
  *   A pointer to buffer contains the formatted MAC address.
- *   The supported formats are:
- *     XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
+ *   The example formats are:
+ *     XX:XX:XX:XX:XX:XX - Canonical form
+ *     XX-XX-XX-XX-XX-XX - Windows and IEEE 802
+ *     XXXX:XXXX:XXXX    - original DPDK
  *   where XX is a hex digit: 0-9, a-f, or A-F.
  * @param eth_addr
  *   A pointer to a ether_addr structure.
-- 
2.39.2

RE: [RFC] rte_ether_unformat: accept more inputs

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, 29 September 2023 18.36
> 
> This updates rte_ether_addr_unformat() to accept more types
> of input. There have been requests to handle Windows and other formats.

Seems very useful.

[...]

> +/*
> + * Be libreal in accepting a wide variety of notational formats

Typo: libreal -> liberal.

> + * for MAC address including:
> + *  - Linux format six groups of hexadecimal digits seperated by colon
> + *  - Windows format six groups seperated by hyphen

Typo (twice): seperated -> separated

[...]

> --- a/lib/net/rte_ether.h
> +++ b/lib/net/rte_ether.h
> @@ -254,8 +254,10 @@ rte_ether_format_addr(char *buf, uint16_t size,
>   *
>   * @param str
>   *   A pointer to buffer contains the formatted MAC address.
> - *   The supported formats are:
> - *     XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
> + *   The example formats are:
> + *     XX:XX:XX:XX:XX:XX - Canonical form
> + *     XX-XX-XX-XX-XX-XX - Windows and IEEE 802
> + *     XXXX:XXXX:XXXX    - original DPDK
>   *   where XX is a hex digit: 0-9, a-f, or A-F.

Actually, XX is a hex number, not a hex digit.

But I think the intention of this patch is also to allow a hex number of 1 or 2 (or up to 4) hex digits for XX and XXXX?

Re: [RFC] rte_ether_unformat: accept more inputs

[Stephen Hemminger]

On Fri, 29 Sep 2023 21:35:45 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Friday, 29 September 2023 18.36
> > 
> > This updates rte_ether_addr_unformat() to accept more types
> > of input. There have been requests to handle Windows and other formats.  
> 
> Seems very useful.
> 
> [...]
> 
> > +/*
> > + * Be libreal in accepting a wide variety of notational formats  
> 
> Typo: libreal -> liberal.
> 
> > + * for MAC address including:
> > + *  - Linux format six groups of hexadecimal digits seperated by colon
> > + *  - Windows format six groups seperated by hyphen  
> 
> Typo (twice): seperated -> separated

Typos will be fixed in next version in addition to adding tests.

> 
> > --- a/lib/net/rte_ether.h
> > +++ b/lib/net/rte_ether.h
> > @@ -254,8 +254,10 @@ rte_ether_format_addr(char *buf, uint16_t size,
> >   *
> >   * @param str
> >   *   A pointer to buffer contains the formatted MAC address.
> > - *   The supported formats are:
> > - *     XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
> > + *   The example formats are:
> > + *     XX:XX:XX:XX:XX:XX - Canonical form
> > + *     XX-XX-XX-XX-XX-XX - Windows and IEEE 802
> > + *     XXXX:XXXX:XXXX    - original DPDK
> >   *   where XX is a hex digit: 0-9, a-f, or A-F.  
> 
> Actually, XX is a hex number, not a hex digit.
> 
> But I think the intention of this patch is also to allow a hex number of 1 or 2 (or up to 4) hex digits for XX and XXXX?
> 


Good point, the single hex digit format came as a later request.

[PATCH v2 0/3] rte_ether_unformat_addr changes

[Stephen Hemminger]

This patchset makes rte_ether_unformat_addr allow other formats
for MAC address. Need to remove some inputs from existing
cmdline_etheraddr test, and add a new test in test suite
to cover this.  There is some overlap between the two tests
but that is fine.

Stephen Hemminger (3):
  test: remove some strings from cmdline_etheraddr tests
  rte_ether_unformat: accept more inputs
  test: add tests for rte_ether routines

 app/test/meson.build              |   1 +
 app/test/test_cmdline_etheraddr.c |  11 +-
 app/test/test_net_ether.c         | 169 ++++++++++++++++++++++++++++++
 lib/net/rte_ether.c               |  87 +++++++++++----
 lib/net/rte_ether.h               |   9 +-
 5 files changed, 248 insertions(+), 29 deletions(-)
 create mode 100644 app/test/test_net_ether.c

-- 
2.39.2

[PATCH v2 1/3] test: remove some strings from cmdline_etheraddr tests

[Stephen Hemminger]

Some of the ethernet address formats which were invalid will
now become valid inputs when rte_ether_unformat_addr is modified
to allow leading zeros.

Also, make local variables static.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_cmdline_etheraddr.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/app/test/test_cmdline_etheraddr.c b/app/test/test_cmdline_etheraddr.c
index 9691c32ba250..166c5716ba9b 100644
--- a/app/test/test_cmdline_etheraddr.c
+++ b/app/test/test_cmdline_etheraddr.c
@@ -20,7 +20,7 @@ struct ether_addr_str {
 };
 
 /* valid strings */
-const struct ether_addr_str ether_addr_valid_strs[] = {
+static const struct ether_addr_str ether_addr_valid_strs[] = {
 		{"01:23:45:67:89:AB", 0xAB8967452301ULL},
 		{"4567:89AB:CDEF", 0xEFCDAB896745ULL},
 };
@@ -30,7 +30,7 @@ const struct ether_addr_str ether_addr_valid_strs[] = {
  * end of token, which is either space chars, null char or
  * a hash sign.
  */
-const char * ether_addr_garbage_strs[] = {
+static const char * const ether_addr_garbage_strs[] = {
 		"00:11:22:33:44:55\0garbage",
 		"00:11:22:33:44:55#garbage",
 		"00:11:22:33:44:55 garbage",
@@ -46,14 +46,13 @@ const char * ether_addr_garbage_strs[] = {
 #define GARBAGE_ETHERADDR 0x554433221100ULL /* corresponding address */
 
 
-const char * ether_addr_invalid_strs[] = {
+static const char * const ether_addr_invalid_strs[] = {
 		/* valid chars, invalid syntax */
 		"0123:45:67:89:AB",
 		"01:23:4567:89:AB",
 		"01:23:45:67:89AB",
 		"012:345:678:9AB",
 		"01:23:45:67:89:ABC",
-		"01:23:45:67:89:A",
 		"01:23:45:67:89",
 		"01:23:45:67:89:AB:CD",
 		/* invalid chars, valid syntax */
@@ -61,10 +60,8 @@ const char * ether_addr_invalid_strs[] = {
 		"INVA:LIDC:HARS",
 		/* misc */
 		"01 23 45 67 89 AB",
-		"01.23.45.67.89.AB",
 		"01,23,45,67,89,AB",
-		"01:23:45\0:67:89:AB",
-		"01:23:45#:67:89:AB",
+		"01:23:45:67#:89:AB",
 		"random invalid text",
 		"random text",
 		"",
-- 
2.39.2

Re: [PATCH v2 1/3] test: remove some strings from cmdline_etheraddr tests

[Ferruh Yigit]

On 10/2/2023 7:37 PM, Stephen Hemminger wrote:
> Some of the ethernet address formats which were invalid will
> now become valid inputs when rte_ether_unformat_addr is modified
> to allow leading zeros.
> 
> Also, make local variables static.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 

<...>

> @@ -61,10 +60,8 @@ const char * ether_addr_invalid_strs[] = {
>  		"INVA:LIDC:HARS",
>  		/* misc */
>  		"01 23 45 67 89 AB",
> -		"01.23.45.67.89.AB",
>  		"01,23,45,67,89,AB",
> -		"01:23:45\0:67:89:AB",
> -		"01:23:45#:67:89:AB",
>

Why these two are valid now?

And I guess second one is still not valid, but first one is parsed as
[1], is this expected?

[1] 00:01:00:23:00:45

Re: [PATCH v2 1/3] test: remove some strings from cmdline_etheraddr tests

[Ferruh Yigit]

On 10/3/2023 11:47 AM, Ferruh Yigit wrote:
> On 10/2/2023 7:37 PM, Stephen Hemminger wrote:
>> Some of the ethernet address formats which were invalid will
>> now become valid inputs when rte_ether_unformat_addr is modified
>> to allow leading zeros.
>>
>> Also, make local variables static.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>
> 
> <...>
> 
>> @@ -61,10 +60,8 @@ const char * ether_addr_invalid_strs[] = {
>>  		"INVA:LIDC:HARS",
>>  		/* misc */
>>  		"01 23 45 67 89 AB",
>> -		"01.23.45.67.89.AB",
>>  		"01,23,45,67,89,AB",
>> -		"01:23:45\0:67:89:AB",
>> -		"01:23:45#:67:89:AB",
>>
> 
> Why these two are valid now?
> 
> And I guess second one is still not valid, but first one is parsed as
> [1], is this expected?
> 
> [1] 00:01:00:23:00:45
> 
> 

Ah, I guess it is taken as "XXXX:XXXX:XXXX" format, but number of digit
is not enforced, so "1:2:3" is a valid format, should we add this to API
documentation as example format? Or is this unintended side effect?

Re: [PATCH v2 1/3] test: remove some strings from cmdline_etheraddr tests

[Stephen Hemminger]

On Tue, 3 Oct 2023 11:59:04 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> Ah, I guess it is taken as "XXXX:XXXX:XXXX" format, but number of digit
> is not enforced, so "1:2:3" is a valid format, should we add this to API
> documentation as example format? Or is this unintended side effect?

By allowing leading zeros, it becomes allowed.
DPDK always allowed the non-standard 3 part format.
Looking around only old Cisco ACS used 3 part MAC format and it used
periods.

The API documentation should mention leading zeros are optional.

Re: [PATCH v2 1/3] test: remove some strings from cmdline_etheraddr tests

[Stephen Hemminger]

On Tue, 3 Oct 2023 11:47:51 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 10/2/2023 7:37 PM, Stephen Hemminger wrote:
> > Some of the ethernet address formats which were invalid will
> > now become valid inputs when rte_ether_unformat_addr is modified
> > to allow leading zeros.
> > 
> > Also, make local variables static.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >   
> 
> <...>
> 
> > @@ -61,10 +60,8 @@ const char * ether_addr_invalid_strs[] = {
> >  		"INVA:LIDC:HARS",
> >  		/* misc */
> >  		"01 23 45 67 89 AB",
> > -		"01.23.45.67.89.AB",
> >  		"01,23,45,67,89,AB",
> > -		"01:23:45\0:67:89:AB",
> > -		"01:23:45#:67:89:AB",
> >  
> 
> Why these two are valid now?
> 
> And I guess second one is still not valid, but first one is parsed as
> [1], is this expected?
> 
> [1] 00:01:00:23:00:45

The code in cmdline converts the comment character # to a null byte.
So both test are the same.

With new unformat, it allows a 3 part mac address with leading
zeros.
	01:23:45 is equivalent to 0001:0023:0045

Re: [PATCH v2 1/3] test: remove some strings from cmdline_etheraddr tests

[Ferruh Yigit]

On 10/3/2023 5:36 PM, Stephen Hemminger wrote:
> On Tue, 3 Oct 2023 11:47:51 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> On 10/2/2023 7:37 PM, Stephen Hemminger wrote:
>>> Some of the ethernet address formats which were invalid will
>>> now become valid inputs when rte_ether_unformat_addr is modified
>>> to allow leading zeros.
>>>
>>> Also, make local variables static.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>   
>>
>> <...>
>>
>>> @@ -61,10 +60,8 @@ const char * ether_addr_invalid_strs[] = {
>>>  		"INVA:LIDC:HARS",
>>>  		/* misc */
>>>  		"01 23 45 67 89 AB",
>>> -		"01.23.45.67.89.AB",
>>>  		"01,23,45,67,89,AB",
>>> -		"01:23:45\0:67:89:AB",
>>> -		"01:23:45#:67:89:AB",
>>>  
>>
>> Why these two are valid now?
>>
>> And I guess second one is still not valid, but first one is parsed as
>> [1], is this expected?
>>
>> [1] 00:01:00:23:00:45
> 
> The code in cmdline converts the comment character # to a null byte.
> So both test are the same.
> 
> With new unformat, it allows a 3 part mac address with leading
> zeros.
> 	01:23:45 is equivalent to 0001:0023:0045
>

With 3 part MAC, omitting leading zeros looks confusing to me, because
that omitted part becomes an octet in MAC. Like:
01:23:45 being equivalent to 00:01:00:23:00:45

As 3 part MAC, and 6 part MAC has separate functions, does it makes
sense to require leading zeros in the 3 part MAC, what do you think?

Re: [PATCH v2 1/3] test: remove some strings from cmdline_etheraddr tests

[Stephen Hemminger]

On Tue, 3 Oct 2023 17:50:13 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 10/3/2023 5:36 PM, Stephen Hemminger wrote:
> > On Tue, 3 Oct 2023 11:47:51 +0100
> > Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >   
> >> On 10/2/2023 7:37 PM, Stephen Hemminger wrote:  
> >>> Some of the ethernet address formats which were invalid will
> >>> now become valid inputs when rte_ether_unformat_addr is modified
> >>> to allow leading zeros.
> >>>
> >>> Also, make local variables static.
> >>>
> >>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>>     
> >>
> >> <...>
> >>  
> >>> @@ -61,10 +60,8 @@ const char * ether_addr_invalid_strs[] = {
> >>>  		"INVA:LIDC:HARS",
> >>>  		/* misc */
> >>>  		"01 23 45 67 89 AB",
> >>> -		"01.23.45.67.89.AB",
> >>>  		"01,23,45,67,89,AB",
> >>> -		"01:23:45\0:67:89:AB",
> >>> -		"01:23:45#:67:89:AB",
> >>>    
> >>
> >> Why these two are valid now?
> >>
> >> And I guess second one is still not valid, but first one is parsed as
> >> [1], is this expected?
> >>
> >> [1] 00:01:00:23:00:45  
> > 
> > The code in cmdline converts the comment character # to a null byte.
> > So both test are the same.
> > 
> > With new unformat, it allows a 3 part mac address with leading
> > zeros.
> > 	01:23:45 is equivalent to 0001:0023:0045
> >  
> 
> With 3 part MAC, omitting leading zeros looks confusing to me, because
> that omitted part becomes an octet in MAC. Like:
> 01:23:45 being equivalent to 00:01:00:23:00:45
> 
> As 3 part MAC, and 6 part MAC has separate functions, does it makes
> sense to require leading zeros in the 3 part MAC, what do you think?
> 

Right, 3 part MAC is only a legacy Cisco format, not used anywhere else.

[PATCH v2 2/3] rte_ether_unformat: accept more inputs

[Stephen Hemminger]

This updates rte_ether_addr_unformat() to accept more types
formats for MAC address. It allows IEEE, IETF and Cisco
formats with leading zeros as well.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/net/rte_ether.c | 87 ++++++++++++++++++++++++++++++++++-----------
 lib/net/rte_ether.h |  9 +++--
 2 files changed, 74 insertions(+), 22 deletions(-)

diff --git a/lib/net/rte_ether.c b/lib/net/rte_ether.c
index 66d9a9d0699a..de3f456207e8 100644
--- a/lib/net/rte_ether.c
+++ b/lib/net/rte_ether.c
@@ -38,7 +38,8 @@ static int8_t get_xdigit(char ch)
 }
 
 /* Convert 00:11:22:33:44:55 to ethernet address */
-static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea)
+static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea,
+			    const char sep)
 {
 	const char *s = s0;
 	int i;
@@ -48,25 +49,29 @@ static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea)
 
 		x = get_xdigit(*s++);
 		if (x < 0)
-			return false;
+			return false;	/* not a hex digit */
 
-		ea->addr_bytes[i] = x << 4;
-		x = get_xdigit(*s++);
-		if (x < 0)
-			return false;
-		ea->addr_bytes[i] |= x;
+		ea->addr_bytes[i] = x;
+		if (*s != sep && *s != '\0') {
+			x = get_xdigit(*s++);
+			if (x < 0)
+				return false;	/* not a hex digit */
+			ea->addr_bytes[i] <<= 4;
+			ea->addr_bytes[i] |= x;
+		}
 
 		if (i < RTE_ETHER_ADDR_LEN - 1 &&
-		    *s++ != ':')
-			return false;
+		    *s++ != sep)
+			return false;	/* premature end of string */
 	}
 
-	/* return true if at end of string */
+	/* return true if no trailing characters */
 	return *s == '\0';
 }
 
 /* Convert 0011:2233:4455 to ethernet address */
-static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
+static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea,
+			    const char sep)
 {
 	int i, j;
 
@@ -78,14 +83,17 @@ static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
 
 			x = get_xdigit(*s++);
 			if (x < 0)
-				return false;
+				return false;	/* not a hex digit */
 			w = (w << 4) | x;
+			if (*s == sep || *s == '\0')
+				break;
 		}
+
 		ea->addr_bytes[i] = w >> 8;
 		ea->addr_bytes[i + 1] = w & 0xff;
 
 		if (i < RTE_ETHER_ADDR_LEN - 2 &&
-		    *s++ != ':')
+		    *s++ != sep)
 			return false;
 	}
 
@@ -93,17 +101,56 @@ static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
 }
 
 /*
- * Like ether_aton_r but can handle either
- * XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
- * and is more restrictive.
+ * Scan input to see if separated by dash, colon or period
+ * Returns separator and number of matches
+ * If separators are mixed will return
+ */
+static unsigned int get_ether_sep(const char *s, char *sep)
+{
+	static const char separators[] = "-:.";
+	unsigned int count = 0;
+	const char *cp;
+
+	cp = strpbrk(s, separators);
+	if (cp == NULL)
+		return 0;	/* no separator found */
+
+	*sep = *cp;		/* return the separator */
+	do {
+		++count;
+		/* find next instance of separator */
+		cp = strchr(cp + 1, *sep);
+	} while (cp != NULL);
+
+	return count;
+}
+
+/*
+ * Be liberal in accepting a wide variety of notational formats
+ * for MAC address including:
+ *  - Linux format six groups of hexadecimal digits separated by colon
+ *  - Windows format six groups separated by hyphen
+ *  - two groups hexadecimal digits
  */
 int
 rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea)
 {
-	if (get_ether_addr6(s, ea))
-		return 0;
-	if (get_ether_addr3(s, ea))
-		return 0;
+	unsigned int count;
+	char sep = '\0';
+
+	count = get_ether_sep(s, &sep);
+	switch (count) {
+	case 5:	/* i.e 01:23:45:67:89:AB */
+		if (get_ether_addr6(s, ea, sep))
+			return 0;
+		break;
+	case 2: /* i.e 0123.4567.89AB */
+		if (get_ether_addr3(s, ea, sep))
+			return 0;
+		break;
+	default:
+		break;
+	}
 
 	rte_errno = EINVAL;
 	return -1;
diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index b35c72c7b0e0..097aedcf21eb 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -254,8 +254,13 @@ rte_ether_format_addr(char *buf, uint16_t size,
  *
  * @param str
  *   A pointer to buffer contains the formatted MAC address.
- *   The supported formats are:
- *     XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
+ *   Accepts either byte or word format separated by colon,
+ *   hyphen or period.
+ *
+ *   The example formats are:
+ *     XX:XX:XX:XX:XX:XX - Canonical form
+ *     XX-XX-XX-XX-XX-XX - Windows and IEEE 802
+ *     XXXX.XXXX.XXXX    - Cisco
  *   where XX is a hex digit: 0-9, a-f, or A-F.
  * @param eth_addr
  *   A pointer to a ether_addr structure.
-- 
2.39.2

[PATCH v2 3/3] test: add tests for rte_ether routines

[Stephen Hemminger]

This add some basic tests for rte_unformat_ether_addr
and other functions in rte_ether.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/meson.build      |   1 +
 app/test/test_net_ether.c | 169 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 170 insertions(+)
 create mode 100644 app/test/test_net_ether.c

diff --git a/app/test/meson.build b/app/test/meson.build
index bf9fc906128f..20a9333c726d 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -124,6 +124,7 @@ source_file_deps = {
     'test_meter.c': ['meter'],
     'test_metrics.c': ['metrics'],
     'test_mp_secondary.c': ['hash', 'lpm'],
+    'test_net_ether.c': ['net'],
     'test_pcapng.c': ['ethdev', 'net', 'pcapng'],
     'test_pdcp.c': ['eventdev', 'pdcp', 'net', 'timer', 'security'],
     'test_pdump.c': ['pdump'] + sample_packet_forward_deps,
diff --git a/app/test/test_net_ether.c b/app/test/test_net_ether.c
new file mode 100644
index 000000000000..a6d38cc2ffa0
--- /dev/null
+++ b/app/test/test_net_ether.c
@@ -0,0 +1,169 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2023 Stephen Hemminger
+ */
+
+#include <rte_ether.h>
+
+#include <rte_test.h>
+#include "test.h"
+
+#define N 1000000
+
+static const struct rte_ether_addr zero_ea;
+static const struct rte_ether_addr bcast_ea = {
+	.addr_bytes = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
+};
+
+static int
+test_ether_addr(void)
+{
+	struct rte_ether_addr rand_ea = { };
+	unsigned int i;
+
+	RTE_TEST_ASSERT(rte_is_zero_ether_addr(&zero_ea), "Zero address is not zero");
+	RTE_TEST_ASSERT(!rte_is_zero_ether_addr(&bcast_ea), "Broadcast is zero");
+
+	for (i = 0; i < N; i++) {
+		rte_eth_random_addr(rand_ea.addr_bytes);
+		RTE_TEST_ASSERT(!rte_is_zero_ether_addr(&rand_ea),
+				"Random address is zero");
+		RTE_TEST_ASSERT(rte_is_unicast_ether_addr(&rand_ea),
+				"Random addrss is not unicast");
+		RTE_TEST_ASSERT(rte_is_local_admin_ether_addr(&rand_ea),
+				"Random address is not local admin");
+	}
+
+	return 0;
+}
+
+static int
+test_format_addr(void)
+{
+	struct rte_ether_addr rand_ea = { };
+	char buf[RTE_ETHER_ADDR_FMT_SIZE];
+	unsigned int i;
+
+	for (i = 0; i < N; i++) {
+		struct rte_ether_addr result = { };
+		int ret;
+
+		rte_eth_random_addr(rand_ea.addr_bytes);
+
+		rte_ether_format_addr(buf, sizeof(buf), &rand_ea);
+
+		ret = rte_ether_unformat_addr(buf, &result);
+		if (ret != 0) {
+			fprintf(stderr, "rte_ether_unformat_addr(%s) failed\n", buf);
+			return -1;
+		}
+		RTE_TEST_ASSERT(rte_is_same_ether_addr(&rand_ea, &result),
+			"rte_ether_format/unformat mismatch");
+	}
+	return 0;
+
+}
+
+static int
+test_unformat_addr(void)
+{
+	const struct rte_ether_addr expected = {
+		.addr_bytes = { 0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc },
+	};
+	const struct rte_ether_addr nozero_ea = {
+		.addr_bytes = { 1, 2, 3, 4, 5, 6 },
+	};
+	struct rte_ether_addr result;
+	int ret;
+
+	/* Test IETF format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("12:34:56:78:9a:bc", &result);
+	RTE_TEST_ASSERT(ret == 0, "IETF unformat failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&expected, &result),
+		"IETF unformat mismatch");
+
+	/* Test IEEE format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("12-34-56-78-9A-BC", &result);
+	RTE_TEST_ASSERT(ret == 0, "IEEE unformat failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&expected, &result),
+			"IEEE unformat mismatch");
+
+	/* Test Cisco format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("1234.5678.9ABC", &result);
+	RTE_TEST_ASSERT(ret == 0, "Cisco unformat failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&expected, &result),
+			"Cisco unformat mismatch");
+
+	/* Test no leading zeros - IETF */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("1:2:3:4:5:6", &result);
+	RTE_TEST_ASSERT(ret == 0, "IETF leading zero failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&nozero_ea, &result),
+			"IETF leading zero mismatch");
+
+	/* Test no-leading zero - IEEE format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("1-2-3-4-5-6", &result);
+	RTE_TEST_ASSERT(ret == 0, "IEEE leading zero failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&nozero_ea, &result),
+			"IEEE leading zero mismatch");
+
+	/* Test no-leading zero - Cisco format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("102.304.506", &result);
+	RTE_TEST_ASSERT(ret == 0, "Cisco leading zero failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&nozero_ea, &result),
+			"Cisco leading zero mismatch");
+
+	return 0;
+}
+
+static int
+test_invalid_addr(void)
+{
+	static const char * const invalid[] = {
+		"NO:ADDRESS",
+		"123",
+		"12:34:56:78:9a:gh",
+		"12:34:56:78:9a",
+		"100:34:56:78:9a:bc",
+		"34-56-78-9a-bc",
+		"12:34:56-78:9a:bc",
+		"12:34:56.78:9a:bc",
+		"123:456:789:abc",
+		"",
+	};
+	struct rte_ether_addr result;
+	unsigned int i;
+
+	for (i = 0; i < RTE_DIM(invalid); ++i) {
+		if (!rte_ether_unformat_addr(invalid[i], &result)) {
+			fprintf(stderr, "rte_ether_unformat_addr(%s) succeeded!\n",
+				invalid[i]);
+			return -1;
+		}
+	}
+	return 0;
+}
+
+static int
+test_net_ether(void)
+{
+	if (test_ether_addr())
+		return -1;
+
+	if (test_format_addr())
+		return -1;
+
+	if (test_unformat_addr())
+		return -1;
+
+	if (test_invalid_addr())
+		return -1;
+
+	return 0;
+}
+
+REGISTER_FAST_TEST(net_ether_autotest, true, true, test_net_ether);
-- 
2.39.2

RE: [PATCH v2 0/3] rte_ether_unformat_addr changes

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, 2 October 2023 20.37
] rte_ether_unformat_addr changes
> 
> This patchset makes rte_ether_unformat_addr allow other formats
> for MAC address. Need to remove some inputs from existing
> cmdline_etheraddr test, and add a new test in test suite
> to cover this.  There is some overlap between the two tests
> but that is fine.
> 
> Stephen Hemminger (3):
>   test: remove some strings from cmdline_etheraddr tests
>   rte_ether_unformat: accept more inputs
>   test: add tests for rte_ether routines
> 
>  app/test/meson.build              |   1 +
>  app/test/test_cmdline_etheraddr.c |  11 +-
>  app/test/test_net_ether.c         | 169 ++++++++++++++++++++++++++++++
>  lib/net/rte_ether.c               |  87 +++++++++++----
>  lib/net/rte_ether.h               |   9 +-
>  5 files changed, 248 insertions(+), 29 deletions(-)
>  create mode 100644 app/test/test_net_ether.c
> 
> --
> 2.39.2

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v2 0/3] rte_ether_unformat_addr changes

[Ferruh Yigit]

On 10/2/2023 7:37 PM, Stephen Hemminger wrote:
> This patchset makes rte_ether_unformat_addr allow other formats
> for MAC address. Need to remove some inputs from existing
> cmdline_etheraddr test, and add a new test in test suite
> to cover this.  There is some overlap between the two tests
> but that is fine.
> 
> Stephen Hemminger (3):
>   test: remove some strings from cmdline_etheraddr tests
>   rte_ether_unformat: accept more inputs
>   test: add tests for rte_ether routines
> 

Thanks Stephen,

This enables using the API as replacement to the tap PMD's local parse
implementation:
https://patchwork.dpdk.org/project/dpdk/patch/20230323170145.129901-1-drc@linux.vnet.ibm.com/

Re: [PATCH v2 0/3] rte_ether_unformat_addr changes

[Stephen Hemminger]

On Tue, 3 Oct 2023 11:44:16 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 10/2/2023 7:37 PM, Stephen Hemminger wrote:
> > This patchset makes rte_ether_unformat_addr allow other formats
> > for MAC address. Need to remove some inputs from existing
> > cmdline_etheraddr test, and add a new test in test suite
> > to cover this.  There is some overlap between the two tests
> > but that is fine.
> > 
> > Stephen Hemminger (3):
> >   test: remove some strings from cmdline_etheraddr tests
> >   rte_ether_unformat: accept more inputs
> >   test: add tests for rte_ether routines
> >   
> 
> Thanks Stephen,
> 
> This enables using the API as replacement to the tap PMD's local parse
> implementation:
> https://patchwork.dpdk.org/project/dpdk/patch/20230323170145.129901-1-drc@linux.vnet.ibm.com/


That looks good, but indentation is wrong and it duplicates a check for value == NULL
which will cause a new Coverity warning.

Re: [PATCH v2 0/3] rte_ether_unformat_addr changes

[Stephen Hemminger]

On Tue, 3 Oct 2023 11:44:16 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 10/2/2023 7:37 PM, Stephen Hemminger wrote:
> > This patchset makes rte_ether_unformat_addr allow other formats
> > for MAC address. Need to remove some inputs from existing
> > cmdline_etheraddr test, and add a new test in test suite
> > to cover this.  There is some overlap between the two tests
> > but that is fine.
> > 
> > Stephen Hemminger (3):
> >   test: remove some strings from cmdline_etheraddr tests
> >   rte_ether_unformat: accept more inputs
> >   test: add tests for rte_ether routines
> >   
> 
> Thanks Stephen,
> 
> This enables using the API as replacement to the tap PMD's local parse
> implementation:
> https://patchwork.dpdk.org/project/dpdk/patch/20230323170145.129901-1-drc@linux.vnet.ibm.com/


It can be simplified to just this.
No need to check value == NULL, already checkd.
No need to check for user_mac == NULL, since only called one place and that place
passes pointer to stack variable.


From b478a17f13a1bedadca3b60608c585f31c9ad8f2 Mon Sep 17 00:00:00 2001
From: David Christensen <drc@linux.vnet.ibm.com>
Date: Thu, 23 Mar 2023 13:01:45 -0400
Subject: [PATCH] net/tap: resolve stringop-overflow with gcc 12 on ppc64le

Building DPDK with gcc 12 on a ppc64le system generates a
stringop-overflow warning. Replace the local MAC address
validation function parse_user_mac() with a call to
rte_ether_unformat_addr() instead.

Bugzilla ID: 1197
Cc: stable@dpdk.org

Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
---
 drivers/net/tap/rte_eth_tap.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index bf98f7555990..b25a52655fa2 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2267,29 +2267,6 @@ set_remote_iface(const char *key __rte_unused,
 	return 0;
 }
 
-static int parse_user_mac(struct rte_ether_addr *user_mac,
-		const char *value)
-{
-	unsigned int index = 0;
-	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
-
-	if (user_mac == NULL || value == NULL)
-		return 0;
-
-	strlcpy(mac_temp, value, sizeof(mac_temp));
-	mac_byte = strtok(mac_temp, ":");
-
-	while ((mac_byte != NULL) &&
-			(strlen(mac_byte) <= 2) &&
-			(strlen(mac_byte) == strspn(mac_byte,
-					ETH_TAP_CMP_MAC_FMT))) {
-		user_mac->addr_bytes[index++] = strtoul(mac_byte, NULL, 16);
-		mac_byte = strtok(NULL, ":");
-	}
-
-	return index;
-}
-
 static int
 set_mac_type(const char *key __rte_unused,
 	     const char *value,
@@ -2311,7 +2288,7 @@ set_mac_type(const char *key __rte_unused,
 		goto success;
 	}
 
-	if (parse_user_mac(user_mac, value) != 6)
+	if (rte_ether_unformat_addr(value, user_mac) < 0)
 		goto error;
 success:
 	TAP_LOG(DEBUG, "TAP user MAC param (%s)", value);
-- 
2.39.2

[PATCH v3 0/4] rte_ether_unformat_addr related changes

[Stephen Hemminger]

This patchset makes rte_ether_unformat_addr allow other formats
for MAC address. Need to remove some inputs from existing
cmdline_etheraddr test, and add a new test in test suite
to cover this.  There is some overlap between the two tests
but that is fine.

v4 - fix spelling errors
     don't allow leading zeros in Cisco 3 part format
     incorporate tap patch to use unformat addr

David Christensen (1):
  net/tap: use rte_ether_unformat_address

Stephen Hemminger (3):
  test: remove some strings from cmdline_etheraddr tests
  rte_ether_unformat: accept more inputs
  test: add tests for rte_ether routines

 app/test/meson.build              |   1 +
 app/test/test_cmdline_etheraddr.c |   8 +-
 app/test/test_net_ether.c         | 165 ++++++++++++++++++++++++++++++
 drivers/net/tap/rte_eth_tap.c     |  25 +----
 lib/net/rte_ether.c               |  85 +++++++++++----
 lib/net/rte_ether.h               |  10 +-
 6 files changed, 243 insertions(+), 51 deletions(-)
 create mode 100644 app/test/test_net_ether.c

-- 
2.39.2

[PATCH v3 1/4] test: remove some strings from cmdline_etheraddr tests

[Stephen Hemminger]

Some of the ethernet address formats which were invalid will
now become valid inputs when rte_ether_unformat_addr is modified
to allow leading zeros.

Also, make local variables static.

Acked-by: Morten Brørup <mb@smartsharesystems.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_cmdline_etheraddr.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/app/test/test_cmdline_etheraddr.c b/app/test/test_cmdline_etheraddr.c
index 9691c32ba250..74953dea6cb9 100644
--- a/app/test/test_cmdline_etheraddr.c
+++ b/app/test/test_cmdline_etheraddr.c
@@ -20,7 +20,7 @@ struct ether_addr_str {
 };
 
 /* valid strings */
-const struct ether_addr_str ether_addr_valid_strs[] = {
+static const struct ether_addr_str ether_addr_valid_strs[] = {
 		{"01:23:45:67:89:AB", 0xAB8967452301ULL},
 		{"4567:89AB:CDEF", 0xEFCDAB896745ULL},
 };
@@ -30,7 +30,7 @@ const struct ether_addr_str ether_addr_valid_strs[] = {
  * end of token, which is either space chars, null char or
  * a hash sign.
  */
-const char * ether_addr_garbage_strs[] = {
+static const char * const ether_addr_garbage_strs[] = {
 		"00:11:22:33:44:55\0garbage",
 		"00:11:22:33:44:55#garbage",
 		"00:11:22:33:44:55 garbage",
@@ -46,14 +46,13 @@ const char * ether_addr_garbage_strs[] = {
 #define GARBAGE_ETHERADDR 0x554433221100ULL /* corresponding address */
 
 
-const char * ether_addr_invalid_strs[] = {
+static const char * const ether_addr_invalid_strs[] = {
 		/* valid chars, invalid syntax */
 		"0123:45:67:89:AB",
 		"01:23:4567:89:AB",
 		"01:23:45:67:89AB",
 		"012:345:678:9AB",
 		"01:23:45:67:89:ABC",
-		"01:23:45:67:89:A",
 		"01:23:45:67:89",
 		"01:23:45:67:89:AB:CD",
 		/* invalid chars, valid syntax */
@@ -61,7 +60,6 @@ const char * ether_addr_invalid_strs[] = {
 		"INVA:LIDC:HARS",
 		/* misc */
 		"01 23 45 67 89 AB",
-		"01.23.45.67.89.AB",
 		"01,23,45,67,89,AB",
 		"01:23:45\0:67:89:AB",
 		"01:23:45#:67:89:AB",
-- 
2.39.2

[PATCH v3 2/4] rte_ether_unformat: accept more inputs

[Stephen Hemminger]

This updates rte_ether_addr_unformat() to accept more types
formats for MAC address. It allows IEEE, IETF and Cisco
formats. Leading zeros are allowed for byte formats.

Acked-by: Morten Brørup <mb@smartsharesystems.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/net/rte_ether.c | 85 ++++++++++++++++++++++++++++++++++-----------
 lib/net/rte_ether.h | 10 ++++--
 2 files changed, 73 insertions(+), 22 deletions(-)

diff --git a/lib/net/rte_ether.c b/lib/net/rte_ether.c
index 66d9a9d0699a..f59c20289d37 100644
--- a/lib/net/rte_ether.c
+++ b/lib/net/rte_ether.c
@@ -38,7 +38,8 @@ static int8_t get_xdigit(char ch)
 }
 
 /* Convert 00:11:22:33:44:55 to ethernet address */
-static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea)
+static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea,
+			    const char sep)
 {
 	const char *s = s0;
 	int i;
@@ -48,25 +49,29 @@ static bool get_ether_addr6(const char *s0, struct rte_ether_addr *ea)
 
 		x = get_xdigit(*s++);
 		if (x < 0)
-			return false;
+			return false;	/* not a hex digit */
 
-		ea->addr_bytes[i] = x << 4;
-		x = get_xdigit(*s++);
-		if (x < 0)
-			return false;
-		ea->addr_bytes[i] |= x;
+		ea->addr_bytes[i] = x;
+		if (*s != sep && *s != '\0') {
+			x = get_xdigit(*s++);
+			if (x < 0)
+				return false;	/* not a hex digit */
+			ea->addr_bytes[i] <<= 4;
+			ea->addr_bytes[i] |= x;
+		}
 
 		if (i < RTE_ETHER_ADDR_LEN - 1 &&
-		    *s++ != ':')
-			return false;
+		    *s++ != sep)
+			return false;	/* premature end of string */
 	}
 
-	/* return true if at end of string */
+	/* return true if no trailing characters */
 	return *s == '\0';
 }
 
 /* Convert 0011:2233:4455 to ethernet address */
-static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
+static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea,
+			    const char sep)
 {
 	int i, j;
 
@@ -78,14 +83,15 @@ static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
 
 			x = get_xdigit(*s++);
 			if (x < 0)
-				return false;
+				return false;	/* not a hex digit */
 			w = (w << 4) | x;
 		}
+
 		ea->addr_bytes[i] = w >> 8;
 		ea->addr_bytes[i + 1] = w & 0xff;
 
 		if (i < RTE_ETHER_ADDR_LEN - 2 &&
-		    *s++ != ':')
+		    *s++ != sep)
 			return false;
 	}
 
@@ -93,17 +99,56 @@ static bool get_ether_addr3(const char *s, struct rte_ether_addr *ea)
 }
 
 /*
- * Like ether_aton_r but can handle either
- * XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
- * and is more restrictive.
+ * Scan input to see if separated by dash, colon or period
+ * Returns separator and number of matches
+ * If separators are mixed will return
+ */
+static unsigned int get_ether_sep(const char *s, char *sep)
+{
+	static const char separators[] = "-:.";
+	unsigned int count = 0;
+	const char *cp;
+
+	cp = strpbrk(s, separators);
+	if (cp == NULL)
+		return 0;	/* no separator found */
+
+	*sep = *cp;		/* return the separator */
+	do {
+		++count;
+		/* find next instance of separator */
+		cp = strchr(cp + 1, *sep);
+	} while (cp != NULL);
+
+	return count;
+}
+
+/*
+ * Be liberal in accepting a wide variety of notational formats
+ * for MAC address including:
+ *  - Linux format six groups of hexadecimal digits separated by colon
+ *  - Windows format six groups separated by hyphen
+ *  - two groups hexadecimal digits
  */
 int
 rte_ether_unformat_addr(const char *s, struct rte_ether_addr *ea)
 {
-	if (get_ether_addr6(s, ea))
-		return 0;
-	if (get_ether_addr3(s, ea))
-		return 0;
+	unsigned int count;
+	char sep = '\0';
+
+	count = get_ether_sep(s, &sep);
+	switch (count) {
+	case 5:	/* i.e 01:23:45:67:89:AB */
+		if (get_ether_addr6(s, ea, sep))
+			return 0;
+		break;
+	case 2: /* i.e 0123.4567.89AB */
+		if (get_ether_addr3(s, ea, sep))
+			return 0;
+		break;
+	default:
+		break;
+	}
 
 	rte_errno = EINVAL;
 	return -1;
diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index b35c72c7b0e0..ce073ea818a2 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -254,9 +254,15 @@ rte_ether_format_addr(char *buf, uint16_t size,
  *
  * @param str
  *   A pointer to buffer contains the formatted MAC address.
- *   The supported formats are:
- *     XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX
+ *   Accepts either byte or word format separated by colon,
+ *   hyphen or period.
+ *
+ *   The example formats are:
+ *     XX:XX:XX:XX:XX:XX - Canonical form
+ *     XX-XX-XX-XX-XX-XX - Windows and IEEE 802
+ *     XXXX.XXXX.XXXX    - Cisco
  *   where XX is a hex digit: 0-9, a-f, or A-F.
+ *   In the byte format, leading zeros are optional.
  * @param eth_addr
  *   A pointer to a ether_addr structure.
  * @return
-- 
2.39.2

[PATCH v3 3/4] test: add tests for rte_ether routines

[Stephen Hemminger]

This add some basic tests for rte_unformat_ether_addr
and other functions in rte_ether.

Acked-by: Morten Brørup <mb@smartsharesystems.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/meson.build      |   1 +
 app/test/test_net_ether.c | 165 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 app/test/test_net_ether.c

diff --git a/app/test/meson.build b/app/test/meson.build
index bf9fc906128f..20a9333c726d 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -124,6 +124,7 @@ source_file_deps = {
     'test_meter.c': ['meter'],
     'test_metrics.c': ['metrics'],
     'test_mp_secondary.c': ['hash', 'lpm'],
+    'test_net_ether.c': ['net'],
     'test_pcapng.c': ['ethdev', 'net', 'pcapng'],
     'test_pdcp.c': ['eventdev', 'pdcp', 'net', 'timer', 'security'],
     'test_pdump.c': ['pdump'] + sample_packet_forward_deps,
diff --git a/app/test/test_net_ether.c b/app/test/test_net_ether.c
new file mode 100644
index 000000000000..1cb6845a9c48
--- /dev/null
+++ b/app/test/test_net_ether.c
@@ -0,0 +1,165 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2023 Stephen Hemminger
+ */
+
+#include <rte_ether.h>
+
+#include <rte_test.h>
+#include "test.h"
+
+#define N 1000000
+
+static const struct rte_ether_addr zero_ea;
+static const struct rte_ether_addr bcast_ea = {
+	.addr_bytes = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
+};
+
+static int
+test_ether_addr(void)
+{
+	struct rte_ether_addr rand_ea = { };
+	unsigned int i;
+
+	RTE_TEST_ASSERT(rte_is_zero_ether_addr(&zero_ea), "Zero address is not zero");
+	RTE_TEST_ASSERT(!rte_is_zero_ether_addr(&bcast_ea), "Broadcast is zero");
+
+	for (i = 0; i < N; i++) {
+		rte_eth_random_addr(rand_ea.addr_bytes);
+		RTE_TEST_ASSERT(!rte_is_zero_ether_addr(&rand_ea),
+				"Random address is zero");
+		RTE_TEST_ASSERT(rte_is_unicast_ether_addr(&rand_ea),
+				"Random address is not unicast");
+		RTE_TEST_ASSERT(rte_is_local_admin_ether_addr(&rand_ea),
+				"Random address is not local admin");
+	}
+
+	return 0;
+}
+
+static int
+test_format_addr(void)
+{
+	struct rte_ether_addr rand_ea = { };
+	char buf[RTE_ETHER_ADDR_FMT_SIZE];
+	unsigned int i;
+
+	for (i = 0; i < N; i++) {
+		struct rte_ether_addr result = { };
+		int ret;
+
+		rte_eth_random_addr(rand_ea.addr_bytes);
+
+		rte_ether_format_addr(buf, sizeof(buf), &rand_ea);
+
+		ret = rte_ether_unformat_addr(buf, &result);
+		if (ret != 0) {
+			fprintf(stderr, "rte_ether_unformat_addr(%s) failed\n", buf);
+			return -1;
+		}
+		RTE_TEST_ASSERT(rte_is_same_ether_addr(&rand_ea, &result),
+			"rte_ether_format/unformat mismatch");
+	}
+	return 0;
+
+}
+
+static int
+test_unformat_addr(void)
+{
+	const struct rte_ether_addr expected = {
+		.addr_bytes = { 0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc },
+	};
+	const struct rte_ether_addr nozero_ea = {
+		.addr_bytes = { 1, 2, 3, 4, 5, 6 },
+	};
+	struct rte_ether_addr result;
+	int ret;
+
+	/* Test IETF format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("12:34:56:78:9a:bc", &result);
+	RTE_TEST_ASSERT(ret == 0, "IETF unformat failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&expected, &result),
+		"IETF unformat mismatch");
+
+	/* Test IEEE format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("12-34-56-78-9A-BC", &result);
+	RTE_TEST_ASSERT(ret == 0, "IEEE unformat failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&expected, &result),
+			"IEEE unformat mismatch");
+
+	/* Test Cisco format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("1234.5678.9ABC", &result);
+	RTE_TEST_ASSERT(ret == 0, "Cisco unformat failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&expected, &result),
+			"Cisco unformat mismatch");
+
+	/* Test no leading zeros - IETF */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("1:2:3:4:5:6", &result);
+	RTE_TEST_ASSERT(ret == 0, "IETF leading zero failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&nozero_ea, &result),
+			"IETF leading zero mismatch");
+
+	/* Test no-leading zero - IEEE format */
+	memset(&result, 0, sizeof(result));
+	ret = rte_ether_unformat_addr("1-2-3-4-5-6", &result);
+	RTE_TEST_ASSERT(ret == 0, "IEEE leading zero failed");
+	RTE_TEST_ASSERT(rte_is_same_ether_addr(&nozero_ea, &result),
+			"IEEE leading zero mismatch");
+
+
+	return 0;
+}
+
+static int
+test_invalid_addr(void)
+{
+	static const char * const invalid[] = {
+		"123",
+		"123:456",
+		"12:34:56:78:9a:gh",
+		"12:34:56:78:9a",
+		"100:34:56:78:9a:bc",
+		"34-56-78-9a-bc",
+		"12:34:56-78:9a:bc",
+		"12:34:56.78:9a:bc",
+		"123:456:789:abc",
+		"NOT.AN.ADDRESS",
+		"102.304.506",
+		"",
+	};
+	struct rte_ether_addr result;
+	unsigned int i;
+
+	for (i = 0; i < RTE_DIM(invalid); ++i) {
+		if (!rte_ether_unformat_addr(invalid[i], &result)) {
+			fprintf(stderr, "rte_ether_unformat_addr(%s) succeeded!\n",
+				invalid[i]);
+			return -1;
+		}
+	}
+	return 0;
+}
+
+static int
+test_net_ether(void)
+{
+	if (test_ether_addr())
+		return -1;
+
+	if (test_format_addr())
+		return -1;
+
+	if (test_unformat_addr())
+		return -1;
+
+	if (test_invalid_addr())
+		return -1;
+
+	return 0;
+}
+
+REGISTER_FAST_TEST(net_ether_autotest, true, true, test_net_ether);
-- 
2.39.2

[PATCH v3 4/4] net/tap: use rte_ether_unformat_address

[Stephen Hemminger]

From: David Christensen <drc@linux.vnet.ibm.com>

Building DPDK with gcc 12 on a ppc64le system generates a
stringop-overflow warning. Replace the local MAC address
validation function parse_user_mac() with a call to
rte_ether_unformat_addr() instead.

Bugzilla ID: 1197
Cc: stable@dpdk.org

Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index bf98f7555990..b25a52655fa2 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2267,29 +2267,6 @@ set_remote_iface(const char *key __rte_unused,
 	return 0;
 }
 
-static int parse_user_mac(struct rte_ether_addr *user_mac,
-		const char *value)
-{
-	unsigned int index = 0;
-	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
-
-	if (user_mac == NULL || value == NULL)
-		return 0;
-
-	strlcpy(mac_temp, value, sizeof(mac_temp));
-	mac_byte = strtok(mac_temp, ":");
-
-	while ((mac_byte != NULL) &&
-			(strlen(mac_byte) <= 2) &&
-			(strlen(mac_byte) == strspn(mac_byte,
-					ETH_TAP_CMP_MAC_FMT))) {
-		user_mac->addr_bytes[index++] = strtoul(mac_byte, NULL, 16);
-		mac_byte = strtok(NULL, ":");
-	}
-
-	return index;
-}
-
 static int
 set_mac_type(const char *key __rte_unused,
 	     const char *value,
@@ -2311,7 +2288,7 @@ set_mac_type(const char *key __rte_unused,
 		goto success;
 	}
 
-	if (parse_user_mac(user_mac, value) != 6)
+	if (rte_ether_unformat_addr(value, user_mac) < 0)
 		goto error;
 success:
 	TAP_LOG(DEBUG, "TAP user MAC param (%s)", value);
-- 
2.39.2

Re: [PATCH v3 4/4] net/tap: use rte_ether_unformat_address

[Ferruh Yigit]

On 10/3/2023 9:29 PM, Stephen Hemminger wrote:
> From: David Christensen <drc@linux.vnet.ibm.com>
> 
> Building DPDK with gcc 12 on a ppc64le system generates a
> stringop-overflow warning. Replace the local MAC address
> validation function parse_user_mac() with a call to
> rte_ether_unformat_addr() instead.
> 
> Bugzilla ID: 1197
>

For record, this should be 1195, caught by David:
Bugzilla ID: 1195

Re: [PATCH v3 0/4] rte_ether_unformat_addr related changes

[Ferruh Yigit]

On 10/3/2023 9:29 PM, Stephen Hemminger wrote:
> This patchset makes rte_ether_unformat_addr allow other formats
> for MAC address. Need to remove some inputs from existing
> cmdline_etheraddr test, and add a new test in test suite
> to cover this.  There is some overlap between the two tests
> but that is fine.
> 
> v4 - fix spelling errors
>      don't allow leading zeros in Cisco 3 part format
>      incorporate tap patch to use unformat addr
> 
> David Christensen (1):
>   net/tap: use rte_ether_unformat_address
> 
> Stephen Hemminger (3):
>   test: remove some strings from cmdline_etheraddr tests
>   rte_ether_unformat: accept more inputs
>   test: add tests for rte_ether routines
> 
>  

Thanks for the patch, appreciated.

For series,
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>


Series applied to dpdk-next-net/main, thanks.