[dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Smoczynski, MarcinX]

Hi.
One of my patches submitted this week is breaking build on BSD systems. I dug
deeper and found out that it's because I'm using IPPROTO_* macros from
<netinet/in.h> in a header (rte_ip.h) which is included in the driver which uses
_XOPEN_SOURCE definition in its Makefile/meson.build.

On Linux and glibc this is not a problem, because:
1. Those IPPROTO macros are included unconditionally and
2. We're using _GNU_SOURCE at top level anyway which gives us visibility of
   every feature set. According to "features.h" from glibc 2.18
   (used in Ubuntu 18.04):

   https://github.molgen.mpg.de/git-mirror/glibc/blob/release/2.18/master/include/features.h
     /* If _GNU_SOURCE was defined by the user, turn on all the other features.  */
     #ifdef _GNU_SOURCE
     # undef  _ISOC95_SOURCE
     # define _ISOC95_SOURCE    1
     # undef  _ISOC99_SOURCE
     # define _ISOC99_SOURCE    1
     # undef  _ISOC11_SOURCE
     # define _ISOC11_SOURCE    1
     # undef  _POSIX_SOURCE
     # define _POSIX_SOURCE     1
     # undef  _POSIX_C_SOURCE
     # define _POSIX_C_SOURCE   200809L
     # undef  _XOPEN_SOURCE
     # define _XOPEN_SOURCE     700
     # undef  _XOPEN_SOURCE_EXTENDED
     # define _XOPEN_SOURCE_EXTENDED 1
     # undef    _LARGEFILE64_SOURCE
     # define _LARGEFILE64_SOURCE    1
     # undef  _BSD_SOURCE
     # define _BSD_SOURCE 1
     # undef  _SVID_SOURCE
     # define _SVID_SOURCE 1
     # undef  _ATFILE_SOURCE
     # define _ATFILE_SOURCE    1
     #endif

BSD systems are a little bit more orthodox:
1. IPPROTOs are defined conditionally if __BSD_VISIBLE macro is defined. If I
   understand it correctly, they are not a part of any POSIX specification
   and that's why they are kept separated.
2. __BSD_VISIBLE is set to 0 if any of XOPEN_SOURCE or POSIX_C_SOURCE macros
   are defined. The reasoning here is: if you don't specify feature set you want
   to use you have it all, but when you do, you have exactly what you've
   explicitly asked for. Using XOPEN_SOURCE=700 means: give me POSIX 2k8 and
   XSI only. In other words using this macro alone is restricting portability;
   by default DPDK is building with full visibility.

   https://github.com/freebsd/freebsd/blob/release/12.0.0/sys/sys/cdefs.h
   Lines 680 to 765
     /* Deal with various X/Open Portability Guides and Single UNIX Spec. */
     #ifdef _XOPEN_SOURCE
     #if _XOPEN_SOURCE - 0 >= 700
     #define    __XSI_VISIBLE        700
     #undef _POSIX_C_SOURCE
     #define    _POSIX_C_SOURCE      200809
     #elif _XOPEN_SOURCE - 0 >= 600
     #define    __XSI_VISIBLE        600
     #undef _POSIX_C_SOURCE
     #define    _POSIX_C_SOURCE      200112
     #elif _XOPEN_SOURCE - 0 >= 500
     #define    __XSI_VISIBLE        500
     #undef _POSIX_C_SOURCE
     #define    _POSIX_C_SOURCE      199506
     #endif
     #endif

     /*
     * Deal with all versions of POSIX.  The ordering relative to the tests above is
     * important.
     */
     #if defined(_POSIX_SOURCE) && !defined(_POSIX_C_SOURCE)
     #define    _POSIX_C_SOURCE      198808
     #endif
     #ifdef _POSIX_C_SOURCE
     #if _POSIX_C_SOURCE >= 200809
     #define    __POSIX_VISIBLE      200809
     #define    __ISO_C_VISIBLE      1999
     #elif _POSIX_C_SOURCE >= 200112
     #define    __POSIX_VISIBLE      200112
     #define    __ISO_C_VISIBLE      1999
     #elif _POSIX_C_SOURCE >= 199506
     #define    __POSIX_VISIBLE      199506
     #define    __ISO_C_VISIBLE      1990
     #elif _POSIX_C_SOURCE >= 199309
     #define    __POSIX_VISIBLE      199309
     #define    __ISO_C_VISIBLE      1990
     #elif _POSIX_C_SOURCE >= 199209
     #define    __POSIX_VISIBLE      199209
     #define    __ISO_C_VISIBLE      1990
     #elif _POSIX_C_SOURCE >= 199009
     #define    __POSIX_VISIBLE      199009
     #define    __ISO_C_VISIBLE      1990
     #else
     #define    __POSIX_VISIBLE      198808
     #define    __ISO_C_VISIBLE      0
     #endif /* _POSIX_C_SOURCE */
     #else                <----- !!!
     /*-
     * Deal with _ANSI_SOURCE:
     * If it is defined, and no other compilation environment is explicitly
     * requested, then define our internal feature-test macros to zero.  This
     * makes no difference to the preprocessor (undefined symbols in preprocessing
     * expressions are defined to have value zero), but makes it more convenient for
     * a test program to print out the values.
     *
     * If a program mistakenly defines _ANSI_SOURCE and some other macro such as
     * _POSIX_C_SOURCE, we will assume that it wants the broader compilation
     * environment (and in fact we will never get here).
     */
     #if defined(_ANSI_SOURCE)  /* Hide almost everything. */
     #define    __POSIX_VISIBLE      0
     #define    __XSI_VISIBLE        0
     #define    __BSD_VISIBLE        0
     #define    __ISO_C_VISIBLE      1990
     #define    __EXT1_VISIBLE       0
     #elif defined(_C99_SOURCE) /* Localism to specify strict C99 env. */
     #define    __POSIX_VISIBLE      0
     #define    __XSI_VISIBLE        0
     #define    __BSD_VISIBLE        0
     #define    __ISO_C_VISIBLE      1999
     #define    __EXT1_VISIBLE       0
     #elif defined(_C11_SOURCE) /* Localism to specify strict C11 env. */
     #define    __POSIX_VISIBLE      0
     #define    __XSI_VISIBLE        0
     #define    __BSD_VISIBLE        0
     #define    __ISO_C_VISIBLE      2011
     #define    __EXT1_VISIBLE       0
     #else                /* Default environment: show everything. */

     By default DPDK falls here:

     #define    __POSIX_VISIBLE      200809
     #define    __XSI_VISIBLE        700
     #define    __BSD_VISIBLE        1
     #define    __ISO_C_VISIBLE      2011
     #define    __EXT1_VISIBLE       1
     #endif
     #endif

To summarize we have different visibility sets for Linux and BSD when using
XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome this situation we can
either remove problematic XOPEN macros from mk/meson rules (drivers/net/failsafe,
drivers/net/mlx4, drivers/net/mlx5) or add explicit -D__BSD_VISIBLE when
building for BSD. I think also that defining _GNU_SOURCE for BSD builds makes no
sense although it does not cause any problems.

I have checked that removing those problematic macros solves build problems on
FreeBSD12 and does not break on Ubuntu 18.04.

I'd appreciate your thoughts on this topic.

Regards, Marcin.

[dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Smoczynski, MarcinX]

Hi.
One of my patches submitted this week is breaking build on BSD systems. I dug
deeper and found out that it's because I'm using IPPROTO_* macros from
<netinet/in.h> in a header (rte_ip.h) which is included in the driver which uses
_XOPEN_SOURCE definition in its Makefile/meson.build.

On Linux and glibc this is not a problem, because:
1. Those IPPROTO macros are included unconditionally and
2. We're using _GNU_SOURCE at top level anyway which gives us visibility of
   every feature set. According to "features.h" from glibc 2.18
   (used in Ubuntu 18.04):

   https://github.molgen.mpg.de/git-mirror/glibc/blob/release/2.18/master/include/features.h
     /* If _GNU_SOURCE was defined by the user, turn on all the other features.  */
     #ifdef _GNU_SOURCE
     # undef  _ISOC95_SOURCE
     # define _ISOC95_SOURCE    1
     # undef  _ISOC99_SOURCE
     # define _ISOC99_SOURCE    1
     # undef  _ISOC11_SOURCE
     # define _ISOC11_SOURCE    1
     # undef  _POSIX_SOURCE
     # define _POSIX_SOURCE     1
     # undef  _POSIX_C_SOURCE
     # define _POSIX_C_SOURCE   200809L
     # undef  _XOPEN_SOURCE
     # define _XOPEN_SOURCE     700
     # undef  _XOPEN_SOURCE_EXTENDED
     # define _XOPEN_SOURCE_EXTENDED 1
     # undef    _LARGEFILE64_SOURCE
     # define _LARGEFILE64_SOURCE    1
     # undef  _BSD_SOURCE
     # define _BSD_SOURCE 1
     # undef  _SVID_SOURCE
     # define _SVID_SOURCE 1
     # undef  _ATFILE_SOURCE
     # define _ATFILE_SOURCE    1
     #endif

BSD systems are a little bit more orthodox:
1. IPPROTOs are defined conditionally if __BSD_VISIBLE macro is defined. If I
   understand it correctly, they are not a part of any POSIX specification
   and that's why they are kept separated.
