[dpdk-dev] [PATCH] eal/windows: definition for ETOOMANYREFS errno

[dpdk-dev] [PATCH] eal/windows: definition for ETOOMANYREFS errno

[Tal Shnaiderman]

The ETOOMANYREFS errno is missing from the Windows clang build
is it used in initialization of flow error structures.

The commit will define it as it is done in the minGW Windows build.

Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
---
 lib/librte_eal/windows/include/rte_os.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/windows/include/rte_os.h b/lib/librte_eal/windows/include/rte_os.h
index 569ed92d51..2a91ebf6a1 100644
--- a/lib/librte_eal/windows/include/rte_os.h
+++ b/lib/librte_eal/windows/include/rte_os.h
@@ -90,6 +90,7 @@ eal_strerror(int code)
 }
 
 #define strerror eal_strerror
+#define ETOOMANYREFS WSAETOOMANYREFS
 
 #endif /* RTE_TOOLCHAIN_GCC */
 
-- 
2.16.1.windows.4

Re: [dpdk-dev] [PATCH] eal/windows: definition for ETOOMANYREFS errno

[Dmitry Kozlyuk]

On Sat, 14 Nov 2020 23:11:56 +0200, Tal Shnaiderman wrote:
> The ETOOMANYREFS errno is missing from the Windows clang build
> is it used in initialization of flow error structures.

"is it" -> "it is"
 
> The commit will define it as it is done in the minGW Windows build.

"The commit will" is unnecessary.

"minGW" -> "MinGW"
 
> Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> ---
>  lib/librte_eal/windows/include/rte_os.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_eal/windows/include/rte_os.h b/lib/librte_eal/windows/include/rte_os.h
> index 569ed92d51..2a91ebf6a1 100644
> --- a/lib/librte_eal/windows/include/rte_os.h
> +++ b/lib/librte_eal/windows/include/rte_os.h
> @@ -90,6 +90,7 @@ eal_strerror(int code)
>  }
>  
>  #define strerror eal_strerror
> +#define ETOOMANYREFS WSAETOOMANYREFS
>  
>  #endif /* RTE_TOOLCHAIN_GCC */

Should be #define ETOOMANYREFS 10059 /* WSAETOOMANYREFS */ for all toolchains:

1. Users of librte_ethdev, who check for ETOOMANYREFS, may not wish to include
<winsock2.h> because of its defines that break librte_net headers.

2. Now that I looked closely, MinGW-w64's #define ETOOMANYREFS
WSAETOOMANYREFS is under #if 0 clause (for documentation?). Apologies for
earlier misinformation.

Re: [dpdk-dev] [PATCH] eal/windows: definition for ETOOMANYREFS errno

[Tal Shnaiderman]

> Subject: Re: [PATCH] eal/windows: definition for ETOOMANYREFS errno
> 
> External email: Use caution opening links or attachments
> 
> 
> On Sat, 14 Nov 2020 23:11:56 +0200, Tal Shnaiderman wrote:
> > The ETOOMANYREFS errno is missing from the Windows clang build is it
> > used in initialization of flow error structures.
> 
> "is it" -> "it is"
> 
> > The commit will define it as it is done in the minGW Windows build.
> 
> "The commit will" is unnecessary.
> 
> "minGW" -> "MinGW"
> 
> > Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> > ---
> >  lib/librte_eal/windows/include/rte_os.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_eal/windows/include/rte_os.h
> > b/lib/librte_eal/windows/include/rte_os.h
> > index 569ed92d51..2a91ebf6a1 100644
> > --- a/lib/librte_eal/windows/include/rte_os.h
> > +++ b/lib/librte_eal/windows/include/rte_os.h
> > @@ -90,6 +90,7 @@ eal_strerror(int code)  }
> >
> >  #define strerror eal_strerror
> > +#define ETOOMANYREFS WSAETOOMANYREFS
> >
> >  #endif /* RTE_TOOLCHAIN_GCC */
> 
> Should be #define ETOOMANYREFS 10059 /* WSAETOOMANYREFS */ for all
> toolchains:
> 
> 1. Users of librte_ethdev, who check for ETOOMANYREFS, may not wish to
> include <winsock2.h> because of its defines that break librte_net headers.
> 
> 2. Now that I looked closely, MinGW-w64's #define ETOOMANYREFS
> WSAETOOMANYREFS is under #if 0 clause (for documentation?). Apologies
> for earlier misinformation.

Thank you for the comments Dmitry, will send a v2 promptly.

Re: [dpdk-dev] [PATCH] eal/windows: definition for ETOOMANYREFS errno

