diff mbox series

[v4] doc: announce API changes for Windows compatibility

Message ID 20210721195557.762726-2-dmitry.kozliuk@gmail.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series [v4] doc: announce API changes for Windows compatibility |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Dmitry Kozlyuk July 21, 2021, 7:55 p.m. UTC 
  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(+)

Comments

Thomas Monjalon Aug. 2, 2021, 12:13 p.m. UTC | #1 
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>
Akhil Goyal Aug. 2, 2021, 12:45 p.m. UTC | #2 
> 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.
Dmitry Kozlyuk Aug. 2, 2021, 1 p.m. UTC | #3 
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.
Akhil Goyal Aug. 2, 2021, 1:48 p.m. UTC | #4 
> 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.
Tal Shnaiderman Aug. 2, 2021, 2:57 p.m. UTC | #5 
> 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.
Thomas Monjalon Aug. 2, 2021, 5:46 p.m. UTC | #6 
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.
diff mbox series

Patch

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.