2. __BSD_VISIBLE is set to 0 if any of XOPEN_SOURCE or POSIX_C_SOURCE macros
   are defined. The reasoning here is: if you don't specify feature set you want
   to use you have it all, but when you do, you have exactly what you've
   explicitly asked for. Using XOPEN_SOURCE=700 means: give me POSIX 2k8 and
   XSI only. In other words using this macro alone is restricting portability;
   by default DPDK is building with full visibility.

   https://github.com/freebsd/freebsd/blob/release/12.0.0/sys/sys/cdefs.h
   Lines 680 to 765
     /* Deal with various X/Open Portability Guides and Single UNIX Spec. */
     #ifdef _XOPEN_SOURCE
     #if _XOPEN_SOURCE - 0 >= 700
     #define    __XSI_VISIBLE        700
     #undef _POSIX_C_SOURCE
     #define    _POSIX_C_SOURCE      200809
     #elif _XOPEN_SOURCE - 0 >= 600
     #define    __XSI_VISIBLE        600
     #undef _POSIX_C_SOURCE
     #define    _POSIX_C_SOURCE      200112
     #elif _XOPEN_SOURCE - 0 >= 500
     #define    __XSI_VISIBLE        500
     #undef _POSIX_C_SOURCE
     #define    _POSIX_C_SOURCE      199506
     #endif
     #endif

     /*
     * Deal with all versions of POSIX.  The ordering relative to the tests above is
     * important.
     */
     #if defined(_POSIX_SOURCE) && !defined(_POSIX_C_SOURCE)
     #define    _POSIX_C_SOURCE      198808
     #endif
     #ifdef _POSIX_C_SOURCE
     #if _POSIX_C_SOURCE >= 200809
     #define    __POSIX_VISIBLE      200809
     #define    __ISO_C_VISIBLE      1999
     #elif _POSIX_C_SOURCE >= 200112
     #define    __POSIX_VISIBLE      200112
     #define    __ISO_C_VISIBLE      1999
     #elif _POSIX_C_SOURCE >= 199506
     #define    __POSIX_VISIBLE      199506
     #define    __ISO_C_VISIBLE      1990
     #elif _POSIX_C_SOURCE >= 199309
     #define    __POSIX_VISIBLE      199309
     #define    __ISO_C_VISIBLE      1990
     #elif _POSIX_C_SOURCE >= 199209
     #define    __POSIX_VISIBLE      199209
     #define    __ISO_C_VISIBLE      1990
     #elif _POSIX_C_SOURCE >= 199009
     #define    __POSIX_VISIBLE      199009
     #define    __ISO_C_VISIBLE      1990
     #else
     #define    __POSIX_VISIBLE      198808
     #define    __ISO_C_VISIBLE      0
     #endif /* _POSIX_C_SOURCE */
     #else                <----- !!!
     /*-
     * Deal with _ANSI_SOURCE:
     * If it is defined, and no other compilation environment is explicitly
     * requested, then define our internal feature-test macros to zero.  This
     * makes no difference to the preprocessor (undefined symbols in preprocessing
     * expressions are defined to have value zero), but makes it more convenient for
     * a test program to print out the values.
     *
     * If a program mistakenly defines _ANSI_SOURCE and some other macro such as
     * _POSIX_C_SOURCE, we will assume that it wants the broader compilation
     * environment (and in fact we will never get here).
     */
     #if defined(_ANSI_SOURCE)  /* Hide almost everything. */
     #define    __POSIX_VISIBLE      0
     #define    __XSI_VISIBLE        0
     #define    __BSD_VISIBLE        0
     #define    __ISO_C_VISIBLE      1990
     #define    __EXT1_VISIBLE       0
     #elif defined(_C99_SOURCE) /* Localism to specify strict C99 env. */
     #define    __POSIX_VISIBLE      0
     #define    __XSI_VISIBLE        0
     #define    __BSD_VISIBLE        0
     #define    __ISO_C_VISIBLE      1999
     #define    __EXT1_VISIBLE       0
     #elif defined(_C11_SOURCE) /* Localism to specify strict C11 env. */
     #define    __POSIX_VISIBLE      0
     #define    __XSI_VISIBLE        0
     #define    __BSD_VISIBLE        0
     #define    __ISO_C_VISIBLE      2011
     #define    __EXT1_VISIBLE       0
     #else                /* Default environment: show everything. */

     By default DPDK falls here:

     #define    __POSIX_VISIBLE      200809
     #define    __XSI_VISIBLE        700
     #define    __BSD_VISIBLE        1
     #define    __ISO_C_VISIBLE      2011
     #define    __EXT1_VISIBLE       1
     #endif
     #endif

To summarize we have different visibility sets for Linux and BSD when using
XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome this situation we can
either remove problematic XOPEN macros from mk/meson rules (drivers/net/failsafe,
drivers/net/mlx4, drivers/net/mlx5) or add explicit -D__BSD_VISIBLE when
building for BSD. I think also that defining _GNU_SOURCE for BSD builds makes no
sense although it does not cause any problems.

I have checked that removing those problematic macros solves build problems on
FreeBSD12 and does not break on Ubuntu 18.04.

I'd appreciate your thoughts on this topic.

Regards, Marcin.

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Thomas Monjalon]

10/05/2019 19:14, Smoczynski, MarcinX:
> To summarize we have different visibility sets for Linux and BSD when using
> XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome this situation we can
> either remove problematic XOPEN macros from mk/meson rules (drivers/net/failsafe,
> drivers/net/mlx4, drivers/net/mlx5)

What is the consequence of removing these macros in mlx and failsafe PMDs?

> or add explicit -D__BSD_VISIBLE when
> building for BSD. I think also that defining _GNU_SOURCE for BSD builds makes no
> sense although it does not cause any problems.
> 
> I have checked that removing those problematic macros solves build problems on
> FreeBSD12 and does not break on Ubuntu 18.04.

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Thomas Monjalon]

10/05/2019 19:14, Smoczynski, MarcinX:
> To summarize we have different visibility sets for Linux and BSD when using
> XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome this situation we can
> either remove problematic XOPEN macros from mk/meson rules (drivers/net/failsafe,
> drivers/net/mlx4, drivers/net/mlx5)

What is the consequence of removing these macros in mlx and failsafe PMDs?

> or add explicit -D__BSD_VISIBLE when
> building for BSD. I think also that defining _GNU_SOURCE for BSD builds makes no
> sense although it does not cause any problems.
> 
> I have checked that removing those problematic macros solves build problems on
> FreeBSD12 and does not break on Ubuntu 18.04.

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Thomas Monjalon]

re-send with fixed Cc

10/05/2019 20:17, Thomas Monjalon:
> 10/05/2019 19:14, Smoczynski, MarcinX:
> > To summarize we have different visibility sets for Linux and BSD when using
> > XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome this situation we can
> > either remove problematic XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > drivers/net/mlx4, drivers/net/mlx5)
> 
> What is the consequence of removing these macros in mlx and failsafe PMDs?
> 
> > or add explicit -D__BSD_VISIBLE when
> > building for BSD. I think also that defining _GNU_SOURCE for BSD builds makes no
> > sense although it does not cause any problems.
> > 
> > I have checked that removing those problematic macros solves build problems on
> > FreeBSD12 and does not break on Ubuntu 18.04.

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Thomas Monjalon]

re-send with fixed Cc

10/05/2019 20:17, Thomas Monjalon:
> 10/05/2019 19:14, Smoczynski, MarcinX:
> > To summarize we have different visibility sets for Linux and BSD when using
> > XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome this situation we can
> > either remove problematic XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > drivers/net/mlx4, drivers/net/mlx5)
> 
> What is the consequence of removing these macros in mlx and failsafe PMDs?
> 
> > or add explicit -D__BSD_VISIBLE when
> > building for BSD. I think also that defining _GNU_SOURCE for BSD builds makes no
> > sense although it does not cause any problems.
> > 
> > I have checked that removing those problematic macros solves build problems on
> > FreeBSD12 and does not break on Ubuntu 18.04.

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Smoczynski, MarcinX]

10/05/2019 20:17, Thomas Monjalon:
> 10/05/2019 19:14, Smoczynski, MarcinX:
> > To summarize we have different visibility sets for Linux and BSD 
> > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome 
> > this situation we can either remove problematic XOPEN macros from 
> > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4, 
> > drivers/net/mlx5)
> 
> What is the consequence of removing these macros in mlx and failsafe PMDs?

The purpose of these *_SOURCE constants is to enable particular feature sets
visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
consequences. On BSD it will unify feature sets visibility with the rest of
sources. Can't think of any downsides here.

I believe XOPEN_SOURCE was introduced to extend features not to restrict them.

MS

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Smoczynski, MarcinX]

10/05/2019 20:17, Thomas Monjalon:
> 10/05/2019 19:14, Smoczynski, MarcinX:
> > To summarize we have different visibility sets for Linux and BSD 
> > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome 
> > this situation we can either remove problematic XOPEN macros from 
> > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4, 
> > drivers/net/mlx5)
> 
> What is the consequence of removing these macros in mlx and failsafe PMDs?

The purpose of these *_SOURCE constants is to enable particular feature sets
visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
consequences. On BSD it will unify feature sets visibility with the rest of
sources. Can't think of any downsides here.

I believe XOPEN_SOURCE was introduced to extend features not to restrict them.

MS

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Adrien Mazarguil]

On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX wrote:
> 10/05/2019 20:17, Thomas Monjalon:
> > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > To summarize we have different visibility sets for Linux and BSD 
> > > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome 
> > > this situation we can either remove problematic XOPEN macros from 
> > > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4, 
> > > drivers/net/mlx5)
> > 
> > What is the consequence of removing these macros in mlx and failsafe PMDs?
> 
> The purpose of these *_SOURCE constants is to enable particular feature sets
> visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
> consequences. On BSD it will unify feature sets visibility with the rest of
> sources. Can't think of any downsides here.
> 
> I believe XOPEN_SOURCE was introduced to extend features not to restrict them.

I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700) are defined
regardless (_GNU_SOURCE not even needed), while under FreeBSD, the non-POSIX
versions are only defined when __BSD_VISIBLE is set.

The FreeBSD behavior is more correct in this respect since the purpose of
_XOPEN_SOURCE and friends is also to let applications limit the risk of
redefinitions in case they were written for an earlier standard
(e.g. -D_XOPEN_SOURCE=500 vs. -D_XOPEN_SOURCE=600).

DPDK applications may also define _XOPEN_SOURCE for their own needs. They
should still be able to use rte_ip.h afterward. I think this reason is
enough to go with -D__BSD_VISIBLE under FreeBSD without removing
_XOPEN_SOURCE, as it should work regardless.

Looking at the patch [1], I also think there's another, simpler approach:
unless really performance critical, defining rte_ipv6_get_next_ext() in
rte_net.c instead of a static inline in rte_ip.h should address this issue.

[1] "[PATCH 1/3] net: new ipv6 header extension parsing function"
    https://mails.dpdk.org/archives/dev/2019-May/131885.html

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Adrien Mazarguil]

On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX wrote:
> 10/05/2019 20:17, Thomas Monjalon:
> > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > To summarize we have different visibility sets for Linux and BSD 
> > > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome 
> > > this situation we can either remove problematic XOPEN macros from 
> > > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4, 
> > > drivers/net/mlx5)
> > 
> > What is the consequence of removing these macros in mlx and failsafe PMDs?
> 
> The purpose of these *_SOURCE constants is to enable particular feature sets
> visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
> consequences. On BSD it will unify feature sets visibility with the rest of
> sources. Can't think of any downsides here.
> 
> I believe XOPEN_SOURCE was introduced to extend features not to restrict them.

