From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f65.google.com (mail-wm0-f65.google.com [74.125.82.65]) by dpdk.org (Postfix) with ESMTP id 7CA7E1B38A for ; Fri, 22 Dec 2017 15:25:40 +0100 (CET) Received: by mail-wm0-f65.google.com with SMTP id b199so22264519wme.1 for ; Fri, 22 Dec 2017 06:25:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=S91GTFTl0fmE1Y7p2OB11pwQ98+HP/7wA/Kbjguxq3c=; b=gr8F/R9fZYtHFkj8JIsLgewOVQ66Xd/FxE7JdX6wYBp7Wyp0JWOYYwqtNiJZUd7fPn fQGfKsqFwYSflq6OsYvK7HHssZILQ6XqgRrXMFAPd7yB/2LYP0gfo7YTFNO3fR1ObMFt gJjDyGK3u0LKwzrowGJNZIHCJ7ks/+aUhnKWYaD6O268dLlm+tafr5A6DkiYozSOdLu9 XPjGLS/EKixugSezulSLm92ijaSO4iUuTGkLnCu/WoQWHpO8mZgdfmhTIr4LkRDbwtAD qt2SyQC7ahOqT9+kHFxR7Qkz07df2HrztATn5Pt9lva2LRNX9zCDNn/aq8I9P4Qd8T/t TzSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=S91GTFTl0fmE1Y7p2OB11pwQ98+HP/7wA/Kbjguxq3c=; b=npMuYqN4pD9Z0OG8dsofRNsGVX2zG956CERWXuhqBTCBi+raNu1QJmeY4HJnODS+UE rZKLScr4wAVlu3JQ7MjIM818SLqzWmXeI3ehRI9bc5hZaoRGJe+4iU0033dx8kzdms3p vaVa8EK7TtsT1OLUm8o8bJFUTaDFGjhHJFsvebvrKxC99S0dIQ2UkARusUPJvp87K5ZV TLKHeVEWrFL1z/y4sIJ2Z6gh+BED8kTuSSfeERtfTfdEkXLTgk1K1lbj1NI3/aUEFVPR 76OcW3YYea4TfaUxNTIUfpLPjQ+aCnPtM5pCUiukgjBOdlMR5YSIE0Gg+YXFxT8n4GlT Iwtg== X-Gm-Message-State: AKGB3mKsJui3mhYRfaQOCjtSbtVnKyPR2m8/ShNc9KcONcESsE+/zdK4 kzQxbtfIh3XMs/E5pZU2JiR/uw== X-Google-Smtp-Source: ACJfBovK+Ww9V+6nygWHo3c5WLyKdElEV8HPwetRT8z6rkQftFtQc1EhgZjMSJ+8qxr26mgHms4AUQ== X-Received: by 10.80.205.156 with SMTP id p28mr15434235edi.255.1513952740150; Fri, 22 Dec 2017 06:25:40 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id h56sm18761444eda.97.2017.12.22.06.25.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 22 Dec 2017 06:25:39 -0800 (PST) Date: Fri, 22 Dec 2017 15:25:27 +0100 From: Adrien Mazarguil To: Olivier MATZ Cc: dev@dpdk.org, Bruce Richardson Message-ID: <20171222142527.GQ5377@6wind.com> References: <20171221122458.811-1-adrien.mazarguil@6wind.com> <20171221122458.811-7-adrien.mazarguil@6wind.com> <20171222133420.64zut4nbwcuft3fs@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222133420.64zut4nbwcuft3fs@platinum> Subject: Re: [dpdk-dev] [PATCH v1 6/6] net: fix rte_ether conflicts with libc X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Dec 2017 14:25:40 -0000 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 > > Cc: Olivier Matz > > Cc: Bruce Richardson > > [...] > > > --- 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 > > #include > > #include > > > > @@ -52,15 +53,7 @@ extern "C" { > > #include > > #include > > > > -#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