DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
@ 2021-03-03 22:51 Dmitry Kozlyuk
  2021-03-03 23:54 ` Stephen Hemminger
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-03-03 22:51 UTC (permalink / raw)
  To: dev
  Cc: Ferruh Yigit, Pallavi Kadam, Dmitry Malloy,
	Narcisa Ana Maria Vasile, Dmitry Kozlyuk, Ray Kinsella,
	Neil Horman

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(+)

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
+  in order to avoid conflict with Windows Sockets headers.
-- 
2.29.2


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-03-03 22:51 [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields Dmitry Kozlyuk
@ 2021-03-03 23:54 ` Stephen Hemminger
  2021-03-04  7:09   ` Dmitry Kozlyuk
  2021-03-10 23:54 ` [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility Dmitry Kozlyuk
  2021-05-20 14:24 ` [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields Ferruh Yigit
  2 siblings, 1 reply; 22+ messages in thread
From: Stephen Hemminger @ 2021-03-03 23:54 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Ferruh Yigit, Pallavi Kadam, Dmitry Malloy,
	Narcisa Ana Maria Vasile, Ray Kinsella, Neil Horman


> +
> +* 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?



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-03-03 23:54 ` Stephen Hemminger
@ 2021-03-04  7:09   ` Dmitry Kozlyuk
  2021-05-20 14:28     ` Ferruh Yigit
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-03-04  7:09 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: dev, Ferruh Yigit, Pallavi Kadam, Dmitry Malloy,
	Narcisa Ana Maria Vasile, Ray Kinsella, Neil Horman

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility
  2021-03-03 22:51 [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields Dmitry Kozlyuk
  2021-03-03 23:54 ` Stephen Hemminger
@ 2021-03-10 23:54 ` Dmitry Kozlyuk
  2021-03-11 16:19   ` John Alexander
                     ` (2 more replies)
  2021-05-20 14:24 ` [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields Ferruh Yigit
  2 siblings, 3 replies; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-03-10 23:54 UTC (permalink / raw)
  To: dev
  Cc: Ferruh Yigit, Pallavi Kadam, Dmitry Malloy,
	Narcisa Ana Maria Vasile, Stephen Hemminger, Tyler Retzlaff,
	Declan Doherty, Fiona Trahe, Ashish Gupta, Dmitry Kozlyuk,
	Ray Kinsella, Neil Horman

Windows socket headers define `s_addr`, `min`, and `max` macros which
break structure definitions containing fields with one of these names.
Undefining those macros would break some consumers as well.

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 proposed to rename the conflicting fields on DPDK side,
because Win32 API has wider use and is slower and harder to change.

New names are left unspecified, open suggestions:

* `struct rte_ether_hdr` (by Stephen Hemminger):

    * `s_addr` -> `ether_shost`
    * `d_addr` -> `ether_dhost` (for consistency)

* `struct rte_param_log2_range`, `struct rte_crypto_param_range`:

    * `min` -> `minimum`
    * `max` -> `maximum`

For `s_addr`, a workaround is possible to use it until 21.11.
For `min` and `max`, no workaround seems available. If cryptodev or
compressdev is going to be enabled on Windows before 21.11, the only
option seems to use a new name on Windows (using #ifdef).

Workaround for `s_addr` 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):

    #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>
---
v2: * Propose to rename all problematic fields identified so far.
    * Leave future names unspecified, no need to promise now.
    * Propose better names for MAC addresses (Stephen Hemminger).

Thread about `s_addr` workaround:
https://mails.dpdk.org/archives/dev/2021-March/200700.html
Tyler Retzlaff confirmed offline that Microsoft took similar approach.

 doc/guides/rel_notes/deprecation.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 64629e0641..854618f091 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -130,3 +130,12 @@ 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 in DPDK 20.11 to avoid conflict with Windows Sockets headers.
+
+* compressdev: ``min`` and ``max`` fields of ``rte_param_log2_range`` structure
+  will be renamed in DPDK 20.11 to avoid conflict with Windows Sockets headers.
+
+* cryptodev: ``min`` and ``max`` fields of ``rte_crypto_param_range`` structure
+  will be renamed in DPDK 20.11 to avoid conflict with Windows Sockets headers.
-- 
2.29.2


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility
  2021-03-10 23:54 ` [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility Dmitry Kozlyuk
@ 2021-03-11 16:19   ` John Alexander
  2021-03-11 17:01     ` Dmitry Kozlyuk
  2021-03-16 10:37   ` Thomas Monjalon
  2021-05-20 18:42   ` [dpdk-dev] [PATCH v3] " Dmitry Kozlyuk
  2 siblings, 1 reply; 22+ messages in thread
From: John Alexander @ 2021-03-11 16:19 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev
  Cc: Ferruh Yigit, Pallavi Kadam, Dmitry Malloy,
	Narcisa Ana Maria Vasile, Stephen Hemminger, Tyler Retzlaff,
	Declan Doherty, Fiona Trahe, Ashish Gupta, Ray Kinsella,
	Neil Horman


> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Dmitry Kozlyuk
> Sent: 10 March 2021 23:54
> To: dev@dpdk.org
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Pallavi Kadam
> <pallavi.kadam@intel.com>; Dmitry Malloy <dmitrym@microsoft.com>;
> Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>; Stephen
> Hemminger <stephen@networkplumber.org>; Tyler Retzlaff
> <roretzla@linux.microsoft.com>; Declan Doherty
> <declan.doherty@intel.com>; Fiona Trahe <fiona.trahe@intel.com>; Ashish
> Gupta <ashish.gupta@marvell.com>; Dmitry Kozlyuk
> <dmitry.kozliuk@gmail.com>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> <nhorman@tuxdriver.com>
> Subject: [dpdk-dev] [PATCH v2] doc: announce API changes for Windows
> compatibility
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you recognize the sender and know the
> content is safe.
> 
> Windows socket headers define `s_addr`, `min`, and `max` macros which
> break structure definitions containing fields with one of these names.
> Undefining those macros would break some consumers as well.
> 
> 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 proposed to rename the conflicting fields on DPDK side, because Win32
> API has wider use and is slower and harder to change.
> 
> New names are left unspecified, open suggestions:
> 
> * `struct rte_ether_hdr` (by Stephen Hemminger):
> 
>     * `s_addr` -> `ether_shost`
>     * `d_addr` -> `ether_dhost` (for consistency)
> 
> * `struct rte_param_log2_range`, `struct rte_crypto_param_range`:
> 
>     * `min` -> `minimum`
>     * `max` -> `maximum`


The min/max macros in the Windows headers cause issues with C++ projects also (breaks std::min/std::max).  The fix there is to "#define NOMINMAX" prior to including windows.h, maybe that's appropriate here too?

> 
> For `s_addr`, a workaround is possible to use it until 21.11.
> For `min` and `max`, no workaround seems available. If cryptodev or
> compressdev is going to be enabled on Windows before 21.11, the only
> option seems to use a new name on Windows (using #ifdef).
> 
> Workaround for `s_addr` 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):
> 
>     #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>
> ---
> v2: * Propose to rename all problematic fields identified so far.
>     * Leave future names unspecified, no need to promise now.
>     * Propose better names for MAC addresses (Stephen Hemminger).
> 
> Thread about `s_addr` workaround:
> https://mails.dpdk.org/archives/dev/2021-March/200700.html
> Tyler Retzlaff confirmed offline that Microsoft took similar approach.
> 
>  doc/guides/rel_notes/deprecation.rst | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 64629e0641..854618f091 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -130,3 +130,12 @@ 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 in DPDK 20.11 to avoid conflict with Windows Sockets
> headers.
> +
> +* compressdev: ``min`` and ``max`` fields of ``rte_param_log2_range``
> +structure
> +  will be renamed in DPDK 20.11 to avoid conflict with Windows Sockets
> headers.
> +
> +* cryptodev: ``min`` and ``max`` fields of ``rte_crypto_param_range``
> +structure
> +  will be renamed in DPDK 20.11 to avoid conflict with Windows Sockets
> headers.
> --
> 2.29.2


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility
  2021-03-11 16:19   ` John Alexander
@ 2021-03-11 17:01     ` Dmitry Kozlyuk
  2021-03-11 17:08       ` Tyler Retzlaff
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-03-11 17:01 UTC (permalink / raw)
  To: John Alexander
  Cc: dev, Ferruh Yigit, Pallavi Kadam, Dmitry Malloy,
	Narcisa Ana Maria Vasile, Stephen Hemminger, Tyler Retzlaff,
	Declan Doherty, Fiona Trahe, Ashish Gupta, Ray Kinsella,
	Neil Horman

2021-03-11 16:19 (UTC+0000), John Alexander:
[...]
> > * `struct rte_param_log2_range`, `struct rte_crypto_param_range`:
> > 
> >     * `min` -> `minimum`
> >     * `max` -> `maximum`  
> 
> 
> The min/max macros in the Windows headers cause issues with C++ projects also (breaks std::min/std::max).  The fix there is to "#define NOMINMAX" prior to including windows.h, maybe that's appropriate here too?

We don't control include order in user code and we shouldn't #undef system
macros in public headers. We could push_macro/pop_macro around structure
definition and have min/max undefined in DPDK internal code, so that including
this header always works. Then, if user wants to access the fields, they
should take care of macros themselves.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility
  2021-03-11 17:01     ` Dmitry Kozlyuk
@ 2021-03-11 17:08       ` Tyler Retzlaff
  0 siblings, 0 replies; 22+ messages in thread
From: Tyler Retzlaff @ 2021-03-11 17:08 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: John Alexander, dev, Ferruh Yigit, Pallavi Kadam, Dmitry Malloy,
	Narcisa Ana Maria Vasile, Stephen Hemminger, Declan Doherty,
	Fiona Trahe, Ashish Gupta, Ray Kinsella, Neil Horman

On Thu, Mar 11, 2021 at 08:01:01PM +0300, Dmitry Kozlyuk wrote:
> 2021-03-11 16:19 (UTC+0000), John Alexander:
> [...]
> > > * `struct rte_param_log2_range`, `struct rte_crypto_param_range`:
> > > 
> > >     * `min` -> `minimum`
> > >     * `max` -> `maximum`  
> > 
> > 
> > The min/max macros in the Windows headers cause issues with C++ projects also (breaks std::min/std::max).  The fix there is to "#define NOMINMAX" prior to including windows.h, maybe that's appropriate here too?
> 
> We don't control include order in user code and we shouldn't #undef system
> macros in public headers. We could push_macro/pop_macro around structure
> definition and have min/max undefined in DPDK internal code, so that including
> this header always works. Then, if user wants to access the fields, they
> should take care of macros themselves.

agreed, nothing stops an application from including windows.h before
dpdk includes windows.h and similarly the application shouldn't have
visibility of min/max (broken as they are) disappear from the
application namespace after including dpdk headers.

the approach of altering what appears in the namespace for the scope of
the dpdk header preprocessing is about the best that can be achieved.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility
  2021-03-10 23:54 ` [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility Dmitry Kozlyuk
  2021-03-11 16:19   ` John Alexander
@ 2021-03-16 10:37   ` Thomas Monjalon
  2021-05-20 18:42   ` [dpdk-dev] [PATCH v3] " Dmitry Kozlyuk
  2 siblings, 0 replies; 22+ messages in thread
From: Thomas Monjalon @ 2021-03-16 10:37 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Ferruh Yigit, Pallavi Kadam, Dmitry Malloy,
	Narcisa Ana Maria Vasile, Stephen Hemminger, Tyler Retzlaff,
	Declan Doherty, Fiona Trahe, Ashish Gupta, Ray Kinsella,
	david.marchand, bruce.richardson

11/03/2021 00:54, Dmitry Kozlyuk:
> Windows socket headers define `s_addr`, `min`, and `max` macros which
> break structure definitions containing fields with one of these names.
> Undefining those macros would break some consumers as well.
> 
> 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 proposed to rename the conflicting fields on DPDK side,
> because Win32 API has wider use and is slower and harder to change.
> 
> New names are left unspecified, open suggestions:
> 
> * `struct rte_ether_hdr` (by Stephen Hemminger):
> 
>     * `s_addr` -> `ether_shost`
>     * `d_addr` -> `ether_dhost` (for consistency)

I prefer "addr" over "host".


> * `struct rte_param_log2_range`, `struct rte_crypto_param_range`:
> 
>     * `min` -> `minimum`
>     * `max` -> `maximum`
> 
> For `s_addr`, a workaround is possible to use it until 21.11.
> For `min` and `max`, no workaround seems available. If cryptodev or
> compressdev is going to be enabled on Windows before 21.11, the only
> option seems to use a new name on Windows (using #ifdef).

I think we have enough work on ethdev until 21.11.




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-03-03 22:51 [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields Dmitry Kozlyuk
  2021-03-03 23:54 ` Stephen Hemminger
  2021-03-10 23:54 ` [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility Dmitry Kozlyuk
@ 2021-05-20 14:24 ` Ferruh Yigit
  2021-05-20 15:06   ` Dmitry Kozlyuk
  2 siblings, 1 reply; 22+ messages in thread
From: Ferruh Yigit @ 2021-05-20 14:24 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev
  Cc: Pallavi Kadam, Dmitry Malloy, Narcisa Ana Maria Vasile,
	Ray Kinsella, Neil Horman

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.
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-03-04  7:09   ` Dmitry Kozlyuk
@ 2021-05-20 14:28     ` Ferruh Yigit
  0 siblings, 0 replies; 22+ messages in thread
From: Ferruh Yigit @ 2021-05-20 14:28 UTC (permalink / raw)
  To: Dmitry Kozlyuk, Stephen Hemminger
  Cc: dev, Pallavi Kadam, Dmitry Malloy, Narcisa Ana Maria Vasile,
	Ray Kinsella, Neil Horman

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.
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-05-20 14:24 ` [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields Ferruh Yigit
@ 2021-05-20 15:06   ` Dmitry Kozlyuk
  2021-05-20 15:27     ` Ferruh Yigit
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-20 15:06 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: dev, Pallavi Kadam, Dmitry Malloy, Narcisa Ana Maria Vasile,
	Ray Kinsella, Neil Horman

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-05-20 15:06   ` Dmitry Kozlyuk
@ 2021-05-20 15:27     ` Ferruh Yigit
  2021-05-20 15:50       ` Dmitry Kozlyuk
  0 siblings, 1 reply; 22+ messages in thread
From: Ferruh Yigit @ 2021-05-20 15:27 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Pallavi Kadam, Dmitry Malloy, Narcisa Ana Maria Vasile,
	Ray Kinsella, Neil Horman

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;


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-05-20 15:27     ` Ferruh Yigit
@ 2021-05-20 15:50       ` Dmitry Kozlyuk
  2021-05-20 16:04         ` Ferruh Yigit
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-20 15:50 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: dev, Pallavi Kadam, Dmitry Malloy, Narcisa Ana Maria Vasile,
	Ray Kinsella, Neil Horman

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-05-20 15:50       ` Dmitry Kozlyuk
@ 2021-05-20 16:04         ` Ferruh Yigit
  2021-05-20 16:16           ` Dmitry Kozlyuk
  0 siblings, 1 reply; 22+ messages in thread
From: Ferruh Yigit @ 2021-05-20 16:04 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Pallavi Kadam, Dmitry Malloy, Narcisa Ana Maria Vasile,
	Ray Kinsella, Neil Horman

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.
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-05-20 16:04         ` Ferruh Yigit
@ 2021-05-20 16:16           ` Dmitry Kozlyuk
  2021-05-20 16:25             ` Ferruh Yigit
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-20 16:16 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: dev, Pallavi Kadam, Dmitry Malloy, Narcisa Ana Maria Vasile,
	Ray Kinsella, Neil Horman

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.
> >   
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields
  2021-05-20 16:16           ` Dmitry Kozlyuk
@ 2021-05-20 16:25             ` Ferruh Yigit
  0 siblings, 0 replies; 22+ messages in thread
From: Ferruh Yigit @ 2021-05-20 16:25 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Pallavi Kadam, Dmitry Malloy, Narcisa Ana Maria Vasile,
	Ray Kinsella, Neil Horman

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.
>>>   
>>
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v3] doc: announce API changes for Windows compatibility
  2021-03-10 23:54 ` [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility Dmitry Kozlyuk
  2021-03-11 16:19   ` John Alexander
  2021-03-16 10:37   ` Thomas Monjalon
@ 2021-05-20 18:42   ` Dmitry Kozlyuk
  2021-05-20 18:59     ` [dpdk-dev] [EXT] " Akhil Goyal
  2 siblings, 1 reply; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-20 18:42 UTC (permalink / raw)
  To: dev
  Cc: Ferruh Yigit, Akhil Goyal, Fiona Trahe, Ashish Gupta,
	Dmitry Kozlyuk, Khoa To, Ray Kinsella, Neil Horman

Windows system headers define `s_addr`, `min`, and `max` macros which
break structure definitions containing fields with one of these names.
Undefining those macros would break consumer code that relies on them.

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 */

Commit 6c068dbd9fea ("net: work around s_addr macro on Windows")
modified definition of `struct rte_ether_hdr` to avoid the issue.
However, the workaround assumes `#define s_addr S_addr.S_un`
in Windows headers, which is not a part of official API.
It also complicates the definition of `struct rte_ether_hdr`.

For `min` and `max`, no workaround seems available. If cryptodev or
compressdev is going to be enabled on Windows before 21.11, the only
option seems to use a new name on Windows (using #ifdef).

It is proposed to rename the conflicting fields on DPDK side,
because Win32 API has wider use and is slower and harder to change.
Exact new names are left for further discussion.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Acked-by: Khoa To <khot@microsoft.com>
---
v3: fix typos (Ferruh), remove naming speculation,
    replace workaround snippet with commit reference.

 doc/guides/rel_notes/deprecation.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 9584d6bfd7..cc6e8db92c 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -147,3 +147,12 @@ 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 in DPDK 21.11 to avoid conflict with Windows Sockets headers.
+
+* compressdev: ``min`` and ``max`` fields of ``rte_param_log2_range`` structure
+  will be renamed in DPDK 21.11 to avoid conflict with Windows Sockets headers.
+
+* cryptodev: ``min`` and ``max`` fields of ``rte_crypto_param_range`` structure
+  will be renamed in DPDK 21.11 to avoid conflict with Windows Sockets headers.
-- 
2.29.3


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v3] doc: announce API changes for Windows compatibility
  2021-05-20 18:42   ` [dpdk-dev] [PATCH v3] " Dmitry Kozlyuk
@ 2021-05-20 18:59     ` Akhil Goyal
  2021-05-20 19:31       ` Dmitry Kozlyuk
  0 siblings, 1 reply; 22+ messages in thread
From: Akhil Goyal @ 2021-05-20 18:59 UTC (permalink / raw)
  To: Dmitry Kozlyuk, dev
  Cc: Ferruh Yigit, Fiona Trahe, Ashish Gupta, Khoa To, Ray Kinsella,
	Neil Horman

> Windows system headers define `s_addr`, `min`, and `max` macros which
> break structure definitions containing fields with one of these names.
> Undefining those macros would break consumer code that relies on them.
> 

From the commit message the requirement for changing the structure definitions
Is not clear. Please note that 'min' - 'max' are not macros. These are variables of a
structure which should not break any other structure/Macro in windows.

> 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 */
> 
> Commit 6c068dbd9fea ("net: work around s_addr macro on Windows")
> modified definition of `struct rte_ether_hdr` to avoid the issue.
> However, the workaround assumes `#define s_addr S_addr.S_un`
> in Windows headers, which is not a part of official API.
> It also complicates the definition of `struct rte_ether_hdr`.
> 
> For `min` and `max`, no workaround seems available. If cryptodev or
> compressdev is going to be enabled on Windows before 21.11, the only
> option seems to use a new name on Windows (using #ifdef).
> 
> It is proposed to rename the conflicting fields on DPDK side,
> because Win32 API has wider use and is slower and harder to change.
> Exact new names are left for further discussion.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Acked-by: Khoa To <khot@microsoft.com>
> ---
> v3: fix typos (Ferruh), remove naming speculation,
>     replace workaround snippet with commit reference.
> 
>  doc/guides/rel_notes/deprecation.rst | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 9584d6bfd7..cc6e8db92c 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -147,3 +147,12 @@ 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 in DPDK 21.11 to avoid conflict with Windows Sockets
> headers.
> +
> +* compressdev: ``min`` and ``max`` fields of ``rte_param_log2_range``
> structure
> +  will be renamed in DPDK 21.11 to avoid conflict with Windows Sockets
> headers.
> +
> +* cryptodev: ``min`` and ``max`` fields of ``rte_crypto_param_range``
> structure
> +  will be renamed in DPDK 21.11 to avoid conflict with Windows Sockets
> headers.
> --
> 2.29.3


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v3] doc: announce API changes for Windows compatibility
  2021-05-20 18:59     ` [dpdk-dev] [EXT] " Akhil Goyal
@ 2021-05-20 19:31       ` Dmitry Kozlyuk
  2021-05-20 20:17         ` Akhil Goyal
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-20 19:31 UTC (permalink / raw)
  To: Akhil Goyal
  Cc: dev, Ferruh Yigit, Fiona Trahe, Ashish Gupta, Khoa To,
	Ray Kinsella, Neil Horman

2021-05-20 18:59 (UTC+0000), Akhil Goyal:
> > Windows system headers define `s_addr`, `min`, and `max` macros which
> > break structure definitions containing fields with one of these names.
> > Undefining those macros would break consumer code that relies on them.
> >   
> 
> From the commit message the requirement for changing the structure definitions
> Is not clear. Please note that 'min' - 'max' are not macros. These are variables of a
> structure which should not break any other structure/Macro in windows.

Err, yes, that's what the commit message says.
Structure fields of course break nothing; they are broken by Windows macros.
Would this make more sense?


	Windows headers define `s_addr`, `min`, and `max` as macros.
	If DPDK headers are included after Windows ones, DPDK structure
	definitions containing fields with these names get broken.
	If DPDK headers undefined these macros, it could break consumer code
	relying on these macros. It is proposed to rename structure fields
	in DPDK, because Win32 headers are more widely used and harder to fix.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v3] doc: announce API changes for Windows compatibility
  2021-05-20 19:31       ` Dmitry Kozlyuk
@ 2021-05-20 20:17         ` Akhil Goyal
  2021-06-09 15:52           ` Dmitry Kozlyuk
  2021-06-17 14:27           ` Tyler Retzlaff
  0 siblings, 2 replies; 22+ messages in thread
From: Akhil Goyal @ 2021-05-20 20:17 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: dev, Ferruh Yigit, Fiona Trahe, Ashish Gupta, Khoa To,
	Ray Kinsella, Neil Horman, Thomas Monjalon, bruce.richardson,
	Konstantin Ananyev, Jerin Jacob Kollanukkaran, Zhang, Roy Fan

> 
> 2021-05-20 18:59 (UTC+0000), Akhil Goyal:
> > > Windows system headers define `s_addr`, `min`, and `max` macros which
> > > break structure definitions containing fields with one of these names.
> > > Undefining those macros would break consumer code that relies on
> them.
> > >
> >
> > From the commit message the requirement for changing the structure
> definitions
> > Is not clear. Please note that 'min' - 'max' are not macros. These are
> variables of a
> > structure which should not break any other structure/Macro in windows.
> 
> Err, yes, that's what the commit message says.
> Structure fields of course break nothing; they are broken by Windows
> macros.
> Would this make more sense?
> 
> 
> 	Windows headers define `s_addr`, `min`, and `max` as macros.
> 	If DPDK headers are included after Windows ones, DPDK structure
> 	definitions containing fields with these names get broken.
> 	If DPDK headers undefined these macros, it could break consumer
> code
> 	relying on these macros. It is proposed to rename structure fields
> 	in DPDK, because Win32 headers are more widely used and harder
> to fix.

Yes it makes more sense now. But ideally it should be fixed in windows.
This may be just one such issue, there may be many more.
Will this also mean that nobody can define a local variable 'min'?
Is this acceptable?

Any macro definition in a subsystem should have a prefix to denote that,
Just like in DPDK 'RTE_' is added.
Macros with generic names should be avoided so that we do not get into
these issues.

Adding more people for comments. I don't have a good feeling about
this change.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v3] doc: announce API changes for Windows compatibility
  2021-05-20 20:17         ` Akhil Goyal
@ 2021-06-09 15:52           ` Dmitry Kozlyuk
  2021-06-17 14:27           ` Tyler Retzlaff
  1 sibling, 0 replies; 22+ messages in thread
From: Dmitry Kozlyuk @ 2021-06-09 15:52 UTC (permalink / raw)
  To: Akhil Goyal
  Cc: dev, Ferruh Yigit, Fiona Trahe, Ashish Gupta, Khoa To,
	Ray Kinsella, Neil Horman, Thomas Monjalon, bruce.richardson,
	Konstantin Ananyev, Jerin Jacob Kollanukkaran, Zhang, Roy Fan

2021-05-20 20:17 (UTC+0000), Akhil Goyal:
> > 
> > 2021-05-20 18:59 (UTC+0000), Akhil Goyal:  
> > > > Windows system headers define `s_addr`, `min`, and `max` macros which
> > > > break structure definitions containing fields with one of these names.
> > > > Undefining those macros would break consumer code that relies on  
> > them.  
> > > >  
> > >
> > > From the commit message the requirement for changing the structure  
> > definitions  
> > > Is not clear. Please note that 'min' - 'max' are not macros. These are  
> > variables of a  
> > > structure which should not break any other structure/Macro in windows.  
> > 
> > Err, yes, that's what the commit message says.
> > Structure fields of course break nothing; they are broken by Windows
> > macros.
> > Would this make more sense?
> > 
> > 
> > 	Windows headers define `s_addr`, `min`, and `max` as macros.
> > 	If DPDK headers are included after Windows ones, DPDK structure
> > 	definitions containing fields with these names get broken.
> > 	If DPDK headers undefined these macros, it could break consumer
> > code
> > 	relying on these macros. It is proposed to rename structure fields
> > 	in DPDK, because Win32 headers are more widely used and harder
> > to fix.  
> 
> Yes it makes more sense now. But ideally it should be fixed in windows.
> This may be just one such issue, there may be many more.
> Will this also mean that nobody can define a local variable 'min'?
> Is this acceptable?

Only in public headers. There happens to be one such, rte_lru_x86.h.

> Any macro definition in a subsystem should have a prefix to denote that,
> Just like in DPDK 'RTE_' is added.
> Macros with generic names should be avoided so that we do not get into
> these issues.
> 
> Adding more people for comments. I don't have a good feeling about
> this change.

Friendly ping to everyone Akhil cc'ed.
As far as I understand, if we want to fix it in 21.11,
deprecation notice should make it into 21.08.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v3] doc: announce API changes for Windows compatibility
  2021-05-20 20:17         ` Akhil Goyal
  2021-06-09 15:52           ` Dmitry Kozlyuk
@ 2021-06-17 14:27           ` Tyler Retzlaff
  1 sibling, 0 replies; 22+ messages in thread
From: Tyler Retzlaff @ 2021-06-17 14:27 UTC (permalink / raw)
  To: Akhil Goyal
  Cc: Dmitry Kozlyuk, dev, Ferruh Yigit, Fiona Trahe, Ashish Gupta,
	Khoa To, Ray Kinsella, Neil Horman, Thomas Monjalon,
	bruce.richardson, Konstantin Ananyev, Jerin Jacob Kollanukkaran,
	Zhang, Roy Fan

On Thu, May 20, 2021 at 08:17:54PM +0000, Akhil Goyal wrote:
> > 
> 
> Yes it makes more sense now. But ideally it should be fixed in windows.

they won't be fixed in windows.  it is extremely rare to withdraw
something from a platform headers namespace and is avoided to maintain
API compatibility no matter how horrible these macros are.

> This may be just one such issue, there may be many more.
> Will this also mean that nobody can define a local variable 'min'?

that is correct. As well as a long list of other commonly used names not
only from windows but other platform headers, standard library headers
and keywords.

> Any macro definition in a subsystem should have a prefix to denote that,
> Just like in DPDK 'RTE_' is added.

completely agree, best practice would be to avoid contaminating the
applications namespace.

> Macros with generic names should be avoided so that we do not get into
> these issues.
> 
> Adding more people for comments. I don't have a good feeling about
> this change.

there is no real choice, the platform header won't be "fixed" so it has
to be dealt with and even thinking you can avoid it by just not
including windows.h here keep in mind the consuming application will
have to anyway so it's difficult to avoid.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-06-17 14:27 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 22:51 [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields Dmitry Kozlyuk
2021-03-03 23:54 ` Stephen Hemminger
2021-03-04  7:09   ` Dmitry Kozlyuk
2021-05-20 14:28     ` Ferruh Yigit
2021-03-10 23:54 ` [dpdk-dev] [PATCH v2] doc: announce API changes for Windows compatibility Dmitry Kozlyuk
2021-03-11 16:19   ` John Alexander
2021-03-11 17:01     ` Dmitry Kozlyuk
2021-03-11 17:08       ` Tyler Retzlaff
2021-03-16 10:37   ` Thomas Monjalon
2021-05-20 18:42   ` [dpdk-dev] [PATCH v3] " Dmitry Kozlyuk
2021-05-20 18:59     ` [dpdk-dev] [EXT] " Akhil Goyal
2021-05-20 19:31       ` Dmitry Kozlyuk
2021-05-20 20:17         ` Akhil Goyal
2021-06-09 15:52           ` Dmitry Kozlyuk
2021-06-17 14:27           ` Tyler Retzlaff
2021-05-20 14:24 ` [dpdk-dev] [PATCH] doc: announce renaming of rte_ether_hdr fields Ferruh Yigit
2021-05-20 15:06   ` Dmitry Kozlyuk
2021-05-20 15:27     ` Ferruh Yigit
2021-05-20 15:50       ` Dmitry Kozlyuk
2021-05-20 16:04         ` Ferruh Yigit
2021-05-20 16:16           ` Dmitry Kozlyuk
2021-05-20 16:25             ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git