[dpdk-dev,v1,6/6] net: fix rte_ether conflicts with libc

  Applications can't combine either net/ethernet.h or netinet/ether.h
together with rte_ether.h due to the redefinition of struct ether_addr and
various macros by the latter.

This patch adapts rte_ether.h to rely on system definitions while
maintaining DPDK additions.

An unforeseen consequence of involving more system header files compilation
issues with some base drivers (i40e, ixgbe) defining their own conflicting
types (e.g. __le64). This is addressed by explicitly including rte_ether.h
where missing to ensure system definitions always come first.

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Olivier Matz <olivier.matz@6wind.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>
---
This patch should work but wasn't validated on FreeBSD, any volunteers?
---
 drivers/net/i40e/base/i40e_osdep.h   |  1 +
 drivers/net/ixgbe/base/ixgbe_osdep.h |  1 +
 drivers/net/qede/base/bcm_osal.h     |  1 -
 lib/librte_net/rte_ether.h           | 30 ++++++++++++++++++++----------
 4 files changed, 22 insertions(+), 11 deletions(-)
  

Hi Adrien,

On Thu, Dec 21, 2017 at 02:00:06PM +0100, Adrien Mazarguil wrote:
> Applications can't combine either net/ethernet.h or netinet/ether.h
> together with rte_ether.h due to the redefinition of struct ether_addr and
> various macros by the latter.
> 
> This patch adapts rte_ether.h to rely on system definitions while
> maintaining DPDK additions.
> 
> An unforeseen consequence of involving more system header files compilation
> issues with some base drivers (i40e, ixgbe) defining their own conflicting
> types (e.g. __le64). This is addressed by explicitly including rte_ether.h
> where missing to ensure system definitions always come first.
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>
> Cc: Bruce Richardson <bruce.richardson@intel.com>

[...]

> --- a/drivers/net/qede/base/bcm_osal.h
> +++ b/drivers/net/qede/base/bcm_osal.h
> @@ -334,7 +334,6 @@ u32 qede_find_first_zero_bit(unsigned long *, u32);
>  	qede_find_first_zero_bit(bitmap, length)
>  
>  #define OSAL_BUILD_BUG_ON(cond)		nothing
> -#define ETH_ALEN			ETHER_ADDR_LEN
>  
>  #define OSAL_BITMAP_WEIGHT(bitmap, count) 0
>  

Not sure we can update code in a 'base' driver as easily.
It should be checked how it can be done with the qede maintainer.


> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 06d7b486c..19e62ea89 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -44,6 +44,7 @@
>  extern "C" {
>  #endif
>  
> +#include <net/ethernet.h>
>  #include <stdint.h>
>  #include <stdio.h>
>  
> @@ -52,15 +53,7 @@ extern "C" {
>  #include <rte_mbuf.h>
>  #include <rte_byteorder.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. */
> +#define ETHER_MTU ETHERMTU /**< Deprecated, defined for compatibility. */
>  
>  #define ETHER_MAX_VLAN_FRAME_LEN \
>  	(ETHER_MAX_LEN + 4) /**< Maximum VLAN frame length, including CRC. */
> @@ -72,8 +65,11 @@ extern "C" {
>  
>  #define ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
>  
> +#ifdef __DOXYGEN__
> +
>  /**
> - * Ethernet address:
> + * 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
> @@ -82,11 +78,25 @@ extern "C" {
>   * 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
> + *
> + * This structure is defined system-wide by "net/ethernet.h", however since
> + * the name of its data field is OS-dependent, a macro named "addr_bytes" is
> + * defined as an alias for the convenience of DPDK applications.
> + *
> + * The following definition is only for documentation purposes.
>   */
>  struct ether_addr {
>  	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
>  } __attribute__((__packed__));
>  
> +#endif /* __DOXYGEN__ */
> +
> +#if defined(__FreeBSD__)
> +#define addr_bytes octet
> +#else
> +#define addr_bytes ether_addr_octet
> +#endif
> +
>  #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
>  #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */

This kind of #define looks a bit dangerous to me: it can trigger
strange bugs because it will replace all occurences of addr_bytes
after this header is included.

Wouldn't it be a good opportunity to think about adding the rte_ prefix
to all variables/functions of rte_ether.h?
  

Hi Olivier,

On Fri, Dec 22, 2017 at 02:34:21PM +0100, Olivier MATZ wrote:
> Hi Adrien,
> 
> On Thu, Dec 21, 2017 at 02:00:06PM +0100, Adrien Mazarguil wrote:
> > Applications can't combine either net/ethernet.h or netinet/ether.h
> > together with rte_ether.h due to the redefinition of struct ether_addr and
> > various macros by the latter.
> > 
> > This patch adapts rte_ether.h to rely on system definitions while
> > maintaining DPDK additions.
> > 
> > An unforeseen consequence of involving more system header files compilation
> > issues with some base drivers (i40e, ixgbe) defining their own conflicting
> > types (e.g. __le64). This is addressed by explicitly including rte_ether.h
> > where missing to ensure system definitions always come first.
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Cc: Olivier Matz <olivier.matz@6wind.com>
> > Cc: Bruce Richardson <bruce.richardson@intel.com>
> 
> [...]
> 
> > --- a/drivers/net/qede/base/bcm_osal.h
> > +++ b/drivers/net/qede/base/bcm_osal.h
> > @@ -334,7 +334,6 @@ u32 qede_find_first_zero_bit(unsigned long *, u32);
> >  	qede_find_first_zero_bit(bitmap, length)
> >  
> >  #define OSAL_BUILD_BUG_ON(cond)		nothing
> > -#define ETH_ALEN			ETHER_ADDR_LEN
> >  
> >  #define OSAL_BITMAP_WEIGHT(bitmap, count) 0
> >  
> 
> Not sure we can update code in a 'base' driver as easily.
> It should be checked how it can be done with the qede maintainer.

Sure, although I have to send an update for this chunk already: ETH_ALEN
seems only defined in Linux, not under FreeBSD. I intend to enclose the
above within #ifdef ETH_ALEN.

Besides, updating this file shouldn't be a problem, it's already tailored
for DPDK as it includes and uses several DPDK headers.

> > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> > index 06d7b486c..19e62ea89 100644
> > --- a/lib/librte_net/rte_ether.h
> > +++ b/lib/librte_net/rte_ether.h
> > @@ -44,6 +44,7 @@
> >  extern "C" {
> >  #endif
> >  
> > +#include <net/ethernet.h>
> >  #include <stdint.h>
> >  #include <stdio.h>
> >  
> > @@ -52,15 +53,7 @@ extern "C" {
> >  #include <rte_mbuf.h>
> >  #include <rte_byteorder.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. */
> > +#define ETHER_MTU ETHERMTU /**< Deprecated, defined for compatibility. */
> >  
> >  #define ETHER_MAX_VLAN_FRAME_LEN \
> >  	(ETHER_MAX_LEN + 4) /**< Maximum VLAN frame length, including CRC. */
> > @@ -72,8 +65,11 @@ extern "C" {
> >  
> >  #define ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
> >  
> > +#ifdef __DOXYGEN__
> > +
> >  /**
> > - * Ethernet address:
> > + * 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
> > @@ -82,11 +78,25 @@ extern "C" {
> >   * 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
> > + *
> > + * This structure is defined system-wide by "net/ethernet.h", however since
> > + * the name of its data field is OS-dependent, a macro named "addr_bytes" is
> > + * defined as an alias for the convenience of DPDK applications.
> > + *
> > + * The following definition is only for documentation purposes.
> >   */
> >  struct ether_addr {
> >  	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
> >  } __attribute__((__packed__));
> >  
> > +#endif /* __DOXYGEN__ */
> > +
> > +#if defined(__FreeBSD__)
> > +#define addr_bytes octet
> > +#else
> > +#define addr_bytes ether_addr_octet
> > +#endif
> > +
> >  #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
> >  #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
> 
> This kind of #define looks a bit dangerous to me: it can trigger
> strange bugs because it will replace all occurences of addr_bytes
> after this header is included.

Understandable, I checked before settling on this macro though, there's no
other usage of addr_bytes inside DPDK.

As for applications, there's no way to be completely sure. If we consider
they have to explicitly include rte_ether.h to get this definition, there
are chances addr_bytes is exclusively used with MAC addresses.

This change results in an API change (addr_bytes now documented as a
reserved macro) but has no ABI impact. I think it's a rather harmless
workaround pending your next suggestion:

> Wouldn't it be a good opportunity to think about adding the rte_ prefix
> to all variables/functions of rte_ether.h?

That would be ideal, however since almost all definitions in librte_net
(rte_ip.h, rte_udp.h etc.) also lack this prefix and can still technically
trigger conflicts with outside definitions (I'm aware work was done to avoid
that, but it doesn't change the fact they're not in the official DPDK
namespace), a massive API change would be needed for consistency.

Such a change is unlikely to be accepted for 18.02 in any case, therefore if
the proposed workaround is deemed too risky, I offer to remove this patch
from the series. What do you suggest?
  

Hi Adrien,

On Fri, Dec 22, 2017 at 03:25:27PM +0100, Adrien Mazarguil wrote:
> On Fri, Dec 22, 2017 at 02:34:21PM +0100, Olivier MATZ wrote:
> > On Thu, Dec 21, 2017 at 02:00:06PM +0100, Adrien Mazarguil wrote:

...

> > > +#if defined(__FreeBSD__)
> > > +#define addr_bytes octet
> > > +#else
> > > +#define addr_bytes ether_addr_octet
> > > +#endif
> > > +
> > >  #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
> > >  #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
> > 
> > This kind of #define looks a bit dangerous to me: it can trigger
> > strange bugs because it will replace all occurences of addr_bytes
> > after this header is included.
> 
> Understandable, I checked before settling on this macro though, there's no
> other usage of addr_bytes inside DPDK.
> 
> As for applications, there's no way to be completely sure. If we consider
> they have to explicitly include rte_ether.h to get this definition, there
> are chances addr_bytes is exclusively used with MAC addresses.
> 
> This change results in an API change (addr_bytes now documented as a
> reserved macro) but has no ABI impact. I think it's a rather harmless
> workaround pending your next suggestion:
> 
> > Wouldn't it be a good opportunity to think about adding the rte_ prefix
> > to all variables/functions of rte_ether.h?
> 
> That would be ideal, however since almost all definitions in librte_net
> (rte_ip.h, rte_udp.h etc.) also lack this prefix and can still technically
> trigger conflicts with outside definitions (I'm aware work was done to avoid
> that, but it doesn't change the fact they're not in the official DPDK
> namespace), a massive API change would be needed for consistency.
> 
> Such a change is unlikely to be accepted for 18.02 in any case, therefore if
> the proposed workaround is deemed too risky, I offer to remove this patch
> from the series. What do you suggest?

I think the only good long term solution is to have a proper namespace
for all rte_net structures and definitions. If there is no blocking issue
requiring this patch now, we can consider the following:

- 18.02: announce an api change for 18.05
- 18.05:
  - add new net structures and definitions with rte_ prefix
  - mark the old ones as deprecated
  - removes the structures or definitions that conflicts with system headers
- 18.08: remove the old structures and definitions
  

22/12/2017 15:25, Adrien Mazarguil:
> Such a change is unlikely to be accepted for 18.02 in any case, therefore if
> the proposed workaround is deemed too risky, I offer to remove this patch
> from the series. What do you suggest?

Series applied without this patch.

Please let's fix it in 18.05.