I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700) are defined
regardless (_GNU_SOURCE not even needed), while under FreeBSD, the non-POSIX
versions are only defined when __BSD_VISIBLE is set.

The FreeBSD behavior is more correct in this respect since the purpose of
_XOPEN_SOURCE and friends is also to let applications limit the risk of
redefinitions in case they were written for an earlier standard
(e.g. -D_XOPEN_SOURCE=500 vs. -D_XOPEN_SOURCE=600).

DPDK applications may also define _XOPEN_SOURCE for their own needs. They
should still be able to use rte_ip.h afterward. I think this reason is
enough to go with -D__BSD_VISIBLE under FreeBSD without removing
_XOPEN_SOURCE, as it should work regardless.

Looking at the patch [1], I also think there's another, simpler approach:
unless really performance critical, defining rte_ipv6_get_next_ext() in
rte_net.c instead of a static inline in rte_ip.h should address this issue.

[1] "[PATCH 1/3] net: new ipv6 header extension parsing function"
    https://mails.dpdk.org/archives/dev/2019-May/131885.html

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Ananyev, Konstantin]

Hi Adrien,

> 
> On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX wrote:
> > 10/05/2019 20:17, Thomas Monjalon:
> > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > To summarize we have different visibility sets for Linux and BSD
> > > > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome
> > > > this situation we can either remove problematic XOPEN macros from
> > > > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4,
> > > > drivers/net/mlx5)
> > >
> > > What is the consequence of removing these macros in mlx and failsafe PMDs?
> >
> > The purpose of these *_SOURCE constants is to enable particular feature sets
> > visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
> > consequences. On BSD it will unify feature sets visibility with the rest of
> > sources. Can't think of any downsides here.
> >
> > I believe XOPEN_SOURCE was introduced to extend features not to restrict them.
> 
> I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700) are defined
> regardless (_GNU_SOURCE not even needed), while under FreeBSD, the non-POSIX
> versions are only defined when __BSD_VISIBLE is set.
> 
> The FreeBSD behavior is more correct in this respect since the purpose of
> _XOPEN_SOURCE and friends is also to let applications limit the risk of
> redefinitions in case they were written for an earlier standard
> (e.g. -D_XOPEN_SOURCE=500 vs. -D_XOPEN_SOURCE=600).

Still not sure why do you need it for failsafe and mlx PMDs?
Would something in these PMDs be broken without  '-D_XOPEN_SOURCE=600'?

> 
> DPDK applications may also define _XOPEN_SOURCE for their own needs. They
> should still be able to use rte_ip.h afterward.

I suppose they can, they would just have (on FreeBSD) to add '-D __BSD_VISIBLE'
themselves. 

> I think this reason is
> enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> _XOPEN_SOURCE, as it should work regardless.

So do you suggest to add '-D __BSD_VISIBLE'  into
mlx/failsafe PMDs Makefiles/meson.build, or ... ?
 
> Looking at the patch [1], I also think there's another, simpler approach:
> unless really performance critical, defining rte_ipv6_get_next_ext() in
> rte_net.c instead of a static inline in rte_ip.h should address this issue.

It is performance critical, and I think that 
function call for each ext header is a way too expensive approach.
Will prefer to keep that function inline.

Konstantin

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Ananyev, Konstantin]

Hi Adrien,

> 
> On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX wrote:
> > 10/05/2019 20:17, Thomas Monjalon:
> > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > To summarize we have different visibility sets for Linux and BSD
> > > > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome
> > > > this situation we can either remove problematic XOPEN macros from
> > > > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4,
> > > > drivers/net/mlx5)
> > >
> > > What is the consequence of removing these macros in mlx and failsafe PMDs?
> >
> > The purpose of these *_SOURCE constants is to enable particular feature sets
> > visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
> > consequences. On BSD it will unify feature sets visibility with the rest of
> > sources. Can't think of any downsides here.
> >
> > I believe XOPEN_SOURCE was introduced to extend features not to restrict them.
> 
> I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700) are defined
> regardless (_GNU_SOURCE not even needed), while under FreeBSD, the non-POSIX
> versions are only defined when __BSD_VISIBLE is set.
> 
> The FreeBSD behavior is more correct in this respect since the purpose of
> _XOPEN_SOURCE and friends is also to let applications limit the risk of
> redefinitions in case they were written for an earlier standard
> (e.g. -D_XOPEN_SOURCE=500 vs. -D_XOPEN_SOURCE=600).

Still not sure why do you need it for failsafe and mlx PMDs?
Would something in these PMDs be broken without  '-D_XOPEN_SOURCE=600'?

> 
> DPDK applications may also define _XOPEN_SOURCE for their own needs. They
> should still be able to use rte_ip.h afterward.

I suppose they can, they would just have (on FreeBSD) to add '-D __BSD_VISIBLE'
themselves. 

> I think this reason is
> enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> _XOPEN_SOURCE, as it should work regardless.

So do you suggest to add '-D __BSD_VISIBLE'  into
mlx/failsafe PMDs Makefiles/meson.build, or ... ?
 
> Looking at the patch [1], I also think there's another, simpler approach:
> unless really performance critical, defining rte_ipv6_get_next_ext() in
> rte_net.c instead of a static inline in rte_ip.h should address this issue.

It is performance critical, and I think that 
function call for each ext header is a way too expensive approach.
Will prefer to keep that function inline.

Konstantin

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Adrien Mazarguil]

Hey Konstantin,

On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> Hi Adrien,
> 
> > 
> > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX wrote:
> > > 10/05/2019 20:17, Thomas Monjalon:
> > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > To summarize we have different visibility sets for Linux and BSD
> > > > > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome
> > > > > this situation we can either remove problematic XOPEN macros from
> > > > > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4,
> > > > > drivers/net/mlx5)
> > > >
> > > > What is the consequence of removing these macros in mlx and failsafe PMDs?
> > >
> > > The purpose of these *_SOURCE constants is to enable particular feature sets
> > > visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
> > > consequences. On BSD it will unify feature sets visibility with the rest of
> > > sources. Can't think of any downsides here.
> > >
> > > I believe XOPEN_SOURCE was introduced to extend features not to restrict them.
> > 
> > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700) are defined
> > regardless (_GNU_SOURCE not even needed), while under FreeBSD, the non-POSIX
> > versions are only defined when __BSD_VISIBLE is set.
> > 
> > The FreeBSD behavior is more correct in this respect since the purpose of
> > _XOPEN_SOURCE and friends is also to let applications limit the risk of
> > redefinitions in case they were written for an earlier standard
> > (e.g. -D_XOPEN_SOURCE=500 vs. -D_XOPEN_SOURCE=600).
> 
> Still not sure why do you need it for failsafe and mlx PMDs?
> Would something in these PMDs be broken without  '-D_XOPEN_SOURCE=600'?

Well, not really. At least not anymore if they compile fine without it on
all supported targets. I don't mind if they are removed from PMDs.

_XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by mlx5 and
failsafe) for the following reasons:

- Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
  to be exposed. Some affected definitions were likely needed at some point.

- Besides toggling C syntax extensions, forcing a C standard through the
  -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
  C compliance disables the implicit presence of nonstandard definitions,
  which must be re-enabled as needed through the appropriate #defines.

For instance, including unistd.h for getsid() stops working as soon as you
use -std=c99. On Linux you can get it back through -std=gnu99 or by
combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The latter was
chosen because it is the standard define supposed to work across OSes.

Historically mlx4 had to enable -std=c99 to be able to use various features
not present when GCC defaulted to -std=gnu90. It was later transformed to
-std=c11 for similar reasons (anonymous members in structs/unions if memory
serves me right).

> > DPDK applications may also define _XOPEN_SOURCE for their own needs. They
> > should still be able to use rte_ip.h afterward.
> 
> I suppose they can, they would just have (on FreeBSD) to add '-D __BSD_VISIBLE'
> themselves. 

Of course, but public headers should be as self sufficient as possible.
Unless they provide really insane compiler flags, if user applications get
compilation errors after including some header we install on the system,
I think the blame is on DPDK.

> > I think this reason is
> > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > _XOPEN_SOURCE, as it should work regardless.
> 
> So do you suggest to add '-D __BSD_VISIBLE'  into
> mlx/failsafe PMDs Makefiles/meson.build, or ... ?

Since headers of our public API potentially require it, it must be defined
globally (unlike _XOPEN_SOURCE which is only local to a few PMDs):
app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk, alongside
-D_GNU_SOURCE.

Add it to mlx*/failsafe only if that's not enough. Just make sure
applications inherit this flag.

> > Looking at the patch [1], I also think there's another, simpler approach:
> > unless really performance critical, defining rte_ipv6_get_next_ext() in
> > rte_net.c instead of a static inline in rte_ip.h should address this issue.
> 
> It is performance critical, and I think that 
> function call for each ext header is a way too expensive approach.
> Will prefer to keep that function inline.

OK, a bit cumbersome but since we're heading this way [2], how about
defining our own instead of all the above?

 #define RTE_IPPROTO_HOPOPTS 0
 #define RTE_IPPROTO_ROUTING 43
 ...

Which could prove handy later as it appears Linux and FreeBSD don't have the
same set of available IPPROTO_* definitions.

Thoughts?

[2] "[RFC v2 00/14] prefix network structures"
    https://mails.dpdk.org/archives/dev/2019-April/129752.html

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Adrien Mazarguil]

Hey Konstantin,

On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> Hi Adrien,
> 
> > 
> > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX wrote:
> > > 10/05/2019 20:17, Thomas Monjalon:
> > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > To summarize we have different visibility sets for Linux and BSD
> > > > > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome
> > > > > this situation we can either remove problematic XOPEN macros from
> > > > > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4,
> > > > > drivers/net/mlx5)
> > > >
> > > > What is the consequence of removing these macros in mlx and failsafe PMDs?
> > >
> > > The purpose of these *_SOURCE constants is to enable particular feature sets
> > > visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
> > > consequences. On BSD it will unify feature sets visibility with the rest of
> > > sources. Can't think of any downsides here.
> > >
> > > I believe XOPEN_SOURCE was introduced to extend features not to restrict them.
> > 
> > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700) are defined
> > regardless (_GNU_SOURCE not even needed), while under FreeBSD, the non-POSIX
> > versions are only defined when __BSD_VISIBLE is set.
> > 
> > The FreeBSD behavior is more correct in this respect since the purpose of
> > _XOPEN_SOURCE and friends is also to let applications limit the risk of
> > redefinitions in case they were written for an earlier standard
> > (e.g. -D_XOPEN_SOURCE=500 vs. -D_XOPEN_SOURCE=600).
> 
> Still not sure why do you need it for failsafe and mlx PMDs?
> Would something in these PMDs be broken without  '-D_XOPEN_SOURCE=600'?

