[dpdk-dev] [RFC] resolve conflict between net/ethernet.h and rte_ethdev.h

[dpdk-dev] [RFC] resolve conflict between net/ethernet.h and rte_ethdev.h

[Stephen Hemminger]

This is a patch to address the conflict between <net/ethernet.h>
and the definitions in <rte_ethdev.h>. It has two side effects
worth discussion:
  1. It forces inclusion of net/ethernet.h
  2. It has definition to deal with the differing structure elements
     in the two versions of struct ether_addr.

By doing this ether_ntoa and related functions can be used without
messing with prototypes.

Alternative is more complex #ifdef magic like linux/libc-compat.h


--- a/lib/librte_ether/rte_ether.h
+++ b/lib/librte_ether/rte_ether.h
@@ -46,17 +46,11 @@
 
 #include <stdint.h>
 #include <stdio.h>
+#include <net/ethernet.h>
 
 #include <rte_memcpy.h>
 #include <rte_random.h>
 
-#define ETHER_ADDR_LEN  6 /**< Length of Ethernet address. */
-#define ETHER_TYPE_LEN  2 /**< Length of Ethernet type field. */
-#define ETHER_CRC_LEN   4 /**< Length of Ethernet CRC. */
-#define ETHER_HDR_LEN   \
-	(ETHER_ADDR_LEN * 2 + ETHER_TYPE_LEN) /**< Length of Ethernet header. */
-#define ETHER_MIN_LEN   64    /**< Minimum frame len, including CRC. */
-#define ETHER_MAX_LEN   1518  /**< Maximum frame len, including CRC. */
 #define ETHER_MTU       \
 	(ETHER_MAX_LEN - ETHER_HDR_LEN - ETHER_CRC_LEN) /**< Ethernet MTU. */
 
@@ -69,25 +63,12 @@
 #define ETHER_MAX_VLAN_ID  4095 /**< Maximum VLAN ID. */
 
 #define ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
-
-/**
- * Ethernet address:
- * A universally administered address is uniquely assigned to a device by its
- * manufacturer. The first three octets (in transmission order) contain the
- * Organizationally Unique Identifier (OUI). The following three (MAC-48 and
- * EUI-48) octets are assigned by that organization with the only constraint
- * of uniqueness.
- * A locally administered address is assigned to a device by a network
- * administrator and does not contain OUIs.
- * See http://standards.ieee.org/regauth/groupmac/tutorial.html
- */
-struct ether_addr {
-	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Address bytes in transmission order */
-} __attribute__((__packed__));
-
 #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
 #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
 