[Thomas Monjalon]

14/11/2020 23:11, Tal Shnaiderman:
> > On Sat, 14 Nov 2020 23:11:56 +0200, Tal Shnaiderman wrote:
> > > The ETOOMANYREFS errno is missing from the Windows clang build is it
> > > used in initialization of flow error structures.
> > 
> > "is it" -> "it is"
> > 
> > > The commit will define it as it is done in the minGW Windows build.
> > 
> > "The commit will" is unnecessary.
> > 
> > "minGW" -> "MinGW"

[...]
> Thank you for the comments Dmitry, will send a v2 promptly.

Just a detail, I see you missed this comment in v2:
	"The commit will" is unnecessary.

Re: [dpdk-dev] [PATCH] eal/windows: definition for ETOOMANYREFS errno

[Tal Shnaiderman]

> Subject: Re: [PATCH] eal/windows: definition for ETOOMANYREFS errno
> 
> External email: Use caution opening links or attachments
> 
> 
> 14/11/2020 23:11, Tal Shnaiderman:
> > > On Sat, 14 Nov 2020 23:11:56 +0200, Tal Shnaiderman wrote:
> > > > The ETOOMANYREFS errno is missing from the Windows clang build is
> > > > it used in initialization of flow error structures.
> > >
> > > "is it" -> "it is"
> > >
> > > > The commit will define it as it is done in the minGW Windows build.
> > >
> > > "The commit will" is unnecessary.
> > >
> > > "minGW" -> "MinGW"
> 
> [...]
> > Thank you for the comments Dmitry, will send a v2 promptly.
> 
> Just a detail, I see you missed this comment in v2:
>         "The commit will" is unnecessary.
> 

Yes I missed it, Thanks for noticing Thomas.

> 

[dpdk-dev] [PATCH v2] eal/windows: definition for ETOOMANYREFS errno

[Tal Shnaiderman]

The ETOOMANYREFS errno is missing from the Windows build.
it is used in initialization of flow error structures.

The commit will define it with the same error code used by
WSAETOOMANYREFS.

Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>

---
v2: log message fix, apply errno to both Windows builds
and remove dependency on winsock2.h [DmitryK]
---
---
 lib/librte_eal/windows/include/rte_os.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/windows/include/rte_os.h b/lib/librte_eal/windows/include/rte_os.h
