[v2] eal/windows: definition for ETOOMANYREFS errno

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

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>
  

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
  

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

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.
  

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

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.
  

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

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