From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 49F64A056D; Wed, 3 Mar 2021 20:30:25 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C939140692; Wed, 3 Mar 2021 20:30:24 +0100 (CET) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mails.dpdk.org (Postfix) with ESMTP id B654D40683 for ; Wed, 3 Mar 2021 20:30:22 +0100 (CET) IronPort-SDR: 0lZ42kXd4OBD8iClgS3XEnVu9wpvLDO37WqqGlbJuws2BV/7sio+TS+T6Q78jjRXO9+vwhdbqz 0Ov5U7xxU0nQ== X-IronPort-AV: E=McAfee;i="6000,8403,9912"; a="167163102" X-IronPort-AV: E=Sophos;i="5.81,220,1610438400"; d="scan'208";a="167163102" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Mar 2021 11:30:18 -0800 IronPort-SDR: vzYJxiMdA9BoPy+hwJejeMRnvYBgeoZfD8fFUR7LMLDxRKezKRK4SeDj6ByPACB7T5LOhN7REt /dWZ1BC446Qw== X-IronPort-AV: E=Sophos;i="5.81,220,1610438400"; d="scan'208";a="506998881" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.9.23]) ([10.252.9.23]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Mar 2021 11:30:16 -0800 To: Dmitry Kozlyuk , Tyler Retzlaff Cc: Nick Connolly , dev@dpdk.org, Mike Wells , Narcisa Ana Maria Vasile , Dmitry Malloy , Pallavi Kadam , Jie Zhou References: <20210214012013.23165-1-dmitry.kozliuk@gmail.com> <20210214021616.26970-1-dmitry.kozliuk@gmail.com> <20210214021616.26970-5-dmitry.kozliuk@gmail.com> <6c5e9d34-abfa-d808-eef0-165315baf3ce@intel.com> <20210225220440.2bda87c3@sovereign> <20210226021001.73829588@sovereign> <826eb0d3-3c0b-7581-99be-77e9bb6e8fa2@mayadata.io> <20210302020559.6d3317d7@sovereign> <65bd9188-0fcd-ab0c-2434-6a298be19b1a@mayadata.io> <20210303193240.2b83d9c9@sovereign> <20210303211920.59ad403d@sovereign> From: Ferruh Yigit X-User: ferruhy Message-ID: Date: Wed, 3 Mar 2021 19:30:11 +0000 MIME-Version: 1.0 In-Reply-To: <20210303211920.59ad403d@sovereign> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 4/6] net/pcap: add libpcap wrappers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 3/3/2021 6:19 PM, Dmitry Kozlyuk wrote: > 2021-03-03 16:47, Ferruh Yigit: >> On 3/3/2021 4:32 PM, Dmitry Kozlyuk wrote: > [...] >>> If we can't help including / from public headers, >>> might as well use `struct in_addr`, just replace `#include ` >>> with `#include ` everywhere. >>> >>> The only remaining issue will be `s_addr` macro on Windows that conflicts with >>> `struct rte_ether_addr` field `s_addr`. I don't know a better solution than >>> renaming `s_addr` to `src_addr` in DPDK (OTOH, it will be consistent with >>> `struct rte_ipvX_hdr` field naming). >>> >>> Ferruh, what do you think? >>> >> >> No problem on the chosen name, that will work fine, but the 'struct >> rte_eth_addr' is public struct, we can't rename it without breaking the user >> applications. >> >> Still it is possible to rename public struct, but it is a long process, need to >> send a deprecation notice and wait for next LTS, 21.11. >> >> Ethernet header already has following, doesn't it help: >> #undef s_addr /* Defined in winsock2.h included in windows.h. */ > > It helps until you do the following: > > #include /* #define s_addr S_un.S_addr */ > #include /* #undef s_addr */ > > struct in_addr a; > a.s_addr = 0; /* ERROR, `s_addr` has been defined to do that */ > > Or the following: > > #include > #include > > struct rte_ether_hdr eh; > eh.s_addr.addr_bytes[0] = 0; /* ERROR: `addr_s` is a macro */ > > If you swap include order in each case, it compiles, i.e . code is fragile. > Shims redefine `struct in_addr` and we're trying to get rid of them anyway. > Got it, thanks for clarification. > > Here's a solution that always works, however ugly it looks. It defines > `struct rte_ether_hdr` for Windows such that `s_addr` macro works for it if > defined. We can keep it until 21.11, then remove it and rename fields. In > return, we can remove networking shims and workarounds immediately. > Agree it looks ugly :), but I am OK with above plan. You need to send a deprecation notice for this. We can discuss there if 'd_addr' also should be renamed for consistency, or leave with the workaround instead of rename ... > `S_un.S_addr` part of `struct in_addr` is documented, and thus is unlikely to > ever change: > > https://docs.microsoft.com/en-us/windows/win32/api/winsock2/ns-winsock2-in_addr > > > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h > index 060b63fc9..67d340bb2 100644 > --- a/lib/librte_net/rte_ether.h > +++ b/lib/librte_net/rte_ether.h > @@ -23,10 +23,6 @@ extern "C" { > #include > #include > > -#ifdef RTE_EXEC_ENV_WINDOWS /* Workaround conflict with rte_ether_hdr. */ > -#undef s_addr /* Defined in winsock2.h included in windows.h. */ > -#endif > - > #define RTE_ETHER_ADDR_LEN 6 /**< Length of Ethernet address. */ > #define RTE_ETHER_TYPE_LEN 2 /**< Length of Ethernet type field. */ > #define RTE_ETHER_CRC_LEN 4 /**< Length of Ethernet CRC. */ > @@ -257,6 +253,8 @@ __rte_experimental > int > rte_ether_unformat_addr(const char *str, struct rte_ether_addr *eth_addr); > > +#if !defined RTE_EXEC_ENV_WINDOWS || defined __DOXYGEN__ > + > /** > * Ethernet header: Contains the destination address, source address > * and frame type. > @@ -267,6 +265,28 @@ struct rte_ether_hdr { > uint16_t ether_type; /**< Frame type. */ > } __rte_aligned(2); > > +#else > + > +#pragma push_macro("s_addr") > +#ifdef s_addr > +#undef s_addr > +#endif I guess this needs to be as following, in case this header included before the windows header: #ifdef s_addr #pragma push_macro("s_addr") #undef s_addr #endif > + > +struct rte_ether_hdr { > + struct rte_ether_addr d_addr; /**< Destination address. */ > + RTE_STD_C11 > + union { > + struct rte_ether_addr s_addr; /**< Source address. */ > + struct { > + struct rte_ether_addr S_un; > + } S_addr; > + }; > + uint16_t ether_type; /**< Frame type. */ > +} __rte_aligned(2); > + > +#pragma pop_macro("s_addr") > +#endif > + > /** > * Ethernet VLAN Header. > * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet type >