Well, not really. At least not anymore if they compile fine without it on
all supported targets. I don't mind if they are removed from PMDs.

_XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by mlx5 and
failsafe) for the following reasons:

- Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
  to be exposed. Some affected definitions were likely needed at some point.

- Besides toggling C syntax extensions, forcing a C standard through the
  -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
  C compliance disables the implicit presence of nonstandard definitions,
  which must be re-enabled as needed through the appropriate #defines.

For instance, including unistd.h for getsid() stops working as soon as you
use -std=c99. On Linux you can get it back through -std=gnu99 or by
combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The latter was
chosen because it is the standard define supposed to work across OSes.

Historically mlx4 had to enable -std=c99 to be able to use various features
not present when GCC defaulted to -std=gnu90. It was later transformed to
-std=c11 for similar reasons (anonymous members in structs/unions if memory
serves me right).

> > DPDK applications may also define _XOPEN_SOURCE for their own needs. They
> > should still be able to use rte_ip.h afterward.
> 
> I suppose they can, they would just have (on FreeBSD) to add '-D __BSD_VISIBLE'
> themselves. 

Of course, but public headers should be as self sufficient as possible.
Unless they provide really insane compiler flags, if user applications get
compilation errors after including some header we install on the system,
I think the blame is on DPDK.

> > I think this reason is
> > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > _XOPEN_SOURCE, as it should work regardless.
> 
> So do you suggest to add '-D __BSD_VISIBLE'  into
> mlx/failsafe PMDs Makefiles/meson.build, or ... ?

Since headers of our public API potentially require it, it must be defined
globally (unlike _XOPEN_SOURCE which is only local to a few PMDs):
app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk, alongside
-D_GNU_SOURCE.

Add it to mlx*/failsafe only if that's not enough. Just make sure
applications inherit this flag.

> > Looking at the patch [1], I also think there's another, simpler approach:
> > unless really performance critical, defining rte_ipv6_get_next_ext() in
> > rte_net.c instead of a static inline in rte_ip.h should address this issue.
> 
> It is performance critical, and I think that 
> function call for each ext header is a way too expensive approach.
> Will prefer to keep that function inline.

OK, a bit cumbersome but since we're heading this way [2], how about
defining our own instead of all the above?

 #define RTE_IPPROTO_HOPOPTS 0
 #define RTE_IPPROTO_ROUTING 43
 ...

Which could prove handy later as it appears Linux and FreeBSD don't have the
same set of available IPPROTO_* definitions.

Thoughts?

[2] "[RFC v2 00/14] prefix network structures"
    https://mails.dpdk.org/archives/dev/2019-April/129752.html

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Ananyev, Konstantin]

 
> Hey Konstantin,
> 
> On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > Hi Adrien,
> >
> > >
> > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX wrote:
> > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > To summarize we have different visibility sets for Linux and BSD
> > > > > > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome
> > > > > > this situation we can either remove problematic XOPEN macros from
> > > > > > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4,
> > > > > > drivers/net/mlx5)
> > > > >
> > > > > What is the consequence of removing these macros in mlx and failsafe PMDs?
> > > >
> > > > The purpose of these *_SOURCE constants is to enable particular feature sets
> > > > visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
> > > > consequences. On BSD it will unify feature sets visibility with the rest of
> > > > sources. Can't think of any downsides here.
> > > >
> > > > I believe XOPEN_SOURCE was introduced to extend features not to restrict them.
> > >
> > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700) are defined
> > > regardless (_GNU_SOURCE not even needed), while under FreeBSD, the non-POSIX
> > > versions are only defined when __BSD_VISIBLE is set.
> > >
> > > The FreeBSD behavior is more correct in this respect since the purpose of
> > > _XOPEN_SOURCE and friends is also to let applications limit the risk of
> > > redefinitions in case they were written for an earlier standard
> > > (e.g. -D_XOPEN_SOURCE=500 vs. -D_XOPEN_SOURCE=600).
> >
> > Still not sure why do you need it for failsafe and mlx PMDs?
> > Would something in these PMDs be broken without  '-D_XOPEN_SOURCE=600'?
> 
> Well, not really. At least not anymore if they compile fine without it on
> all supported targets. I don't mind if they are removed from PMDs.
> 
> _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by mlx5 and
> failsafe) for the following reasons:
> 
> - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
>   to be exposed. Some affected definitions were likely needed at some point.
> 
> - Besides toggling C syntax extensions, forcing a C standard through the
>   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
>   C compliance disables the implicit presence of nonstandard definitions,
>   which must be re-enabled as needed through the appropriate #defines.
> 
> For instance, including unistd.h for getsid() stops working as soon as you
> use -std=c99. On Linux you can get it back through -std=gnu99 or by
> combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The latter was
> chosen because it is the standard define supposed to work across OSes.
> 
> Historically mlx4 had to enable -std=c99 to be able to use various features
> not present when GCC defaulted to -std=gnu90. It was later transformed to
> -std=c11 for similar reasons (anonymous members in structs/unions if memory
> serves me right).
> 
> > > DPDK applications may also define _XOPEN_SOURCE for their own needs. They
> > > should still be able to use rte_ip.h afterward.
> >
> > I suppose they can, they would just have (on FreeBSD) to add '-D __BSD_VISIBLE'
> > themselves.
> 
> Of course, but public headers should be as self sufficient as possible.
> Unless they provide really insane compiler flags, if user applications get
> compilation errors after including some header we install on the system,
> I think the blame is on DPDK.
> 
> > > I think this reason is
> > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > _XOPEN_SOURCE, as it should work regardless.
> >
> > So do you suggest to add '-D __BSD_VISIBLE'  into
> > mlx/failsafe PMDs Makefiles/meson.build, or ... ?
> 
> Since headers of our public API potentially require it, it must be defined
> globally (unlike _XOPEN_SOURCE which is only local to a few PMDs):
> app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk, alongside
> -D_GNU_SOURCE.
> 
> Add it to mlx*/failsafe only if that's not enough. Just make sure
> applications inherit this flag.

Ok, to summarize, eyour suggestion is:
1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
2. add '-D __BSD_VISIBLE'  into top level make/meson files
(app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk)
Similar to what we doing for -DGNU_SOURCE.

If I understand you correctly, then it sounds ok to me.

> 
> > > Looking at the patch [1], I also think there's another, simpler approach:
> > > unless really performance critical, defining rte_ipv6_get_next_ext() in
> > > rte_net.c instead of a static inline in rte_ip.h should address this issue.
> >
> > It is performance critical, and I think that
> > function call for each ext header is a way too expensive approach.
> > Will prefer to keep that function inline.
> 
> OK, a bit cumbersome but since we're heading this way [2], how about
> defining our own instead of all the above?
> 
>  #define RTE_IPPROTO_HOPOPTS 0
>  #define RTE_IPPROTO_ROUTING 43
>  ...
> 
> Which could prove handy later as it appears Linux and FreeBSD don't have the
> same set of available IPPROTO_* definitions.
> 
> Thoughts?
> 
> [2] "[RFC v2 00/14] prefix network structures"
>     https://mails.dpdk.org/archives/dev/2019-April/129752.html

Yep, that's definitely an option too.
But if we going to replace all current references of IPPROTO_ inside DPDK 
to RTE_IPROTO_ - the change will be massive.
And for sure it is out of scope of this patch series.
That's probably need to be done after Olivier RFC will be in and
should be subject of a separate patch series.
Konstantin

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Ananyev, Konstantin]

 
> Hey Konstantin,
> 
> On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > Hi Adrien,
> >
> > >
> > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX wrote:
> > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > To summarize we have different visibility sets for Linux and BSD
> > > > > > when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly. To overcome
> > > > > > this situation we can either remove problematic XOPEN macros from
> > > > > > mk/meson rules (drivers/net/failsafe, drivers/net/mlx4,
> > > > > > drivers/net/mlx5)
> > > > >
> > > > > What is the consequence of removing these macros in mlx and failsafe PMDs?
> > > >
> > > > The purpose of these *_SOURCE constants is to enable particular feature sets
> > > > visibility. As long as we have GNU_SOURCE on Linux removing it won't have any
> > > > consequences. On BSD it will unify feature sets visibility with the rest of
> > > > sources. Can't think of any downsides here.
> > > >
> > > > I believe XOPEN_SOURCE was introduced to extend features not to restrict them.
> > >
> > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700) are defined
> > > regardless (_GNU_SOURCE not even needed), while under FreeBSD, the non-POSIX
> > > versions are only defined when __BSD_VISIBLE is set.
> > >
> > > The FreeBSD behavior is more correct in this respect since the purpose of
> > > _XOPEN_SOURCE and friends is also to let applications limit the risk of
> > > redefinitions in case they were written for an earlier standard
> > > (e.g. -D_XOPEN_SOURCE=500 vs. -D_XOPEN_SOURCE=600).
> >
> > Still not sure why do you need it for failsafe and mlx PMDs?
> > Would something in these PMDs be broken without  '-D_XOPEN_SOURCE=600'?
> 
> Well, not really. At least not anymore if they compile fine without it on
> all supported targets. I don't mind if they are removed from PMDs.
> 
> _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by mlx5 and
> failsafe) for the following reasons:
> 
> - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
>   to be exposed. Some affected definitions were likely needed at some point.
> 
> - Besides toggling C syntax extensions, forcing a C standard through the
>   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
>   C compliance disables the implicit presence of nonstandard definitions,
>   which must be re-enabled as needed through the appropriate #defines.
> 
> For instance, including unistd.h for getsid() stops working as soon as you
> use -std=c99. On Linux you can get it back through -std=gnu99 or by
> combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The latter was
> chosen because it is the standard define supposed to work across OSes.
> 
> Historically mlx4 had to enable -std=c99 to be able to use various features
> not present when GCC defaulted to -std=gnu90. It was later transformed to
> -std=c11 for similar reasons (anonymous members in structs/unions if memory
> serves me right).
> 
> > > DPDK applications may also define _XOPEN_SOURCE for their own needs. They
> > > should still be able to use rte_ip.h afterward.
> >
> > I suppose they can, they would just have (on FreeBSD) to add '-D __BSD_VISIBLE'
> > themselves.
> 
> Of course, but public headers should be as self sufficient as possible.
> Unless they provide really insane compiler flags, if user applications get
> compilation errors after including some header we install on the system,
> I think the blame is on DPDK.
> 
> > > I think this reason is
> > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > _XOPEN_SOURCE, as it should work regardless.
> >
> > So do you suggest to add '-D __BSD_VISIBLE'  into
> > mlx/failsafe PMDs Makefiles/meson.build, or ... ?
> 
> Since headers of our public API potentially require it, it must be defined
> globally (unlike _XOPEN_SOURCE which is only local to a few PMDs):
> app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk, alongside
> -D_GNU_SOURCE.
> 
> Add it to mlx*/failsafe only if that's not enough. Just make sure
> applications inherit this flag.

