* 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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
2021-07-21 19:55 ` [dpdk-dev] " Dmitry Kozlyuk
2 siblings, 2 replies; 31+ 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] 31+ 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
2021-07-21 19:55 ` [dpdk-dev] " Dmitry Kozlyuk
1 sibling, 1 reply; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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-23 15:14 ` Dmitry Kozlyuk
2021-06-17 14:27 ` Tyler Retzlaff
1 sibling, 1 reply; 31+ 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] 31+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v3] doc: announce API changes for Windows compatibility
2021-06-09 15:52 ` Dmitry Kozlyuk
@ 2021-06-23 15:14 ` Dmitry Kozlyuk
0 siblings, 0 replies; 31+ messages in thread
From: Dmitry Kozlyuk @ 2021-06-23 15:14 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-06-09 18:52 (UTC+0300), Dmitry Kozlyuk:
> 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.
Friendly ping v2.
Hopefully Tyler's answer will help with the decision.
^ permalink raw reply [flat|nested] 31+ 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; 31+ 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] 31+ messages in thread
* [dpdk-dev] [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 ` [dpdk-dev] [EXT] " Akhil Goyal
@ 2021-07-21 19:55 ` Dmitry Kozlyuk
2021-07-21 19:55 ` [dpdk-dev] [PATCH v4] " Dmitry Kozlyuk
1 sibling, 1 reply; 31+ messages in thread
From: Dmitry Kozlyuk @ 2021-07-21 19:55 UTC (permalink / raw)
To: dev
Cc: Ferruh Yigit, Akhil Goyal, Fiona Trahe, Neil Horman,
Dmitry Kozlyuk, Khoa To, Ray Kinsella
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] 31+ messages in thread
* [dpdk-dev] [PATCH v4] doc: announce API changes for Windows compatibility
2021-07-21 19:55 ` [dpdk-dev] " Dmitry Kozlyuk
@ 2021-07-21 19:55 ` Dmitry Kozlyuk
2021-08-02 12:13 ` Thomas Monjalon
0 siblings, 1 reply; 31+ messages in thread
From: Dmitry Kozlyuk @ 2021-07-21 19:55 UTC (permalink / raw)
To: dev
Cc: Ferruh Yigit, Akhil Goyal, Fiona Trahe, Neil Horman,
Dmitry Kozlyuk, Khoa To, Ray Kinsella
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 (example 1),
as well as any usage of such fields (example 2). If DPDK headers
undefined these macros, it could break consumer code (example 3).
It is proposed to rename structure fields in DPDK, because Win32 headers
are used more widely than DPDK, as a general-purpose platform compared
to domain-specific kit, and are harder to fix because of that.
Exact new names are left for further discussion.
Example 1:
/* in DPDK public header included after windows.h */
struct rte_type {
int min; /* ERROR: `min` is a macro */
};
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 */
Example 3:
#include <winsock2.h>
#include <rte_ether.h>
struct in_addr addr;
addr.s_addr = 0; /* ERROR: there is no `s_addr` field,
and `s_addr` macro is undefined by DPDK. */
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`.
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Acked-by: Khoa To <khot@microsoft.com>
---
v4: improve wording (Akhil).
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] 31+ messages in thread
* Re: [dpdk-dev] [PATCH v4] doc: announce API changes for Windows compatibility
2021-07-21 19:55 ` [dpdk-dev] [PATCH v4] " Dmitry Kozlyuk
@ 2021-08-02 12:13 ` Thomas Monjalon
2021-08-02 12:45 ` [dpdk-dev] [EXT] " Akhil Goyal
0 siblings, 1 reply; 31+ messages in thread
From: Thomas Monjalon @ 2021-08-02 12:13 UTC (permalink / raw)
To: Dmitry Kozlyuk
Cc: dev, Ferruh Yigit, Akhil Goyal, Fiona Trahe, Khoa To,
Ray Kinsella, andrew.rybchenko, olivier.matz, navasile,
pallavi.kadam, ranjit.menon, ferruh.yigit, bruce.richardson,
stephen
21/07/2021 21:55, Dmitry Kozlyuk:
> 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 (example 1),
> as well as any usage of such fields (example 2). If DPDK headers
> undefined these macros, it could break consumer code (example 3).
> It is proposed to rename structure fields in DPDK, because Win32 headers
> are used more widely than DPDK, as a general-purpose platform compared
> to domain-specific kit, and are harder to fix because of that.
> Exact new names are left for further discussion.
>
> Example 1:
>
> /* in DPDK public header included after windows.h */
> struct rte_type {
> int min; /* ERROR: `min` is a macro */
> };
>
> 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 */
>
> Example 3:
>
> #include <winsock2.h>
> #include <rte_ether.h>
> struct in_addr addr;
> addr.s_addr = 0; /* ERROR: there is no `s_addr` field,
> and `s_addr` macro is undefined by DPDK. */
>
> 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`.
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Acked-by: Khoa To <khot@microsoft.com>
> ---
> +* 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.
The struct rte_param_log2_range should also be renamed to include "compress" prefix.
But as we break the struct API, it is not an issue I guess.
> +* 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.
Acked-by: Thomas Monjalon <thomas@monjalon.net>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH v4] doc: announce API changes for Windows compatibility
2021-08-02 12:13 ` Thomas Monjalon
@ 2021-08-02 12:45 ` Akhil Goyal
2021-08-02 13:00 ` Dmitry Kozlyuk
0 siblings, 1 reply; 31+ messages in thread
From: Akhil Goyal @ 2021-08-02 12:45 UTC (permalink / raw)
To: Thomas Monjalon, Dmitry Kozlyuk
Cc: dev, Ferruh Yigit, Fiona Trahe, Khoa To, Ray Kinsella,
andrew.rybchenko, olivier.matz, navasile, pallavi.kadam,
ranjit.menon, ferruh.yigit, bruce.richardson, stephen
> 21/07/2021 21:55, Dmitry Kozlyuk:
> > 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 (example 1),
> > as well as any usage of such fields (example 2). If DPDK headers
> > undefined these macros, it could break consumer code (example 3).
> > It is proposed to rename structure fields in DPDK, because Win32 headers
> > are used more widely than DPDK, as a general-purpose platform compared
> > to domain-specific kit, and are harder to fix because of that.
> > Exact new names are left for further discussion.
> >
> > Example 1:
> >
> > /* in DPDK public header included after windows.h */
> > struct rte_type {
> > int min; /* ERROR: `min` is a macro */
> > };
> >
> > 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 */
> >
> > Example 3:
> >
> > #include <winsock2.h>
> > #include <rte_ether.h>
> > struct in_addr addr;
> > addr.s_addr = 0; /* ERROR: there is no `s_addr` field,
> > and `s_addr` macro is undefined by DPDK. */
> >
> > 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`.
> >
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > Acked-by: Khoa To <khot@microsoft.com>
> > ---
> > +* 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.
>
> The struct rte_param_log2_range should also be renamed to include
> "compress" prefix.
> But as we break the struct API, it is not an issue I guess.
>
> > +* 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.
>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>
Can we have a local variable named as min/max?
If not, then I believe it is not a good idea.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH v4] doc: announce API changes for Windows compatibility
2021-08-02 12:45 ` [dpdk-dev] [EXT] " Akhil Goyal
@ 2021-08-02 13:00 ` Dmitry Kozlyuk
2021-08-02 13:48 ` Akhil Goyal
0 siblings, 1 reply; 31+ messages in thread
From: Dmitry Kozlyuk @ 2021-08-02 13:00 UTC (permalink / raw)
To: Akhil Goyal
Cc: Thomas Monjalon, dev, Ferruh Yigit, Fiona Trahe, Khoa To,
Ray Kinsella, andrew.rybchenko, olivier.matz, navasile,
pallavi.kadam, ranjit.menon, bruce.richardson, stephen
2021-08-02 12:45 (UTC+0000), Akhil Goyal:
> > 21/07/2021 21:55, Dmitry Kozlyuk:
> > > 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 (example 1),
> > > as well as any usage of such fields (example 2). If DPDK headers
> > > undefined these macros, it could break consumer code (example 3).
> > > It is proposed to rename structure fields in DPDK, because Win32 headers
> > > are used more widely than DPDK, as a general-purpose platform compared
> > > to domain-specific kit, and are harder to fix because of that.
> > > Exact new names are left for further discussion.
> > >
> > > Example 1:
> > >
> > > /* in DPDK public header included after windows.h */
> > > struct rte_type {
> > > int min; /* ERROR: `min` is a macro */
> > > };
> > >
> > > 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 */
> > >
> > > Example 3:
> > >
> > > #include <winsock2.h>
> > > #include <rte_ether.h>
> > > struct in_addr addr;
> > > addr.s_addr = 0; /* ERROR: there is no `s_addr` field,
> > > and `s_addr` macro is undefined by DPDK. */
> > >
> > > 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`.
> > >
> > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > Acked-by: Khoa To <khot@microsoft.com>
> > > ---
> > > +* 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.
> >
> > The struct rte_param_log2_range should also be renamed to include
> > "compress" prefix.
> > But as we break the struct API, it is not an issue I guess.
> >
> > > +* 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.
> >
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> >
> Can we have a local variable named as min/max?
> If not, then I believe it is not a good idea.
Yes, except for inline functions in public headers.
The only problematic one I know of is this (rte_lru_x86.h):
static inline int
f_lru_pos(uint64_t lru_list)
{
__m128i lst = _mm_set_epi64x((uint64_t)-1, lru_list);
__m128i min = _mm_minpos_epu16(lst); /* <<< */
return _mm_extract_epi16(min, 1);
}
Fixing it breaks neither API nor ABI, thus no explicit deprecation notice.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH v4] doc: announce API changes for Windows compatibility
2021-08-02 13:00 ` Dmitry Kozlyuk
@ 2021-08-02 13:48 ` Akhil Goyal
2021-08-02 14:57 ` Tal Shnaiderman
2021-08-02 17:46 ` Thomas Monjalon
0 siblings, 2 replies; 31+ messages in thread
From: Akhil Goyal @ 2021-08-02 13:48 UTC (permalink / raw)
To: Dmitry Kozlyuk
Cc: Thomas Monjalon, dev, Ferruh Yigit, Fiona Trahe, Khoa To,
Ray Kinsella, andrew.rybchenko, olivier.matz, navasile,
pallavi.kadam, ranjit.menon, bruce.richardson, stephen
> 2021-08-02 12:45 (UTC+0000), Akhil Goyal:
> > > 21/07/2021 21:55, Dmitry Kozlyuk:
> > > > 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 (example 1),
> > > > as well as any usage of such fields (example 2). If DPDK headers
> > > > undefined these macros, it could break consumer code (example 3).
> > > > It is proposed to rename structure fields in DPDK, because Win32
> headers
> > > > are used more widely than DPDK, as a general-purpose platform
> compared
> > > > to domain-specific kit, and are harder to fix because of that.
> > > > Exact new names are left for further discussion.
> > > >
> > > > Example 1:
> > > >
> > > > /* in DPDK public header included after windows.h */
> > > > struct rte_type {
> > > > int min; /* ERROR: `min` is a macro */
> > > > };
> > > >
> > > > 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 */
> > > >
> > > > Example 3:
> > > >
> > > > #include <winsock2.h>
> > > > #include <rte_ether.h>
> > > > struct in_addr addr;
> > > > addr.s_addr = 0; /* ERROR: there is no `s_addr` field,
> > > > and `s_addr` macro is undefined by DPDK. */
> > > >
> > > > 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`.
> > > >
> > > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > > Acked-by: Khoa To <khot@microsoft.com>
> > > > ---
> > > > +* 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.
> > >
> > > The struct rte_param_log2_range should also be renamed to include
> > > "compress" prefix.
> > > But as we break the struct API, it is not an issue I guess.
> > >
> > > > +* 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.
> > >
> > > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> > >
> > Can we have a local variable named as min/max?
> > If not, then I believe it is not a good idea.
>
> Yes, except for inline functions in public headers.
> The only problematic one I know of is this (rte_lru_x86.h):
>
> static inline int
> f_lru_pos(uint64_t lru_list)
> {
> __m128i lst = _mm_set_epi64x((uint64_t)-1, lru_list);
> __m128i min = _mm_minpos_epu16(lst); /* <<< */
> return _mm_extract_epi16(min, 1);
> }
>
> Fixing it breaks neither API nor ABI, thus no explicit deprecation notice.
OK,
Acked-by: Akhil Goyal <gakhil@marvell.com>
I hope when windows compilation is enabled, it will be part of CI and it will run
on each patch which goes to patchworks.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH v4] doc: announce API changes for Windows compatibility
2021-08-02 13:48 ` Akhil Goyal
@ 2021-08-02 14:57 ` Tal Shnaiderman
2021-08-02 17:46 ` Thomas Monjalon
1 sibling, 0 replies; 31+ messages in thread
From: Tal Shnaiderman @ 2021-08-02 14:57 UTC (permalink / raw)
To: Akhil Goyal, Dmitry Kozlyuk
Cc: NBU-Contact-Thomas Monjalon, dev, Ferruh Yigit, Fiona Trahe,
Khoa To, Ray Kinsella, andrew.rybchenko, olivier.matz, navasile,
pallavi.kadam, ranjit.menon, bruce.richardson, stephen
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v4] doc: announce API changes for
> Windows compatibility
>
> External email: Use caution opening links or attachments
>
>
> > 2021-08-02 12:45 (UTC+0000), Akhil Goyal:
> > > > 21/07/2021 21:55, Dmitry Kozlyuk:
> > > > > 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
> > > > > (example 1), as well as any usage of such fields (example 2). If
> > > > > DPDK headers undefined these macros, it could break consumer code
> (example 3).
> > > > > It is proposed to rename structure fields in DPDK, because Win32
> > headers
> > > > > are used more widely than DPDK, as a general-purpose platform
> > compared
> > > > > to domain-specific kit, and are harder to fix because of that.
> > > > > Exact new names are left for further discussion.
> > > > >
> > > > > Example 1:
> > > > >
> > > > > /* in DPDK public header included after windows.h */
> > > > > struct rte_type {
> > > > > int min; /* ERROR: `min` is a macro */
> > > > > };
> > > > >
> > > > > 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 */
> > > > >
> > > > > Example 3:
> > > > >
> > > > > #include <winsock2.h>
> > > > > #include <rte_ether.h>
> > > > > struct in_addr addr;
> > > > > addr.s_addr = 0; /* ERROR: there is no `s_addr` field,
> > > > > and `s_addr` macro is undefined by
> > > > > DPDK. */
> > > > >
> > > > > 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`.
> > > > >
> > > > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > > > Acked-by: Khoa To <khot@microsoft.com>
> > > > > ---
> > > > > +* 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.
> > > >
> > > > The struct rte_param_log2_range should also be renamed to include
> > > > "compress" prefix.
> > > > But as we break the struct API, it is not an issue I guess.
> > > >
> > > > > +* 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.
> > > >
> > > > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> > > >
> > > Can we have a local variable named as min/max?
> > > If not, then I believe it is not a good idea.
> >
> > Yes, except for inline functions in public headers.
> > The only problematic one I know of is this (rte_lru_x86.h):
> >
> > static inline int
> > f_lru_pos(uint64_t lru_list)
> > {
> > __m128i lst = _mm_set_epi64x((uint64_t)-1, lru_list);
> > __m128i min = _mm_minpos_epu16(lst); /* <<< */
> > return _mm_extract_epi16(min, 1); }
> >
> > Fixing it breaks neither API nor ABI, thus no explicit deprecation notice.
> OK,
> Acked-by: Akhil Goyal <gakhil@marvell.com>
>
> I hope when windows compilation is enabled, it will be part of CI and it will
> run on each patch which goes to patchworks.
Windows compilation is already part of CI in ci/iol-testing and ci/Intel-compilation.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH v4] doc: announce API changes for Windows compatibility
2021-08-02 13:48 ` Akhil Goyal
2021-08-02 14:57 ` Tal Shnaiderman
@ 2021-08-02 17:46 ` Thomas Monjalon
1 sibling, 0 replies; 31+ messages in thread
From: Thomas Monjalon @ 2021-08-02 17:46 UTC (permalink / raw)
To: Dmitry Kozlyuk
Cc: dev, Ferruh Yigit, Fiona Trahe, Khoa To, Ray Kinsella,
andrew.rybchenko, olivier.matz, navasile, pallavi.kadam,
ranjit.menon, bruce.richardson, stephen, Akhil Goyal
02/08/2021 15:48, Akhil Goyal:
> > 2021-08-02 12:45 (UTC+0000), Akhil Goyal:
> > > > 21/07/2021 21:55, Dmitry Kozlyuk:
> > > > > 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 (example 1),
> > > > > as well as any usage of such fields (example 2). If DPDK headers
> > > > > undefined these macros, it could break consumer code (example 3).
> > > > > It is proposed to rename structure fields in DPDK, because Win32
> > headers
> > > > > are used more widely than DPDK, as a general-purpose platform
> > compared
> > > > > to domain-specific kit, and are harder to fix because of that.
> > > > > Exact new names are left for further discussion.
> > > > >
> > > > > Example 1:
> > > > >
> > > > > /* in DPDK public header included after windows.h */
> > > > > struct rte_type {
> > > > > int min; /* ERROR: `min` is a macro */
> > > > > };
> > > > >
> > > > > 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 */
> > > > >
> > > > > Example 3:
> > > > >
> > > > > #include <winsock2.h>
> > > > > #include <rte_ether.h>
> > > > > struct in_addr addr;
> > > > > addr.s_addr = 0; /* ERROR: there is no `s_addr` field,
> > > > > and `s_addr` macro is undefined by DPDK. */
> > > > >
> > > > > 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`.
> > > > >
> > > > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > > > Acked-by: Khoa To <khot@microsoft.com>
[...]
> > > > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> > > >
> > > Can we have a local variable named as min/max?
> > > If not, then I believe it is not a good idea.
> >
> > Yes, except for inline functions in public headers.
> > The only problematic one I know of is this (rte_lru_x86.h):
> >
> > static inline int
> > f_lru_pos(uint64_t lru_list)
> > {
> > __m128i lst = _mm_set_epi64x((uint64_t)-1, lru_list);
> > __m128i min = _mm_minpos_epu16(lst); /* <<< */
> > return _mm_extract_epi16(min, 1);
> > }
> >
> > Fixing it breaks neither API nor ABI, thus no explicit deprecation notice.
> OK,
> Acked-by: Akhil Goyal <gakhil@marvell.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 31+ messages in thread