index 569ed92d51..8300ea483a 100644
--- a/lib/librte_eal/windows/include/rte_os.h
+++ b/lib/librte_eal/windows/include/rte_os.h
@@ -51,6 +51,8 @@ extern "C" {
 /* as in <windows.h> */
 typedef long long ssize_t;
 
+#define ETOOMANYREFS 10059 /* WSAETOOMANYREFS */
+
 #ifndef RTE_TOOLCHAIN_GCC
 
 static inline int
-- 
2.16.1.windows.4

Re: [dpdk-dev] [PATCH v2] eal/windows: definition for ETOOMANYREFS errno

[Dmitry Kozlyuk]

On Sun, 15 Nov 2020 00:21:29 +0200, Tal Shnaiderman wrote:
> The ETOOMANYREFS errno is missing from the Windows build.
> it is used in initialization of flow error structures.
> 
> The commit will define it with the same error code used by
> WSAETOOMANYREFS.
> 
> Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> 
> ---
> v2: log message fix, apply errno to both Windows builds
> and remove dependency on winsock2.h [DmitryK]

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Re: [dpdk-dev] [PATCH v2] eal/windows: definition for ETOOMANYREFS errno

[Thomas Monjalon]

15/11/2020 00:13, Dmitry Kozlyuk:
> On Sun, 15 Nov 2020 00:21:29 +0200, Tal Shnaiderman wrote:
> > The ETOOMANYREFS errno is missing from the Windows build.
> > it is used in initialization of flow error structures.
> > 
> > The commit will define it with the same error code used by
> > WSAETOOMANYREFS.
> > 
> > Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
> > 
> > ---
> > v2: log message fix, apply errno to both Windows builds
> > and remove dependency on winsock2.h [DmitryK]
> 
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Applied, thanks

[dpdk-dev] Windows: A fundamental issue (was eal/windows: definition for ETOOMANYREFS errno)

[Nick Connolly]

Unfortunately, this change has broken the build for SPDK on Windows.

To use the DPDK libraries, an application needs to include the rte_* 
header, which includes rte_os.h via rte_common.h.  The definition of 
ETOOMANYREFS clashes with the value used when building the SPDK on Windows.

The fundamental issue here is how to support missing POSIX/Linux 
functionality.  If I understand correctly, the EAL should be responsible 
for providing all such functionality.  The support should be private to 
the EAL and only exported through rte_* definitions.

This means that rte_os.h should not include POSIX/Linux definitions to 
avoid clashes such as the one seen with this change.  It's clearly not 
sustainable if applications have to be modified every time we add more 
Windows support to the DPDK.

Note that this is not an isolated issue - most of the definitions in 
rte_os.h (redefining close, unlink, strdup etc) should not be present if 
other layers (application, other libraries, etc) are to be able to 
implement their own POSIX/Linux support.

Please can we back this change out until we have a strategy that allows 
us to make these definitions available for 'internal' use, but prevent 
them being visible outside of the DPDK tree.  If we can't wrap them with 
rte_* yet, perhaps the short term solution could be as simple as setting 
RTE_DEFINE_POSIX when building DPDK code and hiding them if it is not set?

Apologies that I didn't spot the issue earlier.

Thanks,
Nick


On 15/11/2020 23:10, Thomas Monjalon wrote:
> 15/11/2020 00:13, Dmitry Kozlyuk:
>> On Sun, 15 Nov 2020 00:21:29 +0200, Tal Shnaiderman wrote:
>>> The ETOOMANYREFS errno is missing from the Windows build.
>>> it is used in initialization of flow error structures.
>>>
>>> The commit will define it with the same error code used by
>>> WSAETOOMANYREFS.
>>>
>>> Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
>>>
>>> ---
>>> v2: log message fix, apply errno to both Windows builds
>>> and remove dependency on winsock2.h [DmitryK]
>> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Applied, thanks
>
>

Re: [dpdk-dev] Windows: A fundamental issue (was eal/windows: definition for ETOOMANYREFS errno)

[Dmitry Kozlyuk]

Hi Nick,

> This means that rte_os.h should not include POSIX/Linux definitions to 
> avoid clashes such as the one seen with this change.  It's clearly not 
> sustainable if applications have to be modified every time we add more 
> Windows support to the DPDK.
> 
> Note that this is not an isolated issue - most of the definitions in 
> rte_os.h (redefining close, unlink, strdup etc) should not be present if 
> other layers (application, other libraries, etc) are to be able to 
> implement their own POSIX/Linux support.

The purpose of rte_os.h must be clarified. It now says:

/**
 * This is header should contain any function/macro definition
 * which are not supported natively or named differently in the
 * ... OS. Functions will be added in future releases.
 */

This doesn't specify if the file should expose wrappers or POSIX-named
bits. Linux and FreeBSD, however, only use it for RTE_CPU_xxx() macros for
CPU_xxx() and don't define anything with POSIX names. So should Windows.

> Please can we back this change out until we have a strategy that allows 
> us to make these definitions available for 'internal' use, but prevent 
> them being visible outside of the DPDK tree.  If we can't wrap them with 
> rte_* yet, perhaps the short term solution could be as simple as setting 
> RTE_DEFINE_POSIX when building DPDK code and hiding them if it is not set?

You need the same value both inside DPDK to return it and outside of DPDK to
match on it. Returning an unnamed, unspecified code is not an option. RTE_
prefix is a way to go. We can just rename ETOOMANYREFS.

Strictly speaking, C standard defines very few errno, so using POSIX values
in API is incorrect anyway. It has to be deprecated and removed eventually,
we already had issues with MMAP_FAILED.

Re: [dpdk-dev] Windows: A fundamental issue (was eal/windows: definition for ETOOMANYREFS errno)

[Tal Shnaiderman]

> Subject: Re: Windows: A fundamental issue (was eal/windows: definition for
> ETOOMANYREFS errno)
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Nick,
> 
> > This means that rte_os.h should not include POSIX/Linux definitions to
> > avoid clashes such as the one seen with this change.  It's clearly not
> > sustainable if applications have to be modified every time we add more
> > Windows support to the DPDK.
> >
> > Note that this is not an isolated issue - most of the definitions in
> > rte_os.h (redefining close, unlink, strdup etc) should not be present
> > if other layers (application, other libraries, etc) are to be able to
> > implement their own POSIX/Linux support.
> 
> The purpose of rte_os.h must be clarified. It now says:
> 
> /**
>  * This is header should contain any function/macro definition
>  * which are not supported natively or named differently in the
>  * ... OS. Functions will be added in future releases.
>  */
> 
> This doesn't specify if the file should expose wrappers or POSIX-named bits.
> Linux and FreeBSD, however, only use it for RTE_CPU_xxx() macros for
> CPU_xxx() and don't define anything with POSIX names. So should Windows.
> 
> > Please can we back this change out until we have a strategy that
> > allows us to make these definitions available for 'internal' use, but
> > prevent them being visible outside of the DPDK tree.  If we can't wrap
> > them with
> > rte_* yet, perhaps the short term solution could be as simple as
> > setting RTE_DEFINE_POSIX when building DPDK code and hiding them if it is
> not set?
> 
> You need the same value both inside DPDK to return it and outside of DPDK
> to match on it. Returning an unnamed, unspecified code is not an option.
> RTE_ prefix is a way to go. We can just rename ETOOMANYREFS.

Thanks for the info Nick.
Dmitry, If we go with RTE_ETOOMANYREFS, I assume we need to define it for Linux and FreeBSD as well?

> 
> Strictly speaking, C standard defines very few errno, so using POSIX values in
> API is incorrect anyway. It has to be deprecated and removed eventually, we
> already had issues with MMAP_FAILED.

Re: [dpdk-dev] Windows: A fundamental issue (was eal/windows: definition for ETOOMANYREFS errno)

[Thomas Monjalon]

19/11/2020 14:21, Tal Shnaiderman:
> > Subject: Re: Windows: A fundamental issue (was eal/windows: definition for
> > ETOOMANYREFS errno)
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Nick,
> > 
> > > This means that rte_os.h should not include POSIX/Linux definitions to
> > > avoid clashes such as the one seen with this change.  It's clearly not
> > > sustainable if applications have to be modified every time we add more
> > > Windows support to the DPDK.
> > >
> > > Note that this is not an isolated issue - most of the definitions in
> > > rte_os.h (redefining close, unlink, strdup etc) should not be present
> > > if other layers (application, other libraries, etc) are to be able to
> > > implement their own POSIX/Linux support.
> > 
> > The purpose of rte_os.h must be clarified. It now says:
> > 
> > /**
> >  * This is header should contain any function/macro definition
> >  * which are not supported natively or named differently in the
> >  * ... OS. Functions will be added in future releases.
> >  */
> > 
> > This doesn't specify if the file should expose wrappers or POSIX-named bits.
> > Linux and FreeBSD, however, only use it for RTE_CPU_xxx() macros for
> > CPU_xxx() and don't define anything with POSIX names. So should Windows.
> > 
> > > Please can we back this change out until we have a strategy that
> > > allows us to make these definitions available for 'internal' use, but
> > > prevent them being visible outside of the DPDK tree.  If we can't wrap
> > > them with
> > > rte_* yet, perhaps the short term solution could be as simple as
> > > setting RTE_DEFINE_POSIX when building DPDK code and hiding them if it is
> > not set?
> > 
> > You need the same value both inside DPDK to return it and outside of DPDK
> > to match on it. Returning an unnamed, unspecified code is not an option.
> > RTE_ prefix is a way to go. We can just rename ETOOMANYREFS.
> 
> Thanks for the info Nick.
> Dmitry, If we go with RTE_ETOOMANYREFS, I assume we need to define it for Linux and FreeBSD as well?

Or we can use a "more standard" error code?


> > Strictly speaking, C standard defines very few errno, so using POSIX values in
> > API is incorrect anyway. It has to be deprecated and removed eventually, we
> > already had issues with MMAP_FAILED.

Re: [dpdk-dev] Windows: A fundamental issue (was eal/windows: definition for ETOOMANYREFS errno)

[Tal Shnaiderman]

> Subject: Re: Windows: A fundamental issue (was eal/windows: definition for
> ETOOMANYREFS errno)
> 
> External email: Use caution opening links or attachments
> 
> 
> 19/11/2020 14:21, Tal Shnaiderman:
> > > Subject: Re: Windows: A fundamental issue (was eal/windows:
> > > definition for ETOOMANYREFS errno)
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > Hi Nick,
> > >
> > > > This means that rte_os.h should not include POSIX/Linux
> > > > definitions to avoid clashes such as the one seen with this
> > > > change.  It's clearly not sustainable if applications have to be
> > > > modified every time we add more Windows support to the DPDK.
> > > >
> > > > Note that this is not an isolated issue - most of the definitions
> > > > in rte_os.h (redefining close, unlink, strdup etc) should not be
> > > > present if other layers (application, other libraries, etc) are to
> > > > be able to implement their own POSIX/Linux support.
> > >
> > > The purpose of rte_os.h must be clarified. It now says:
> > >
> > > /**
> > >  * This is header should contain any function/macro definition
> > >  * which are not supported natively or named differently in the
> > >  * ... OS. Functions will be added in future releases.
> > >  */
> > >
> > > This doesn't specify if the file should expose wrappers or POSIX-named
> bits.
> > > Linux and FreeBSD, however, only use it for RTE_CPU_xxx() macros for
> > > CPU_xxx() and don't define anything with POSIX names. So should
> Windows.
> > >
> > > > Please can we back this change out until we have a strategy that
> > > > allows us to make these definitions available for 'internal' use,
> > > > but prevent them being visible outside of the DPDK tree.  If we
> > > > can't wrap them with
> > > > rte_* yet, perhaps the short term solution could be as simple as
> > > > setting RTE_DEFINE_POSIX when building DPDK code and hiding them
> > > > if it is
> > > not set?
> > >
> > > You need the same value both inside DPDK to return it and outside of
> > > DPDK to match on it. Returning an unnamed, unspecified code is not an
> option.
> > > RTE_ prefix is a way to go. We can just rename ETOOMANYREFS.
> >
> > Thanks for the info Nick.
> > Dmitry, If we go with RTE_ETOOMANYREFS, I assume we need to define it
> for Linux and FreeBSD as well?
> 
> Or we can use a "more standard" error code?
> 

Right, Since it is used rarely and only in our PMD I'll work with the developer on selecting a different errno and will revert this commit, apologies for the inconvenience.

> 
> > > Strictly speaking, C standard defines very few errno, so using POSIX
> > > values in API is incorrect anyway. It has to be deprecated and
> > > removed eventually, we already had issues with MMAP_FAILED.
> 
> 

Re: [dpdk-dev] Windows: A fundamental issue (was eal/windows: definition for ETOOMANYREFS errno)

[Nick Connolly]

Thanks Tal.


On 19/11/2020 15:27, Tal Shnaiderman wrote:
>> Subject: Re: Windows: A fundamental issue (was eal/windows: definition for
>> ETOOMANYREFS errno)
>>
>> External email: Use caution opening links or attachments
>>
>>
>> 19/11/2020 14:21, Tal Shnaiderman:
>>>> Subject: Re: Windows: A fundamental issue (was eal/windows:
>>>> definition for ETOOMANYREFS errno)
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> Hi Nick,
>>>>
>>>>> This means that rte_os.h should not include POSIX/Linux
>>>>> definitions to avoid clashes such as the one seen with this
>>>>> change.  It's clearly not sustainable if applications have to be
>>>>> modified every time we add more Windows support to the DPDK.
>>>>>
>>>>> Note that this is not an isolated issue - most of the definitions
>>>>> in rte_os.h (redefining close, unlink, strdup etc) should not be
>>>>> present if other layers (application, other libraries, etc) are to
>>>>> be able to implement their own POSIX/Linux support.
>>>> The purpose of rte_os.h must be clarified. It now says:
>>>>
>>>> /**
>>>>   * This is header should contain any function/macro definition
>>>>   * which are not supported natively or named differently in the
>>>>   * ... OS. Functions will be added in future releases.
>>>>   */
>>>>
>>>> This doesn't specify if the file should expose wrappers or POSIX-named
>> bits.
>>>> Linux and FreeBSD, however, only use it for RTE_CPU_xxx() macros for
>>>> CPU_xxx() and don't define anything with POSIX names. So should
>> Windows.
>>>>> Please can we back this change out until we have a strategy that
>>>>> allows us to make these definitions available for 'internal' use,
>>>>> but prevent them being visible outside of the DPDK tree.  If we
>>>>> can't wrap them with
>>>>> rte_* yet, perhaps the short term solution could be as simple as
>>>>> setting RTE_DEFINE_POSIX when building DPDK code and hiding them
>>>>> if it is
>>>> not set?
>>>>
>>>> You need the same value both inside DPDK to return it and outside of
>>>> DPDK to match on it. Returning an unnamed, unspecified code is not an
>> option.
>>>> RTE_ prefix is a way to go. We can just rename ETOOMANYREFS.
>>> Thanks for the info Nick.
>>> Dmitry, If we go with RTE_ETOOMANYREFS, I assume we need to define it
>> for Linux and FreeBSD as well?
>>
>> Or we can use a "more standard" error code?
>>
> Right, Since it is used rarely and only in our PMD I'll work with the developer on selecting a different errno and will revert this commit, apologies for the inconvenience.
>
>>>> Strictly speaking, C standard defines very few errno, so using POSIX
>>>> values in API is incorrect anyway. It has to be deprecated and
>>>> removed eventually, we already had issues with MMAP_FAILED.
>>