Ok, to summarize, eyour suggestion is:
1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
2. add '-D __BSD_VISIBLE'  into top level make/meson files
(app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk)
Similar to what we doing for -DGNU_SOURCE.

If I understand you correctly, then it sounds ok to me.

> 
> > > Looking at the patch [1], I also think there's another, simpler approach:
> > > unless really performance critical, defining rte_ipv6_get_next_ext() in
> > > rte_net.c instead of a static inline in rte_ip.h should address this issue.
> >
> > It is performance critical, and I think that
> > function call for each ext header is a way too expensive approach.
> > Will prefer to keep that function inline.
> 
> OK, a bit cumbersome but since we're heading this way [2], how about
> defining our own instead of all the above?
> 
>  #define RTE_IPPROTO_HOPOPTS 0
>  #define RTE_IPPROTO_ROUTING 43
>  ...
> 
> Which could prove handy later as it appears Linux and FreeBSD don't have the
> same set of available IPPROTO_* definitions.
> 
> Thoughts?
> 
> [2] "[RFC v2 00/14] prefix network structures"
>     https://mails.dpdk.org/archives/dev/2019-April/129752.html

Yep, that's definitely an option too.
But if we going to replace all current references of IPPROTO_ inside DPDK 
to RTE_IPROTO_ - the change will be massive.
And for sure it is out of scope of this patch series.
That's probably need to be done after Olivier RFC will be in and
should be subject of a separate patch series.
Konstantin

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Smoczynski, MarcinX]

> > Hey Konstantin,
> >
> > On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > > Hi Adrien,
> > >
> > > >
> > > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX
> wrote:
> > > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > > To summarize we have different visibility sets for Linux and
> > > > > > > BSD when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly.
> To
> > > > > > > overcome this situation we can either remove problematic
> > > > > > > XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > > > > > > drivers/net/mlx4,
> > > > > > > drivers/net/mlx5)
> > > > > >
> > > > > > What is the consequence of removing these macros in mlx and
> failsafe PMDs?
> > > > >
> > > > > The purpose of these *_SOURCE constants is to enable particular
> > > > > feature sets visibility. As long as we have GNU_SOURCE on Linux
> > > > > removing it won't have any consequences. On BSD it will unify
> > > > > feature sets visibility with the rest of sources. Can't think of any
> downsides here.
> > > > >
> > > > > I believe XOPEN_SOURCE was introduced to extend features not to
> restrict them.
> > > >
> > > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700)
> > > > are defined regardless (_GNU_SOURCE not even needed), while under
> > > > FreeBSD, the non-POSIX versions are only defined when
> __BSD_VISIBLE is set.
> > > >
> > > > The FreeBSD behavior is more correct in this respect since the
> > > > purpose of _XOPEN_SOURCE and friends is also to let applications
> > > > limit the risk of redefinitions in case they were written for an
> > > > earlier standard (e.g. -D_XOPEN_SOURCE=500 vs. -
> D_XOPEN_SOURCE=600).
> > >
> > > Still not sure why do you need it for failsafe and mlx PMDs?
> > > Would something in these PMDs be broken without  '-
> D_XOPEN_SOURCE=600'?
> >
> > Well, not really. At least not anymore if they compile fine without it
> > on all supported targets. I don't mind if they are removed from PMDs.
> >
> > _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by
> > mlx5 and
> > failsafe) for the following reasons:
> >
> > - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
> >   to be exposed. Some affected definitions were likely needed at some
> point.
> >
> > - Besides toggling C syntax extensions, forcing a C standard through the
> >   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
> >   C compliance disables the implicit presence of nonstandard definitions,
> >   which must be re-enabled as needed through the appropriate #defines.
> >
> > For instance, including unistd.h for getsid() stops working as soon as
> > you use -std=c99. On Linux you can get it back through -std=gnu99 or
> > by combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The
> > latter was chosen because it is the standard define supposed to work
> across OSes.
> >
> > Historically mlx4 had to enable -std=c99 to be able to use various
> > features not present when GCC defaulted to -std=gnu90. It was later
> > transformed to
> > -std=c11 for similar reasons (anonymous members in structs/unions if
> > memory serves me right).
> >
> > > > DPDK applications may also define _XOPEN_SOURCE for their own
> > > > needs. They should still be able to use rte_ip.h afterward.
> > >
> > > I suppose they can, they would just have (on FreeBSD) to add '-D
> __BSD_VISIBLE'
> > > themselves.
> >
> > Of course, but public headers should be as self sufficient as possible.
> > Unless they provide really insane compiler flags, if user applications
> > get compilation errors after including some header we install on the
> > system, I think the blame is on DPDK.
> >
> > > > I think this reason is
> > > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > > _XOPEN_SOURCE, as it should work regardless.
> > >
> > > So do you suggest to add '-D __BSD_VISIBLE'  into mlx/failsafe PMDs
> > > Makefiles/meson.build, or ... ?
> >
> > Since headers of our public API potentially require it, it must be
> > defined globally (unlike _XOPEN_SOURCE which is only local to a few
> PMDs):
> > app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk,
> > alongside -D_GNU_SOURCE.
> >
> > Add it to mlx*/failsafe only if that's not enough. Just make sure
> > applications inherit this flag.
>
> Ok, to summarize, eyour suggestion is:
> 1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
> 2. add '-D __BSD_VISIBLE'  into top level make/meson files
> (app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk) Similar
> to what we doing for -DGNU_SOURCE.
>
> If I understand you correctly, then it sounds ok to me.
>
> >
> > > > Looking at the patch [1], I also think there's another, simpler approach:
> > > > unless really performance critical, defining
> > > > rte_ipv6_get_next_ext() in rte_net.c instead of a static inline in
> rte_ip.h should address this issue.
> > >
> > > It is performance critical, and I think that function call for each
> > > ext header is a way too expensive approach.
> > > Will prefer to keep that function inline.
> >
> > OK, a bit cumbersome but since we're heading this way [2], how about
> > defining our own instead of all the above?
> >
> >  #define RTE_IPPROTO_HOPOPTS 0
> >  #define RTE_IPPROTO_ROUTING 43
> >  ...
> >
> > Which could prove handy later as it appears Linux and FreeBSD don't
> > have the same set of available IPPROTO_* definitions.
> >
> > Thoughts?
> >
> > [2] "[RFC v2 00/14] prefix network structures"
> >     https://mails.dpdk.org/archives/dev/2019-April/129752.html
>
> Yep, that's definitely an option too.
> But if we going to replace all current references of IPPROTO_ inside DPDK to
> RTE_IPROTO_ - the change will be massive.
> And for sure it is out of scope of this patch series.
> That's probably need to be done after Olivier RFC will be in and should be
> subject of a separate patch series.
> Konstantin

I agree that we need RTE_IPPROTO* macros but as Konstantin pointed out this
would be a huge change and we should do that on top of Oliver's work in
a separate patch set.

I will propose a patch set with:
1. Removed XOPEN_SOURCE macros as they are not needed anymore
2. Added BSD_VISIBLE at the top of build system.

Thanks for your comments, Marcin.

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Smoczynski, MarcinX]

