Re: [dpdk-dev] [PATCH v2 4/6] net/pcap: add libpcap wrappers

[Ferruh Yigit <ferruh.yigit@intel.com>]

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 <ws2tcpip.h>/<netinet/ip.h> from public headers,
>>> might as well use `struct in_addr`, just replace `#include <netinet/ip.h>`
>>> with `#include <rte_ip.h>` 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 <winsock2.h>	/* #define s_addr S_un.S_addr */
> 	#include <rte_ether.h>	/* #undef s_addr */
> 
> 	struct in_addr a;
> 	a.s_addr = 0; /* ERROR, `s_addr` has been defined to do that */
> 
> Or the following:
> 
> 	#include <rte_ether.h>
> 	#include <winsock2.h>
> 
> 	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 <rte_mbuf.h>
>   #include <rte_byteorder.h>
>   
> -#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
>