+/* Deprecated definition to allow for compatiablity with net/ethernet.h */
+#define addr_bytes	ether_addr_octet
+
 /**
  * Check if two Ethernet addresses are the same.
  *
@@ -107,7 +88,7 @@
 {
 	int i;
 	for (i = 0; i < ETHER_ADDR_LEN; i++)
-		if (ea1->addr_bytes[i] != ea2->addr_bytes[i])
+		if (ea1->ether_addr_octet[i] != ea2->ether_addr_octet[i])
 			return 0;
 	return 1;
 }
@@ -126,7 +107,7 @@
 {
 	int i;
 	for (i = 0; i < ETHER_ADDR_LEN; i++)
-		if (ea->addr_bytes[i] != 0x00)
+		if (ea->ether_addr_octet[i] != 0x00)
 			return 0;
 	return 1;
 }
@@ -143,7 +124,7 @@
  */
 static inline int is_unicast_ether_addr(const struct ether_addr *ea)
 {
-	return ((ea->addr_bytes[0] & ETHER_GROUP_ADDR) == 0);
+	return ((ea->ether_addr_octet[0] & ETHER_GROUP_ADDR) == 0);
 }
 
 /**
@@ -158,7 +139,7 @@
  */
 static inline int is_multicast_ether_addr(const struct ether_addr *ea)
 {
-	return (ea->addr_bytes[0] & ETHER_GROUP_ADDR);
+	return (ea->ether_addr_octet[0] & ETHER_GROUP_ADDR);
 }
 
 /**
@@ -191,7 +172,7 @@
  */
 static inline int is_universal_ether_addr(const struct ether_addr *ea)
 {
-	return ((ea->addr_bytes[0] & ETHER_LOCAL_ADMIN_ADDR) == 0);
+	return ((ea->ether_addr_octet[0] & ETHER_LOCAL_ADMIN_ADDR) == 0);
 }
 
 /**
@@ -206,7 +187,7 @@
  */
 static inline int is_local_admin_ether_addr(const struct ether_addr *ea)
 {
-	return ((ea->addr_bytes[0] & ETHER_LOCAL_ADMIN_ADDR) != 0);
+	return ((ea->ether_addr_octet[0] & ETHER_LOCAL_ADMIN_ADDR) != 0);
 }
 
 /**
@@ -253,8 +234,8 @@
 				   struct ether_addr *ea_to)
 {
 #ifdef __INTEL_COMPILER
-	uint16_t *from_words = (uint16_t *)(ea_from->addr_bytes);
-	uint16_t *to_words   = (uint16_t *)(ea_to->addr_bytes);
+	uint16_t *from_words = (uint16_t *)(ea_from->ether_addr_octet);
+	uint16_t *to_words   = (uint16_t *)(ea_to->ether_addr_octet);
 
 	to_words[0] = from_words[0];
 	to_words[1] = from_words[1];
@@ -283,12 +264,12 @@
 		  const struct ether_addr *eth_addr)
 {
 	snprintf(buf, size, "%02X:%02X:%02X:%02X:%02X:%02X",
-		 eth_addr->addr_bytes[0],
-		 eth_addr->addr_bytes[1],
-		 eth_addr->addr_bytes[2],
-		 eth_addr->addr_bytes[3],
-		 eth_addr->addr_bytes[4],
-		 eth_addr->addr_bytes[5]);
+		 eth_addr->ether_addr_octet[0],
+		 eth_addr->ether_addr_octet[1],
+		 eth_addr->ether_addr_octet[2],
+		 eth_addr->ether_addr_octet[3],
+		 eth_addr->ether_addr_octet[4],
+		 eth_addr->ether_addr_octet[5]);
 }
 
 /**

Re: [dpdk-dev] [RFC] resolve conflict between net/ethernet.h and rte_ethdev.h

[Neil Horman]

On Sat, Dec 27, 2014 at 03:13:00PM -0800, Stephen Hemminger wrote:
> This is a patch to address the conflict between <net/ethernet.h>
> and the definitions in <rte_ethdev.h>. It has two side effects
> worth discussion:
>   1. It forces inclusion of net/ethernet.h
>   2. It has definition to deal with the differing structure elements
>      in the two versions of struct ether_addr.
> 
> By doing this ether_ntoa and related functions can be used without
> messing with prototypes.
> 
> Alternative is more complex #ifdef magic like linux/libc-compat.h
> 
> 
This seems like a reasonable fix for the problem, and nasty compat-work

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [dpdk-dev] [RFC] resolve conflict between net/ethernet.h and rte_ethdev.h

[Thomas Monjalon]

2014-12-27 15:13, Stephen Hemminger:
> This is a patch to address the conflict between <net/ethernet.h>
> and the definitions in <rte_ethdev.h>. It has two side effects
> worth discussion:
>   1. It forces inclusion of net/ethernet.h
>   2. It has definition to deal with the differing structure elements
>      in the two versions of struct ether_addr.
> 
> By doing this ether_ntoa and related functions can be used without
> messing with prototypes.
> 
> Alternative is more complex #ifdef magic like linux/libc-compat.h

[...]

> +#include <net/ethernet.h>

[...]

> +/* Deprecated definition to allow for compatiablity with net/ethernet.h */
> +#define addr_bytes	ether_addr_octet