> > Hey Konstantin,
> >
> > On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > > Hi Adrien,
> > >
> > > >
> > > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX
> wrote:
> > > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > > To summarize we have different visibility sets for Linux and
> > > > > > > BSD when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly.
> To
> > > > > > > overcome this situation we can either remove problematic
> > > > > > > XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > > > > > > drivers/net/mlx4,
> > > > > > > drivers/net/mlx5)
> > > > > >
> > > > > > What is the consequence of removing these macros in mlx and
> failsafe PMDs?
> > > > >
> > > > > The purpose of these *_SOURCE constants is to enable particular
> > > > > feature sets visibility. As long as we have GNU_SOURCE on Linux
> > > > > removing it won't have any consequences. On BSD it will unify
> > > > > feature sets visibility with the rest of sources. Can't think of any
> downsides here.
> > > > >
> > > > > I believe XOPEN_SOURCE was introduced to extend features not to
> restrict them.
> > > >
> > > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700)
> > > > are defined regardless (_GNU_SOURCE not even needed), while under
> > > > FreeBSD, the non-POSIX versions are only defined when
> __BSD_VISIBLE is set.
> > > >
> > > > The FreeBSD behavior is more correct in this respect since the
> > > > purpose of _XOPEN_SOURCE and friends is also to let applications
> > > > limit the risk of redefinitions in case they were written for an
> > > > earlier standard (e.g. -D_XOPEN_SOURCE=500 vs. -
> D_XOPEN_SOURCE=600).
> > >
> > > Still not sure why do you need it for failsafe and mlx PMDs?
> > > Would something in these PMDs be broken without  '-
> D_XOPEN_SOURCE=600'?
> >
> > Well, not really. At least not anymore if they compile fine without it
> > on all supported targets. I don't mind if they are removed from PMDs.
> >
> > _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by
> > mlx5 and
> > failsafe) for the following reasons:
> >
> > - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
> >   to be exposed. Some affected definitions were likely needed at some
> point.
> >
> > - Besides toggling C syntax extensions, forcing a C standard through the
> >   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
> >   C compliance disables the implicit presence of nonstandard definitions,
> >   which must be re-enabled as needed through the appropriate #defines.
> >
> > For instance, including unistd.h for getsid() stops working as soon as
> > you use -std=c99. On Linux you can get it back through -std=gnu99 or
> > by combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The
> > latter was chosen because it is the standard define supposed to work
> across OSes.
> >
> > Historically mlx4 had to enable -std=c99 to be able to use various
> > features not present when GCC defaulted to -std=gnu90. It was later
> > transformed to
> > -std=c11 for similar reasons (anonymous members in structs/unions if
> > memory serves me right).
> >
> > > > DPDK applications may also define _XOPEN_SOURCE for their own
> > > > needs. They should still be able to use rte_ip.h afterward.
> > >
> > > I suppose they can, they would just have (on FreeBSD) to add '-D
> __BSD_VISIBLE'
> > > themselves.
> >
> > Of course, but public headers should be as self sufficient as possible.
> > Unless they provide really insane compiler flags, if user applications
> > get compilation errors after including some header we install on the
> > system, I think the blame is on DPDK.
> >
> > > > I think this reason is
> > > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > > _XOPEN_SOURCE, as it should work regardless.
> > >
> > > So do you suggest to add '-D __BSD_VISIBLE'  into mlx/failsafe PMDs
> > > Makefiles/meson.build, or ... ?
> >
> > Since headers of our public API potentially require it, it must be
> > defined globally (unlike _XOPEN_SOURCE which is only local to a few
> PMDs):
> > app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk,
> > alongside -D_GNU_SOURCE.
> >
> > Add it to mlx*/failsafe only if that's not enough. Just make sure
> > applications inherit this flag.
>
> Ok, to summarize, eyour suggestion is:
> 1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
> 2. add '-D __BSD_VISIBLE'  into top level make/meson files
> (app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk) Similar
> to what we doing for -DGNU_SOURCE.
>
> If I understand you correctly, then it sounds ok to me.
>
> >
> > > > Looking at the patch [1], I also think there's another, simpler approach:
> > > > unless really performance critical, defining
> > > > rte_ipv6_get_next_ext() in rte_net.c instead of a static inline in
> rte_ip.h should address this issue.
> > >
> > > It is performance critical, and I think that function call for each
> > > ext header is a way too expensive approach.
> > > Will prefer to keep that function inline.
> >
> > OK, a bit cumbersome but since we're heading this way [2], how about
> > defining our own instead of all the above?
> >
> >  #define RTE_IPPROTO_HOPOPTS 0
> >  #define RTE_IPPROTO_ROUTING 43
> >  ...
> >
> > Which could prove handy later as it appears Linux and FreeBSD don't
> > have the same set of available IPPROTO_* definitions.
> >
> > Thoughts?
> >
> > [2] "[RFC v2 00/14] prefix network structures"
> >     https://mails.dpdk.org/archives/dev/2019-April/129752.html
>
> Yep, that's definitely an option too.
> But if we going to replace all current references of IPPROTO_ inside DPDK to
> RTE_IPROTO_ - the change will be massive.
> And for sure it is out of scope of this patch series.
> That's probably need to be done after Olivier RFC will be in and should be
> subject of a separate patch series.
> Konstantin

I agree that we need RTE_IPPROTO* macros but as Konstantin pointed out this
would be a huge change and we should do that on top of Oliver's work in
a separate patch set.

I will propose a patch set with:
1. Removed XOPEN_SOURCE macros as they are not needed anymore
2. Added BSD_VISIBLE at the top of build system.

Thanks for your comments, Marcin.

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Adrien Mazarguil]

On Tue, May 14, 2019 at 08:58:42AM +0000, Smoczynski, MarcinX wrote:
> > > Hey Konstantin,
> > >
> > > On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > > > Hi Adrien,
> > > >
> > > > >
> > > > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX
> > wrote:
> > > > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > > > To summarize we have different visibility sets for Linux and
> > > > > > > > BSD when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly.
> > To
> > > > > > > > overcome this situation we can either remove problematic
> > > > > > > > XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > > > > > > > drivers/net/mlx4,
> > > > > > > > drivers/net/mlx5)
> > > > > > >
> > > > > > > What is the consequence of removing these macros in mlx and
> > failsafe PMDs?
> > > > > >
> > > > > > The purpose of these *_SOURCE constants is to enable particular
> > > > > > feature sets visibility. As long as we have GNU_SOURCE on Linux
> > > > > > removing it won't have any consequences. On BSD it will unify
> > > > > > feature sets visibility with the rest of sources. Can't think of any
> > downsides here.
> > > > > >
> > > > > > I believe XOPEN_SOURCE was introduced to extend features not to
> > restrict them.
> > > > >
> > > > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700)
> > > > > are defined regardless (_GNU_SOURCE not even needed), while under
> > > > > FreeBSD, the non-POSIX versions are only defined when
> > __BSD_VISIBLE is set.
> > > > >
> > > > > The FreeBSD behavior is more correct in this respect since the
> > > > > purpose of _XOPEN_SOURCE and friends is also to let applications
> > > > > limit the risk of redefinitions in case they were written for an
> > > > > earlier standard (e.g. -D_XOPEN_SOURCE=500 vs. -
> > D_XOPEN_SOURCE=600).
> > > >
> > > > Still not sure why do you need it for failsafe and mlx PMDs?
> > > > Would something in these PMDs be broken without  '-
> > D_XOPEN_SOURCE=600'?
> > >
> > > Well, not really. At least not anymore if they compile fine without it
> > > on all supported targets. I don't mind if they are removed from PMDs.
> > >
> > > _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by
> > > mlx5 and
> > > failsafe) for the following reasons:
> > >
> > > - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
> > >   to be exposed. Some affected definitions were likely needed at some
> > point.
> > >
> > > - Besides toggling C syntax extensions, forcing a C standard through the
> > >   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
> > >   C compliance disables the implicit presence of nonstandard definitions,
> > >   which must be re-enabled as needed through the appropriate #defines.
> > >
> > > For instance, including unistd.h for getsid() stops working as soon as
> > > you use -std=c99. On Linux you can get it back through -std=gnu99 or
> > > by combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The
> > > latter was chosen because it is the standard define supposed to work
> > across OSes.
> > >
> > > Historically mlx4 had to enable -std=c99 to be able to use various
> > > features not present when GCC defaulted to -std=gnu90. It was later
> > > transformed to
> > > -std=c11 for similar reasons (anonymous members in structs/unions if
> > > memory serves me right).
> > >
> > > > > DPDK applications may also define _XOPEN_SOURCE for their own
> > > > > needs. They should still be able to use rte_ip.h afterward.
> > > >
> > > > I suppose they can, they would just have (on FreeBSD) to add '-D
> > __BSD_VISIBLE'
> > > > themselves.
> > >
> > > Of course, but public headers should be as self sufficient as possible.
> > > Unless they provide really insane compiler flags, if user applications
> > > get compilation errors after including some header we install on the
> > > system, I think the blame is on DPDK.
> > >
> > > > > I think this reason is
> > > > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > > > _XOPEN_SOURCE, as it should work regardless.
> > > >
> > > > So do you suggest to add '-D __BSD_VISIBLE'  into mlx/failsafe PMDs
> > > > Makefiles/meson.build, or ... ?
> > >
> > > Since headers of our public API potentially require it, it must be
> > > defined globally (unlike _XOPEN_SOURCE which is only local to a few
> > PMDs):
> > > app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk,
> > > alongside -D_GNU_SOURCE.
> > >
> > > Add it to mlx*/failsafe only if that's not enough. Just make sure
> > > applications inherit this flag.
> >
> > Ok, to summarize, eyour suggestion is:
> > 1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
> > 2. add '-D __BSD_VISIBLE'  into top level make/meson files
> > (app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk) Similar
> > to what we doing for -DGNU_SOURCE.
> >
> > If I understand you correctly, then it sounds ok to me.
> >
> > >
> > > > > Looking at the patch [1], I also think there's another, simpler approach:
> > > > > unless really performance critical, defining
> > > > > rte_ipv6_get_next_ext() in rte_net.c instead of a static inline in
> > rte_ip.h should address this issue.
> > > >
> > > > It is performance critical, and I think that function call for each
> > > > ext header is a way too expensive approach.
> > > > Will prefer to keep that function inline.
> > >
> > > OK, a bit cumbersome but since we're heading this way [2], how about
> > > defining our own instead of all the above?
> > >
> > >  #define RTE_IPPROTO_HOPOPTS 0
> > >  #define RTE_IPPROTO_ROUTING 43
> > >  ...
> > >
> > > Which could prove handy later as it appears Linux and FreeBSD don't
> > > have the same set of available IPPROTO_* definitions.
> > >
> > > Thoughts?
> > >
> > > [2] "[RFC v2 00/14] prefix network structures"
> > >     https://mails.dpdk.org/archives/dev/2019-April/129752.html
> >
> > Yep, that's definitely an option too.
> > But if we going to replace all current references of IPPROTO_ inside DPDK to
> > RTE_IPROTO_ - the change will be massive.
> > And for sure it is out of scope of this patch series.
> > That's probably need to be done after Olivier RFC will be in and should be
> > subject of a separate patch series.
> > Konstantin
> 
> I agree that we need RTE_IPPROTO* macros but as Konstantin pointed out this
> would be a huge change and we should do that on top of Oliver's work in
> a separate patch set.
> 
> I will propose a patch set with:
> 1. Removed XOPEN_SOURCE macros as they are not needed anymore
> 2. Added BSD_VISIBLE at the top of build system.

Actually I still suggest to leave the existing _XOPEN_SOURCE for users of
-std=whatever, even if covered globally by _GNU_SOURCE and __BSD_VISIBLE.
I think it's useful as a reminder that they did their homework since this is
macro is itself standard.

Regarding RTE_IPPROTO*, my suggestion wasn't to convert DPDK entirely, only
to add the missing ones so far only needed by this patch. Given their values
are defined by RFCs, they should be fully compatible and interchangeable
with system definitions.

I'm fine with either approach in any case.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Adrien Mazarguil]

