doc: announce renaming of rte_ether_hdr fields
Checks
Commit Message
It is proposed to rename fields of `struct rte_ether_hdr`,
`s_addr` tp `src_addr` and `d_addr` to `dst_addr`,
due to the clash with system macro on Windows.
Until remaining is done in 21.11, a workaround can be used.
Windows Sockets headers contain `#define s_addr S_un.S_addr`, which
conflicts with `s_addr` field of `struct rte_ether_hdr`. Undefining
this macro in <rte_ether.h> is breaking some usages of DPDK
and Windows headers in one file.
Example 1:
#include <winsock2.h>
#include <rte_ether.h>
struct in_addr addr;
/* addr.s_addr = 0; ERROR: s_addr undefined by DPDK */
Example 2:
#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 */
It is not mandatory to rename `d_addr`, this is for consistency only.
Naming in `rte_ether_hdr` will also resemble `rte_ipv4/6_hdr`.
Workaround is to define `struct rte_ether_hdr` in such a away that
it can be used with or without `s_addr` macro (as defined on Windows)
This can be done for Windows only or for all platforms to save space.
#pragma push_macro("s_addr")
#ifdef 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;
/**< MUST NOT be used directly, only via s_addr */
} S_addr;
/*< MUST NOT be used directly, only via s_addr */
};
uint16_t ether_type; /**< Frame type. */
} __rte_aligned(2);
#pragma pop_macro("s_addr")
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
doc/guides/rel_notes/deprecation.rst | 4 ++++
1 file changed, 4 insertions(+)
Comments
> +
> +* net: ``s_addr`` and ``d_addr`` fields of ``rte_ether_hdr`` structure
> + will be renamed to ``src_addr`` and ``dst_addr`` respectively in DPDK 20.11
> + in order to avoid conflict with Windows Sockets headers.
If those fields were a problem now, there might be others in future.
Don't use src_addr and dst_addr because those are already used in rte_ipv4_hdr.
Linux and FreeBSD use:
struct ether_header
{
uint8_t ether_dhost[ETH_ALEN]; /* destination eth addr */
uint8_t ether_shost[ETH_ALEN]; /* source ether addr */
uint16_t ether_type; /* packet type ID field */
} __attribute__ ((__packed__));
So why not ether_dhost/ether_shost?
2021-03-03 15:54, Stephen Hemminger:
> > +
> > +* net: ``s_addr`` and ``d_addr`` fields of ``rte_ether_hdr`` structure
> > + will be renamed to ``src_addr`` and ``dst_addr`` respectively in DPDK 20.11
> > + in order to avoid conflict with Windows Sockets headers.
>
> If those fields were a problem now, there might be others in future.
One I can think of is `min` and `max` macros from `windows.h`: they are used
as field names in `rte_compressdev.h` and `rte_cryptodev.h` (and more than
once have they been worked around in PMD code, see i40e and ice patches).
Do you prefer a single notice for all such conflicts we can identify now?
> Don't use src_addr and dst_addr because those are already used in rte_ipv4_hdr.
Not sure what DPDK policy is: `rte_ipv4/6_hdr` use completely custom names,
while `rte_arp_hdr` uses traditional names with `arp_` prefix.
Coming from C++, I chose the former approach, but it's not a strong opinion.
> Linux and FreeBSD use:
>
> struct ether_header
> {
> uint8_t ether_dhost[ETH_ALEN]; /* destination eth addr */
> uint8_t ether_shost[ETH_ALEN]; /* source ether addr */
> uint16_t ether_type; /* packet type ID field */
> } __attribute__ ((__packed__));
>
> So why not ether_dhost/ether_shost?
Works for me, let's see what others think.
On 3/3/2021 10:51 PM, Dmitry Kozlyuk wrote:
> It is proposed to rename fields of `struct rte_ether_hdr`,
> `s_addr` tp `src_addr` and `d_addr` to `dst_addr`,
s/tp/to/
> due to the clash with system macro on Windows.
> Until remaining is done in 21.11, a workaround can be used.
s/remaining/renaming/ ??
>
> Windows Sockets headers contain `#define s_addr S_un.S_addr`, which
> conflicts with `s_addr` field of `struct rte_ether_hdr`. Undefining
> this macro in <rte_ether.h> is breaking some usages of DPDK
> and Windows headers in one file.
>
> Example 1:
>
> #include <winsock2.h>
> #include <rte_ether.h>
> struct in_addr addr;
> /* addr.s_addr = 0; ERROR: s_addr undefined by DPDK */
>
> Example 2:
>
> #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 */
s/addr_s/s_addr/ ??
>
> It is not mandatory to rename `d_addr`, this is for consistency only.
> Naming in `rte_ether_hdr` will also resemble `rte_ipv4/6_hdr`.
>
> Workaround is to define `struct rte_ether_hdr` in such a away that
> it can be used with or without `s_addr` macro (as defined on Windows)
> This can be done for Windows only or for all platforms to save space.
>
> #pragma push_macro("s_addr")
> #ifdef 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;
> /**< MUST NOT be used directly, only via s_addr */
> } S_addr;
> /*< MUST NOT be used directly, only via s_addr */
> };
> uint16_t ether_type; /**< Frame type. */
> } __rte_aligned(2);
>
> #pragma pop_macro("s_addr")
>
What is the problem with the workaround, why we can't live with it?
It requires an order in include files, right?
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> doc/guides/rel_notes/deprecation.rst | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 82c1a90a3..f7be10543 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -125,3 +125,7 @@ Deprecation Notices
> * cmdline: ``cmdline`` structure will be made opaque to hide platform-specific
> content. On Linux and FreeBSD, supported prior to DPDK 20.11,
> original structure will be kept until DPDK 21.11.
> +
> +* net: ``s_addr`` and ``d_addr`` fields of ``rte_ether_hdr`` structure
> + will be renamed to ``src_addr`` and ``dst_addr`` respectively in DPDK 20.11
v21.11
> + in order to avoid conflict with Windows Sockets headers.
>
On 3/4/2021 7:09 AM, Dmitry Kozlyuk wrote:
> 2021-03-03 15:54, Stephen Hemminger:
>>> +
>>> +* net: ``s_addr`` and ``d_addr`` fields of ``rte_ether_hdr`` structure
>>> + will be renamed to ``src_addr`` and ``dst_addr`` respectively in DPDK 20.11
>>> + in order to avoid conflict with Windows Sockets headers.
>>
>> If those fields were a problem now, there might be others in future.
>
> One I can think of is `min` and `max` macros from `windows.h`: they are used
> as field names in `rte_compressdev.h` and `rte_cryptodev.h` (and more than
> once have they been worked around in PMD code, see i40e and ice patches).
> Do you prefer a single notice for all such conflicts we can identify now?
>
>> Don't use src_addr and dst_addr because those are already used in rte_ipv4_hdr.
>
Why src_addr being used in rte_ipv4_hdr is a problem?
> Not sure what DPDK policy is: `rte_ipv4/6_hdr` use completely custom names,
> while `rte_arp_hdr` uses traditional names with `arp_` prefix.
> Coming from C++, I chose the former approach, but it's not a strong opinion.
>
I am now aware of a DPDK policy for it, but +1 to former approach.
>> Linux and FreeBSD use:
>>
>> struct ether_header
>> {
>> uint8_t ether_dhost[ETH_ALEN]; /* destination eth addr */
>> uint8_t ether_shost[ETH_ALEN]; /* source ether addr */
>> uint16_t ether_type; /* packet type ID field */
>> } __attribute__ ((__packed__));
>>
>> So why not ether_dhost/ether_shost?
>
> Works for me, let's see what others think.
>
2021-05-20 15:24 (UTC+0100), Ferruh Yigit:
> On 3/3/2021 10:51 PM, Dmitry Kozlyuk wrote:
[...]
> >
> > It is not mandatory to rename `d_addr`, this is for consistency only.
> > Naming in `rte_ether_hdr` will also resemble `rte_ipv4/6_hdr`.
> >
> > Workaround is to define `struct rte_ether_hdr` in such a away that
> > it can be used with or without `s_addr` macro (as defined on Windows)
> > This can be done for Windows only or for all platforms to save space.
> >
> > #pragma push_macro("s_addr")
> > #ifdef 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;
> > /**< MUST NOT be used directly, only via s_addr */
> > } S_addr;
> > /*< MUST NOT be used directly, only via s_addr */
> > };
> > uint16_t ether_type; /**< Frame type. */
> > } __rte_aligned(2);
> >
> > #pragma pop_macro("s_addr")
> >
>
> What is the problem with the workaround, why we can't live with it?
>
> It requires an order in include files, right?
There's no problem except a tricky structure definition with fields that
violate DPDK coding rules. It works with any include order.
Will fix typos in v3, thanks.
On 5/20/2021 4:06 PM, Dmitry Kozlyuk wrote:
> 2021-05-20 15:24 (UTC+0100), Ferruh Yigit:
>> On 3/3/2021 10:51 PM, Dmitry Kozlyuk wrote:
> [...]
>>>
>>> It is not mandatory to rename `d_addr`, this is for consistency only.
>>> Naming in `rte_ether_hdr` will also resemble `rte_ipv4/6_hdr`.
>>>
>>> Workaround is to define `struct rte_ether_hdr` in such a away that
>>> it can be used with or without `s_addr` macro (as defined on Windows)
>>> This can be done for Windows only or for all platforms to save space.
>>>
>>> #pragma push_macro("s_addr")
>>> #ifdef 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;
>>> /**< MUST NOT be used directly, only via s_addr */
>>> } S_addr;
>>> /*< MUST NOT be used directly, only via s_addr */
>>> };
>>> uint16_t ether_type; /**< Frame type. */
>>> } __rte_aligned(2);
>>>
>>> #pragma pop_macro("s_addr")
>>>
>>
>> What is the problem with the workaround, why we can't live with it?
>>
>> It requires an order in include files, right?
>
> There's no problem except a tricky structure definition with fields that
> violate DPDK coding rules. It works with any include order.
>
> Will fix typos in v3, thanks.
>
For following case, won't compiler take 's_addr' as macro?
#include <rte_ether.h>
#include <winsock2.h>
struct rte_ether_hdr eh;
/* eh.s_addr.addr_bytes[0] = 0;
2021-05-20 16:27 (UTC+0100), Ferruh Yigit:
> On 5/20/2021 4:06 PM, Dmitry Kozlyuk wrote:
> > 2021-05-20 15:24 (UTC+0100), Ferruh Yigit:
> >> On 3/3/2021 10:51 PM, Dmitry Kozlyuk wrote:
> > [...]
> >>>
> >>> It is not mandatory to rename `d_addr`, this is for consistency only.
> >>> Naming in `rte_ether_hdr` will also resemble `rte_ipv4/6_hdr`.
> >>>
> >>> Workaround is to define `struct rte_ether_hdr` in such a away that
> >>> it can be used with or without `s_addr` macro (as defined on Windows)
> >>> This can be done for Windows only or for all platforms to save space.
> >>>
> >>> #pragma push_macro("s_addr")
> >>> #ifdef 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;
> >>> /**< MUST NOT be used directly, only via s_addr */
> >>> } S_addr;
> >>> /*< MUST NOT be used directly, only via s_addr */
> >>> };
> >>> uint16_t ether_type; /**< Frame type. */
> >>> } __rte_aligned(2);
> >>>
> >>> #pragma pop_macro("s_addr")
> >>>
> >>
> >> What is the problem with the workaround, why we can't live with it?
> >>
> >> It requires an order in include files, right?
> >
> > There's no problem except a tricky structure definition with fields that
> > violate DPDK coding rules. It works with any include order.
> >
> > Will fix typos in v3, thanks.
> >
>
> For following case, won't compiler take 's_addr' as macro?
>
> #include <rte_ether.h>
> #include <winsock2.h>
> struct rte_ether_hdr eh;
> /* eh.s_addr.addr_bytes[0] = 0;
>
Yes, it will. The macro will expand to `S_addr.S_un` and compile successfully.
In theory, Microsoft can change the definition of `s_addr`, and while I doubt
they will, it's a valid concern and a reason to remove workaround in 21.11.
On 5/20/2021 4:50 PM, Dmitry Kozlyuk wrote:
> 2021-05-20 16:27 (UTC+0100), Ferruh Yigit:
>> On 5/20/2021 4:06 PM, Dmitry Kozlyuk wrote:
>>> 2021-05-20 15:24 (UTC+0100), Ferruh Yigit:
>>>> On 3/3/2021 10:51 PM, Dmitry Kozlyuk wrote:
>>> [...]
>>>>>
>>>>> It is not mandatory to rename `d_addr`, this is for consistency only.
>>>>> Naming in `rte_ether_hdr` will also resemble `rte_ipv4/6_hdr`.
>>>>>
>>>>> Workaround is to define `struct rte_ether_hdr` in such a away that
>>>>> it can be used with or without `s_addr` macro (as defined on Windows)
>>>>> This can be done for Windows only or for all platforms to save space.
>>>>>
>>>>> #pragma push_macro("s_addr")
>>>>> #ifdef 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;
>>>>> /**< MUST NOT be used directly, only via s_addr */
>>>>> } S_addr;
>>>>> /*< MUST NOT be used directly, only via s_addr */
>>>>> };
>>>>> uint16_t ether_type; /**< Frame type. */
>>>>> } __rte_aligned(2);
>>>>>
>>>>> #pragma pop_macro("s_addr")
>>>>>
>>>>
>>>> What is the problem with the workaround, why we can't live with it?
>>>>
>>>> It requires an order in include files, right?
>>>
>>> There's no problem except a tricky structure definition with fields that
>>> violate DPDK coding rules. It works with any include order.
>>>
>>> Will fix typos in v3, thanks.
>>>
>>
>> For following case, won't compiler take 's_addr' as macro?
>>
>> #include <rte_ether.h>
>> #include <winsock2.h>
>> struct rte_ether_hdr eh;
>> /* eh.s_addr.addr_bytes[0] = 0;
>>
>
> Yes, it will. The macro will expand to `S_addr.S_un` and compile successfully.
will 'eh.S_addr.S_un.addr_bytes[0] = 0;' compile successfully?
> In theory, Microsoft can change the definition of `s_addr`, and while I doubt
> they will, it's a valid concern and a reason to remove workaround in 21.11.
>
2021-05-20 17:04 (UTC+0100), Ferruh Yigit:
> On 5/20/2021 4:50 PM, Dmitry Kozlyuk wrote:
> > 2021-05-20 16:27 (UTC+0100), Ferruh Yigit:
> >> On 5/20/2021 4:06 PM, Dmitry Kozlyuk wrote:
> >>> 2021-05-20 15:24 (UTC+0100), Ferruh Yigit:
> >>>> On 3/3/2021 10:51 PM, Dmitry Kozlyuk wrote:
> >>> [...]
> >>>>>
> >>>>> It is not mandatory to rename `d_addr`, this is for consistency only.
> >>>>> Naming in `rte_ether_hdr` will also resemble `rte_ipv4/6_hdr`.
> >>>>>
> >>>>> Workaround is to define `struct rte_ether_hdr` in such a away that
> >>>>> it can be used with or without `s_addr` macro (as defined on Windows)
> >>>>> This can be done for Windows only or for all platforms to save space.
> >>>>>
> >>>>> #pragma push_macro("s_addr")
> >>>>> #ifdef 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;
> >>>>> /**< MUST NOT be used directly, only via s_addr */
> >>>>> } S_addr;
> >>>>> /*< MUST NOT be used directly, only via s_addr */
> >>>>> };
> >>>>> uint16_t ether_type; /**< Frame type. */
> >>>>> } __rte_aligned(2);
> >>>>>
> >>>>> #pragma pop_macro("s_addr")
> >>>>>
> >>>>
> >>>> What is the problem with the workaround, why we can't live with it?
> >>>>
> >>>> It requires an order in include files, right?
> >>>
> >>> There's no problem except a tricky structure definition with fields that
> >>> violate DPDK coding rules. It works with any include order.
> >>>
> >>> Will fix typos in v3, thanks.
> >>>
> >>
> >> For following case, won't compiler take 's_addr' as macro?
> >>
> >> #include <rte_ether.h>
> >> #include <winsock2.h>
> >> struct rte_ether_hdr eh;
> >> /* eh.s_addr.addr_bytes[0] = 0;
> >>
> >
> > Yes, it will. The macro will expand to `S_addr.S_un` and compile successfully.
>
> will 'eh.S_addr.S_un.addr_bytes[0] = 0;' compile successfully?
Yes, only it's `S_un.S_addr`, sorry for the typo in my explanation.
Both code snippets from commit message compile successfully.
>
> > In theory, Microsoft can change the definition of `s_addr`, and while I doubt
> > they will, it's a valid concern and a reason to remove workaround in 21.11.
> >
>
On 5/20/2021 5:16 PM, Dmitry Kozlyuk wrote:
> 2021-05-20 17:04 (UTC+0100), Ferruh Yigit:
>> On 5/20/2021 4:50 PM, Dmitry Kozlyuk wrote:
>>> 2021-05-20 16:27 (UTC+0100), Ferruh Yigit:
>>>> On 5/20/2021 4:06 PM, Dmitry Kozlyuk wrote:
>>>>> 2021-05-20 15:24 (UTC+0100), Ferruh Yigit:
>>>>>> On 3/3/2021 10:51 PM, Dmitry Kozlyuk wrote:
>>>>> [...]
>>>>>>>
>>>>>>> It is not mandatory to rename `d_addr`, this is for consistency only.
>>>>>>> Naming in `rte_ether_hdr` will also resemble `rte_ipv4/6_hdr`.
>>>>>>>
>>>>>>> Workaround is to define `struct rte_ether_hdr` in such a away that
>>>>>>> it can be used with or without `s_addr` macro (as defined on Windows)
>>>>>>> This can be done for Windows only or for all platforms to save space.
>>>>>>>
>>>>>>> #pragma push_macro("s_addr")
>>>>>>> #ifdef 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;
>>>>>>> /**< MUST NOT be used directly, only via s_addr */
>>>>>>> } S_addr;
>>>>>>> /*< MUST NOT be used directly, only via s_addr */
>>>>>>> };
>>>>>>> uint16_t ether_type; /**< Frame type. */
>>>>>>> } __rte_aligned(2);
>>>>>>>
>>>>>>> #pragma pop_macro("s_addr")
>>>>>>>
>>>>>>
>>>>>> What is the problem with the workaround, why we can't live with it?
>>>>>>
>>>>>> It requires an order in include files, right?
>>>>>
>>>>> There's no problem except a tricky structure definition with fields that
>>>>> violate DPDK coding rules. It works with any include order.
>>>>>
>>>>> Will fix typos in v3, thanks.
>>>>>
>>>>
>>>> For following case, won't compiler take 's_addr' as macro?
>>>>
>>>> #include <rte_ether.h>
>>>> #include <winsock2.h>
>>>> struct rte_ether_hdr eh;
>>>> /* eh.s_addr.addr_bytes[0] = 0;
>>>>
>>>
>>> Yes, it will. The macro will expand to `S_addr.S_un` and compile successfully.
>>
>> will 'eh.S_addr.S_un.addr_bytes[0] = 0;' compile successfully?
>
> Yes, only it's `S_un.S_addr`, sorry for the typo in my explanation.
> Both code snippets from commit message compile successfully.
>
Ah, I was missing the union on the struct, yes it will build,
And +1 to deprecation notice and clean the "struct rte_ether_hdr" whenever possible.
>>
>>> In theory, Microsoft can change the definition of `s_addr`, and while I doubt
>>> they will, it's a valid concern and a reason to remove workaround in 21.11.
>>>
>>
>
@@ -125,3 +125,7 @@ Deprecation Notices
* cmdline: ``cmdline`` structure will be made opaque to hide platform-specific
content. On Linux and FreeBSD, supported prior to DPDK 20.11,
original structure will be kept until DPDK 21.11.
+
+* net: ``s_addr`` and ``d_addr`` fields of ``rte_ether_hdr`` structure
+ will be renamed to ``src_addr`` and ``dst_addr`` respectively in DPDK 20.11
+ in order to avoid conflict with Windows Sockets headers.