From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Olivier MATZ <olivier.matz@6wind.com>
Cc: dev@dpdk.org, Bruce Richardson <bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH v1 6/6] net: fix rte_ether conflicts with libc
Date: Fri, 22 Dec 2017 15:25:27 +0100 [thread overview]
Message-ID: <20171222142527.GQ5377@6wind.com> (raw)
In-Reply-To: <20171222133420.64zut4nbwcuft3fs@platinum>
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?
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2017-12-22 14:25 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-21 12:59 [dpdk-dev] [PATCH v1 0/6] Address various issues with exported headers Adrien Mazarguil
2017-12-21 12:59 ` [dpdk-dev] [PATCH v1 1/6] devtools: update check-includes exceptions Adrien Mazarguil
2017-12-21 12:59 ` [dpdk-dev] [PATCH v1 2/6] net/i40e: fix issue in exported header Adrien Mazarguil
2017-12-27 6:53 ` Xing, Beilei
2017-12-21 12:59 ` [dpdk-dev] [PATCH v1 3/6] flow_classify: " Adrien Mazarguil
2018-01-02 15:19 ` Iremonger, Bernard
2017-12-21 13:00 ` [dpdk-dev] [PATCH v1 4/6] member: " Adrien Mazarguil
2017-12-21 13:00 ` [dpdk-dev] [PATCH v1 5/6] fix missing includes in exported headers Adrien Mazarguil
2017-12-21 14:12 ` Bruce Richardson
2017-12-21 14:50 ` Adrien Mazarguil
2017-12-21 16:01 ` Bruce Richardson
2017-12-21 13:00 ` [dpdk-dev] [PATCH v1 6/6] net: fix rte_ether conflicts with libc Adrien Mazarguil
2017-12-22 13:34 ` Olivier MATZ
2017-12-22 14:25 ` Adrien Mazarguil [this message]
2018-01-16 13:04 ` Olivier Matz
2018-01-16 23:19 ` Thomas Monjalon
2018-01-16 23:18 ` [dpdk-dev] [PATCH v1 0/6] Address various issues with exported headers Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171222142527.GQ5377@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).