On Tue, May 14, 2019 at 08:58:42AM +0000, Smoczynski, MarcinX wrote:
> > > Hey Konstantin,
> > >
> > > On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > > > Hi Adrien,
> > > >
> > > > >
> > > > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX
> > wrote:
> > > > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > > > To summarize we have different visibility sets for Linux and
> > > > > > > > BSD when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly.
> > To
> > > > > > > > overcome this situation we can either remove problematic
> > > > > > > > XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > > > > > > > drivers/net/mlx4,
> > > > > > > > drivers/net/mlx5)
> > > > > > >
> > > > > > > What is the consequence of removing these macros in mlx and
> > failsafe PMDs?
> > > > > >
> > > > > > The purpose of these *_SOURCE constants is to enable particular
> > > > > > feature sets visibility. As long as we have GNU_SOURCE on Linux
> > > > > > removing it won't have any consequences. On BSD it will unify
> > > > > > feature sets visibility with the rest of sources. Can't think of any
> > downsides here.
> > > > > >
> > > > > > I believe XOPEN_SOURCE was introduced to extend features not to
> > restrict them.
> > > > >
> > > > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700)
> > > > > are defined regardless (_GNU_SOURCE not even needed), while under
> > > > > FreeBSD, the non-POSIX versions are only defined when
> > __BSD_VISIBLE is set.
> > > > >
> > > > > The FreeBSD behavior is more correct in this respect since the
> > > > > purpose of _XOPEN_SOURCE and friends is also to let applications
> > > > > limit the risk of redefinitions in case they were written for an
> > > > > earlier standard (e.g. -D_XOPEN_SOURCE=500 vs. -
> > D_XOPEN_SOURCE=600).
> > > >
> > > > Still not sure why do you need it for failsafe and mlx PMDs?
> > > > Would something in these PMDs be broken without  '-
> > D_XOPEN_SOURCE=600'?
> > >
> > > Well, not really. At least not anymore if they compile fine without it
> > > on all supported targets. I don't mind if they are removed from PMDs.
> > >
> > > _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by
> > > mlx5 and
> > > failsafe) for the following reasons:
> > >
> > > - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
> > >   to be exposed. Some affected definitions were likely needed at some
> > point.
> > >
> > > - Besides toggling C syntax extensions, forcing a C standard through the
> > >   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
> > >   C compliance disables the implicit presence of nonstandard definitions,
> > >   which must be re-enabled as needed through the appropriate #defines.
> > >
> > > For instance, including unistd.h for getsid() stops working as soon as
> > > you use -std=c99. On Linux you can get it back through -std=gnu99 or
> > > by combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The
> > > latter was chosen because it is the standard define supposed to work
> > across OSes.
> > >
> > > Historically mlx4 had to enable -std=c99 to be able to use various
> > > features not present when GCC defaulted to -std=gnu90. It was later
> > > transformed to
> > > -std=c11 for similar reasons (anonymous members in structs/unions if
> > > memory serves me right).
> > >
> > > > > DPDK applications may also define _XOPEN_SOURCE for their own
> > > > > needs. They should still be able to use rte_ip.h afterward.
> > > >
> > > > I suppose they can, they would just have (on FreeBSD) to add '-D
> > __BSD_VISIBLE'
> > > > themselves.
> > >
> > > Of course, but public headers should be as self sufficient as possible.
> > > Unless they provide really insane compiler flags, if user applications
> > > get compilation errors after including some header we install on the
> > > system, I think the blame is on DPDK.
> > >
> > > > > I think this reason is
> > > > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > > > _XOPEN_SOURCE, as it should work regardless.
> > > >
> > > > So do you suggest to add '-D __BSD_VISIBLE'  into mlx/failsafe PMDs
> > > > Makefiles/meson.build, or ... ?
> > >
> > > Since headers of our public API potentially require it, it must be
> > > defined globally (unlike _XOPEN_SOURCE which is only local to a few
> > PMDs):
> > > app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk,
> > > alongside -D_GNU_SOURCE.
> > >
> > > Add it to mlx*/failsafe only if that's not enough. Just make sure
> > > applications inherit this flag.
> >
> > Ok, to summarize, eyour suggestion is:
> > 1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
> > 2. add '-D __BSD_VISIBLE'  into top level make/meson files
> > (app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk) Similar
> > to what we doing for -DGNU_SOURCE.
> >
> > If I understand you correctly, then it sounds ok to me.
> >
> > >
> > > > > Looking at the patch [1], I also think there's another, simpler approach:
> > > > > unless really performance critical, defining
> > > > > rte_ipv6_get_next_ext() in rte_net.c instead of a static inline in
> > rte_ip.h should address this issue.
> > > >
> > > > It is performance critical, and I think that function call for each
> > > > ext header is a way too expensive approach.
> > > > Will prefer to keep that function inline.
> > >
> > > OK, a bit cumbersome but since we're heading this way [2], how about
> > > defining our own instead of all the above?
> > >
> > >  #define RTE_IPPROTO_HOPOPTS 0
> > >  #define RTE_IPPROTO_ROUTING 43
> > >  ...
> > >
> > > Which could prove handy later as it appears Linux and FreeBSD don't
> > > have the same set of available IPPROTO_* definitions.
> > >
> > > Thoughts?
> > >
> > > [2] "[RFC v2 00/14] prefix network structures"
> > >     https://mails.dpdk.org/archives/dev/2019-April/129752.html
> >
> > Yep, that's definitely an option too.
> > But if we going to replace all current references of IPPROTO_ inside DPDK to
> > RTE_IPROTO_ - the change will be massive.
> > And for sure it is out of scope of this patch series.
> > That's probably need to be done after Olivier RFC will be in and should be
> > subject of a separate patch series.
> > Konstantin
> 
> I agree that we need RTE_IPPROTO* macros but as Konstantin pointed out this
> would be a huge change and we should do that on top of Oliver's work in
> a separate patch set.
> 
> I will propose a patch set with:
> 1. Removed XOPEN_SOURCE macros as they are not needed anymore
> 2. Added BSD_VISIBLE at the top of build system.

Actually I still suggest to leave the existing _XOPEN_SOURCE for users of
-std=whatever, even if covered globally by _GNU_SOURCE and __BSD_VISIBLE.
I think it's useful as a reminder that they did their homework since this is
macro is itself standard.

Regarding RTE_IPPROTO*, my suggestion wasn't to convert DPDK entirely, only
to add the missing ones so far only needed by this patch. Given their values
are defined by RFCs, they should be fully compatible and interchangeable
with system definitions.

I'm fine with either approach in any case.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Ananyev, Konstantin]

Hi Adrien,

> On Tue, May 14, 2019 at 08:58:42AM +0000, Smoczynski, MarcinX wrote:
> > > > Hey Konstantin,
> > > >
> > > > On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > > > > Hi Adrien,
> > > > >
> > > > > >
> > > > > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX
> > > wrote:
> > > > > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > > > > To summarize we have different visibility sets for Linux and
> > > > > > > > > BSD when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly.
> > > To
> > > > > > > > > overcome this situation we can either remove problematic
> > > > > > > > > XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > > > > > > > > drivers/net/mlx4,
> > > > > > > > > drivers/net/mlx5)
> > > > > > > >
> > > > > > > > What is the consequence of removing these macros in mlx and
> > > failsafe PMDs?
> > > > > > >
> > > > > > > The purpose of these *_SOURCE constants is to enable particular
> > > > > > > feature sets visibility. As long as we have GNU_SOURCE on Linux
> > > > > > > removing it won't have any consequences. On BSD it will unify
> > > > > > > feature sets visibility with the rest of sources. Can't think of any
> > > downsides here.
> > > > > > >
> > > > > > > I believe XOPEN_SOURCE was introduced to extend features not to
> > > restrict them.
> > > > > >
> > > > > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700)
> > > > > > are defined regardless (_GNU_SOURCE not even needed), while under
> > > > > > FreeBSD, the non-POSIX versions are only defined when
> > > __BSD_VISIBLE is set.
> > > > > >
> > > > > > The FreeBSD behavior is more correct in this respect since the
> > > > > > purpose of _XOPEN_SOURCE and friends is also to let applications
> > > > > > limit the risk of redefinitions in case they were written for an
> > > > > > earlier standard (e.g. -D_XOPEN_SOURCE=500 vs. -
> > > D_XOPEN_SOURCE=600).
> > > > >
> > > > > Still not sure why do you need it for failsafe and mlx PMDs?
> > > > > Would something in these PMDs be broken without  '-
> > > D_XOPEN_SOURCE=600'?
> > > >
> > > > Well, not really. At least not anymore if they compile fine without it
> > > > on all supported targets. I don't mind if they are removed from PMDs.
> > > >
> > > > _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by
> > > > mlx5 and
> > > > failsafe) for the following reasons:
> > > >
> > > > - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
> > > >   to be exposed. Some affected definitions were likely needed at some
> > > point.
> > > >
> > > > - Besides toggling C syntax extensions, forcing a C standard through the
> > > >   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
> > > >   C compliance disables the implicit presence of nonstandard definitions,
> > > >   which must be re-enabled as needed through the appropriate #defines.
> > > >
> > > > For instance, including unistd.h for getsid() stops working as soon as
> > > > you use -std=c99. On Linux you can get it back through -std=gnu99 or
> > > > by combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The
> > > > latter was chosen because it is the standard define supposed to work
> > > across OSes.
> > > >
> > > > Historically mlx4 had to enable -std=c99 to be able to use various
> > > > features not present when GCC defaulted to -std=gnu90. It was later
> > > > transformed to
> > > > -std=c11 for similar reasons (anonymous members in structs/unions if
> > > > memory serves me right).
> > > >
> > > > > > DPDK applications may also define _XOPEN_SOURCE for their own
> > > > > > needs. They should still be able to use rte_ip.h afterward.
> > > > >
> > > > > I suppose they can, they would just have (on FreeBSD) to add '-D
> > > __BSD_VISIBLE'
> > > > > themselves.
> > > >
> > > > Of course, but public headers should be as self sufficient as possible.
> > > > Unless they provide really insane compiler flags, if user applications
> > > > get compilation errors after including some header we install on the
> > > > system, I think the blame is on DPDK.
> > > >
> > > > > > I think this reason is
> > > > > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > > > > _XOPEN_SOURCE, as it should work regardless.
> > > > >
> > > > > So do you suggest to add '-D __BSD_VISIBLE'  into mlx/failsafe PMDs
> > > > > Makefiles/meson.build, or ... ?
> > > >
> > > > Since headers of our public API potentially require it, it must be
> > > > defined globally (unlike _XOPEN_SOURCE which is only local to a few
> > > PMDs):
> > > > app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk,
> > > > alongside -D_GNU_SOURCE.
> > > >
> > > > Add it to mlx*/failsafe only if that's not enough. Just make sure
> > > > applications inherit this flag.
> > >
> > > Ok, to summarize, eyour suggestion is:
> > > 1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
> > > 2. add '-D __BSD_VISIBLE'  into top level make/meson files
> > > (app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk) Similar
> > > to what we doing for -DGNU_SOURCE.
> > >
> > > If I understand you correctly, then it sounds ok to me.
> > >
> > > >
> > > > > > Looking at the patch [1], I also think there's another, simpler approach:
> > > > > > unless really performance critical, defining
> > > > > > rte_ipv6_get_next_ext() in rte_net.c instead of a static inline in
> > > rte_ip.h should address this issue.
> > > > >
> > > > > It is performance critical, and I think that function call for each
> > > > > ext header is a way too expensive approach.
> > > > > Will prefer to keep that function inline.
> > > >
> > > > OK, a bit cumbersome but since we're heading this way [2], how about
> > > > defining our own instead of all the above?
> > > >
> > > >  #define RTE_IPPROTO_HOPOPTS 0
> > > >  #define RTE_IPPROTO_ROUTING 43
> > > >  ...
> > > >
> > > > Which could prove handy later as it appears Linux and FreeBSD don't
> > > > have the same set of available IPPROTO_* definitions.
> > > >
> > > > Thoughts?
> > > >
> > > > [2] "[RFC v2 00/14] prefix network structures"
> > > >     https://mails.dpdk.org/archives/dev/2019-April/129752.html
> > >
> > > Yep, that's definitely an option too.
> > > But if we going to replace all current references of IPPROTO_ inside DPDK to
> > > RTE_IPROTO_ - the change will be massive.
> > > And for sure it is out of scope of this patch series.
> > > That's probably need to be done after Olivier RFC will be in and should be
> > > subject of a separate patch series.
> > > Konstantin
> >
> > I agree that we need RTE_IPPROTO* macros but as Konstantin pointed out this
> > would be a huge change and we should do that on top of Oliver's work in
> > a separate patch set.
> >
> > I will propose a patch set with:
> > 1. Removed XOPEN_SOURCE macros as they are not needed anymore
> > 2. Added BSD_VISIBLE at the top of build system.
> 
> Actually I still suggest to leave the existing _XOPEN_SOURCE for users of
> -std=whatever, even if covered globally by _GNU_SOURCE and __BSD_VISIBLE.
> I think it's useful as a reminder that they did their homework since this is
> macro is itself standard.
> 