This is defining a common identifier without prefix.
So it will be forbidden to use addr_bytes as variable name.
I understand you are trying to keep compatibility with both structures,
but the drawback is really nasty.
Is there another solution? Or at least, we could mark it as deprecated and
remove it in release 2.1.

-- 
Thomas

Re: [dpdk-dev] [RFC] resolve conflict between net/ethernet.h and rte_ethdev.h

[Thomas Monjalon]

2015-01-06 11:44, Thomas Monjalon:
> 2014-12-27 15:13, Stephen Hemminger:
> > This is a patch to address the conflict between <net/ethernet.h>
> > and the definitions in <rte_ethdev.h>. It has two side effects
> > worth discussion:
> >   1. It forces inclusion of net/ethernet.h
> >   2. It has definition to deal with the differing structure elements
> >      in the two versions of struct ether_addr.
> > 
> > By doing this ether_ntoa and related functions can be used without
> > messing with prototypes.
> > 
> > Alternative is more complex #ifdef magic like linux/libc-compat.h
> 
> [...]
> 
> > +#include <net/ethernet.h>
> 
> [...]
> 
> > +/* Deprecated definition to allow for compatiablity with net/ethernet.h */
> > +#define addr_bytes	ether_addr_octet
> 
> This is defining a common identifier without prefix.
> So it will be forbidden to use addr_bytes as variable name.
> I understand you are trying to keep compatibility with both structures,
> but the drawback is really nasty.
> Is there another solution? Or at least, we could mark it as deprecated and
> remove it in release 2.1.

ping
Any opinion?

Re: [dpdk-dev] [RFC] resolve conflict between net/ethernet.h and rte_ethdev.h

[Thomas Monjalon]

2015-03-05 00:16, Thomas Monjalon:
> 2015-01-06 11:44, Thomas Monjalon:
> > 2014-12-27 15:13, Stephen Hemminger:
> > > This is a patch to address the conflict between <net/ethernet.h>
> > > and the definitions in <rte_ethdev.h>. It has two side effects
> > > worth discussion:
> > >   1. It forces inclusion of net/ethernet.h
> > >   2. It has definition to deal with the differing structure elements
> > >      in the two versions of struct ether_addr.
> > > 
> > > By doing this ether_ntoa and related functions can be used without
> > > messing with prototypes.
> > > 
> > > Alternative is more complex #ifdef magic like linux/libc-compat.h
> > 
> > [...]
> > 
> > > +#include <net/ethernet.h>
> > 
> > [...]
> > 
> > > +/* Deprecated definition to allow for compatiablity with net/ethernet.h */
> > > +#define addr_bytes	ether_addr_octet
> > 
> > This is defining a common identifier without prefix.
> > So it will be forbidden to use addr_bytes as variable name.
> > I understand you are trying to keep compatibility with both structures,
> > but the drawback is really nasty.
> > Is there another solution? Or at least, we could mark it as deprecated and
> > remove it in release 2.1.
> 
> ping
> Any opinion?

Hi Stephen,
If, by any chance, you are willing to reply to this thread,
maybe you would like to send a non-rfc patch with these 2 additions:
- rename addr_bytes to ether_addr_octet everywhere
- mark addr_bytes as deprecated in doc/guides/rel_notes/abi.rst

Re: [dpdk-dev] [RFC] resolve conflict between net/ethernet.h and rte_ethdev.h

[Stephen Hemminger]

On Tue, 10 Mar 2015 14:29:11 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> Hi Stephen,
> If, by any chance, you are willing to reply to this thread,
> maybe you would like to send a non-rfc patch with these 2 additions:
> - rename addr_bytes to ether_addr_octet everywhere
> - mark addr_bytes as deprecated in doc/guides/rel_notes/abi.rst

I can respin with the global changes, just not a big fan of deprecation
since it only pisses off users.