If you insist, I don't mind to keep it  - less changes for us,
again I am not a maintainer, nor a user of these PMDs.
Just don't to see much rationale behind it.
Ss I understand from your previous letters -
with global flags in place, it would build with -std=...
regardless do we have or not XOPEN_SOURCE=... in these PMDs or not.
Konstantin


> Regarding RTE_IPPROTO*, my suggestion wasn't to convert DPDK entirely, only
> to add the missing ones so far only needed by this patch. Given their values
> are defined by RFCs, they should be fully compatible and interchangeable
> with system definitions.
> 
> I'm fine with either approach in any case.
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

[Ananyev, Konstantin]

Hi Adrien,

> On Tue, May 14, 2019 at 08:58:42AM +0000, Smoczynski, MarcinX wrote:
> > > > Hey Konstantin,
> > > >
> > > > On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > > > > Hi Adrien,
> > > > >
> > > > > >
> > > > > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX
> > > wrote:
> > > > > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > > > > To summarize we have different visibility sets for Linux and
> > > > > > > > > BSD when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly.
> > > To
> > > > > > > > > overcome this situation we can either remove problematic
> > > > > > > > > XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > > > > > > > > drivers/net/mlx4,
> > > > > > > > > drivers/net/mlx5)
> > > > > > > >
> > > > > > > > What is the consequence of removing these macros in mlx and
> > > failsafe PMDs?
> > > > > > >
> > > > > > > The purpose of these *_SOURCE constants is to enable particular
> > > > > > > feature sets visibility. As long as we have GNU_SOURCE on Linux
> > > > > > > removing it won't have any consequences. On BSD it will unify
> > > > > > > feature sets visibility with the rest of sources. Can't think of any
> > > downsides here.
> > > > > > >
> > > > > > > I believe XOPEN_SOURCE was introduced to extend features not to
> > > restrict them.
> > > > > >
> > > > > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700)
> > > > > > are defined regardless (_GNU_SOURCE not even needed), while under
> > > > > > FreeBSD, the non-POSIX versions are only defined when
> > > __BSD_VISIBLE is set.
> > > > > >
> > > > > > The FreeBSD behavior is more correct in this respect since the
> > > > > > purpose of _XOPEN_SOURCE and friends is also to let applications
> > > > > > limit the risk of redefinitions in case they were written for an
> > > > > > earlier standard (e.g. -D_XOPEN_SOURCE=500 vs. -
> > > D_XOPEN_SOURCE=600).
> > > > >
> > > > > Still not sure why do you need it for failsafe and mlx PMDs?
> > > > > Would something in these PMDs be broken without  '-
> > > D_XOPEN_SOURCE=600'?
> > > >
> > > > Well, not really. At least not anymore if they compile fine without it
> > > > on all supported targets. I don't mind if they are removed from PMDs.
> > > >
> > > > _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by
> > > > mlx5 and
> > > > failsafe) for the following reasons:
> > > >
> > > > - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
> > > >   to be exposed. Some affected definitions were likely needed at some
> > > point.
> > > >
> > > > - Besides toggling C syntax extensions, forcing a C standard through the
> > > >   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
> > > >   C compliance disables the implicit presence of nonstandard definitions,
> > > >   which must be re-enabled as needed through the appropriate #defines.
> > > >
> > > > For instance, including unistd.h for getsid() stops working as soon as
> > > > you use -std=c99. On Linux you can get it back through -std=gnu99 or
> > > > by combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The
> > > > latter was chosen because it is the standard define supposed to work
> > > across OSes.
> > > >
> > > > Historically mlx4 had to enable -std=c99 to be able to use various
> > > > features not present when GCC defaulted to -std=gnu90. It was later
> > > > transformed to
> > > > -std=c11 for similar reasons (anonymous members in structs/unions if
> > > > memory serves me right).
> > > >
> > > > > > DPDK applications may also define _XOPEN_SOURCE for their own
> > > > > > needs. They should still be able to use rte_ip.h afterward.
> > > > >
> > > > > I suppose they can, they would just have (on FreeBSD) to add '-D
> > > __BSD_VISIBLE'
> > > > > themselves.
> > > >
> > > > Of course, but public headers should be as self sufficient as possible.
> > > > Unless they provide really insane compiler flags, if user applications
> > > > get compilation errors after including some header we install on the
> > > > system, I think the blame is on DPDK.
> > > >
> > > > > > I think this reason is
> > > > > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > > > > _XOPEN_SOURCE, as it should work regardless.
> > > > >
> > > > > So do you suggest to add '-D __BSD_VISIBLE'  into mlx/failsafe PMDs
> > > > > Makefiles/meson.build, or ... ?
> > > >
> > > > Since headers of our public API potentially require it, it must be
> > > > defined globally (unlike _XOPEN_SOURCE which is only local to a few
> > > PMDs):
> > > > app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk,
> > > > alongside -D_GNU_SOURCE.
> > > >
> > > > Add it to mlx*/failsafe only if that's not enough. Just make sure
> > > > applications inherit this flag.
> > >
> > > Ok, to summarize, eyour suggestion is:
> > > 1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
> > > 2. add '-D __BSD_VISIBLE'  into top level make/meson files
> > > (app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk) Similar
> > > to what we doing for -DGNU_SOURCE.
> > >
> > > If I understand you correctly, then it sounds ok to me.
> > >
> > > >
> > > > > > Looking at the patch [1], I also think there's another, simpler approach:
> > > > > > unless really performance critical, defining
> > > > > > rte_ipv6_get_next_ext() in rte_net.c instead of a static inline in
> > > rte_ip.h should address this issue.
> > > > >
> > > > > It is performance critical, and I think that function call for each
> > > > > ext header is a way too expensive approach.
> > > > > Will prefer to keep that function inline.
> > > >
> > > > OK, a bit cumbersome but since we're heading this way [2], how about
> > > > defining our own instead of all the above?
> > > >
> > > >  #define RTE_IPPROTO_HOPOPTS 0
> > > >  #define RTE_IPPROTO_ROUTING 43
> > > >  ...
> > > >
> > > > Which could prove handy later as it appears Linux and FreeBSD don't
> > > > have the same set of available IPPROTO_* definitions.
> > > >
> > > > Thoughts?
> > > >
> > > > [2] "[RFC v2 00/14] prefix network structures"
> > > >     https://mails.dpdk.org/archives/dev/2019-April/129752.html
> > >
> > > Yep, that's definitely an option too.
> > > But if we going to replace all current references of IPPROTO_ inside DPDK to
> > > RTE_IPROTO_ - the change will be massive.
> > > And for sure it is out of scope of this patch series.
> > > That's probably need to be done after Olivier RFC will be in and should be
> > > subject of a separate patch series.
> > > Konstantin
> >
> > I agree that we need RTE_IPPROTO* macros but as Konstantin pointed out this
> > would be a huge change and we should do that on top of Oliver's work in
> > a separate patch set.
> >
> > I will propose a patch set with:
> > 1. Removed XOPEN_SOURCE macros as they are not needed anymore
> > 2. Added BSD_VISIBLE at the top of build system.
> 
> Actually I still suggest to leave the existing _XOPEN_SOURCE for users of
> -std=whatever, even if covered globally by _GNU_SOURCE and __BSD_VISIBLE.
> I think it's useful as a reminder that they did their homework since this is
> macro is itself standard.
> 

If you insist, I don't mind to keep it  - less changes for us,
again I am not a maintainer, nor a user of these PMDs.
Just don't to see much rationale behind it.
Ss I understand from your previous letters -
with global flags in place, it would build with -std=...
regardless do we have or not XOPEN_SOURCE=... in these PMDs or not.
Konstantin


> Regarding RTE_IPPROTO*, my suggestion wasn't to convert DPDK entirely, only
> to add the missing ones so far only needed by this patch. Given their values
> are defined by RFCs, they should be fully compatible and interchangeable
> with system definitions.
> 
> I'm fine with either approach in any case.
> 
> --
> Adrien Mazarguil
> 6WIND