diff mbox series

[2/2] net: fix header include order for FreeBSD

Message ID 20210506151426.28202-3-david.marchand@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series 21.05 fixes for OVS |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation fail Compilation issues
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS

Commit Message

David Marchand May 6, 2021, 3:14 p.m. UTC 
  Spotted by sparse in OVS build:
../../lib/netdev-dpdk.c: note: in included file (through
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
../../include/sparse/arpa/inet.h:22:2: error: "Must include
<netinet/in.h> before <arpa/inet.h> for FreeBSD support"

This is a check enforced by OVS itself.
See [1] for some context.

1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db

Fixes: 89813a522e68 ("net: provide IP-related API on any OS")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/net/rte_ip.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dmitry Kozlyuk May 6, 2021, 10:23 p.m. UTC | #1 
2021-05-06 17:14 (UTC+0200), David Marchand:
> Spotted by sparse in OVS build:
> ../../lib/netdev-dpdk.c: note: in included file (through
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> 
> This is a check enforced by OVS itself.
> See [1] for some context.
> 
> 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> 
> Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/net/rte_ip.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index 8c189009b0..4b728969c1 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -22,8 +22,8 @@
>  #else
>  #include <sys/socket.h>
>  #include <sys/types.h>
> -#include <arpa/inet.h>
>  #include <netinet/in.h>
> +#include <arpa/inet.h>
>  #include <netinet/ip.h>
>  #endif
>

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

It would be interesting to know exact issue this solves for OVS.
Referenced commit only says FreeBSD "insists" on this include order,
but DPDK and standalone files with these includes build either way.
David Marchand May 7, 2021, 9:06 a.m. UTC | #2 
On Fri, May 7, 2021 at 12:24 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> 2021-05-06 17:14 (UTC+0200), David Marchand:
> > Spotted by sparse in OVS build:
> > ../../lib/netdev-dpdk.c: note: in included file (through
> > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> > ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> > <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> >
> > This is a check enforced by OVS itself.
> > See [1] for some context.
> >
> > 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> >
> > Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  lib/net/rte_ip.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > index 8c189009b0..4b728969c1 100644
> > --- a/lib/net/rte_ip.h
> > +++ b/lib/net/rte_ip.h
> > @@ -22,8 +22,8 @@
> >  #else
> >  #include <sys/socket.h>
> >  #include <sys/types.h>
> > -#include <arpa/inet.h>
> >  #include <netinet/in.h>
> > +#include <arpa/inet.h>
> >  #include <netinet/ip.h>
> >  #endif
> >
>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
>
> It would be interesting to know exact issue this solves for OVS.
> Referenced commit only says FreeBSD "insists" on this include order,
> but DPDK and standalone files with these includes build either way.

Indeed, I tried building with FreeBSD 13.0 and I can see no pb.
This might be something that has been fixed in FreeBSD.

Ben, would you have details on this header inclusion order?
Ben Pfaff May 7, 2021, 7:12 p.m. UTC | #3 
On Fri, May 07, 2021 at 11:06:38AM +0200, David Marchand wrote:
> On Fri, May 7, 2021 at 12:24 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> >
> > 2021-05-06 17:14 (UTC+0200), David Marchand:
> > > Spotted by sparse in OVS build:
> > > ../../lib/netdev-dpdk.c: note: in included file (through
> > > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> > > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> > > ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> > > <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> > >
> > > This is a check enforced by OVS itself.
> > > See [1] for some context.
> > >
> > > 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> > >
> > > Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > >  lib/net/rte_ip.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > > index 8c189009b0..4b728969c1 100644
> > > --- a/lib/net/rte_ip.h
> > > +++ b/lib/net/rte_ip.h
> > > @@ -22,8 +22,8 @@
> > >  #else
> > >  #include <sys/socket.h>
> > >  #include <sys/types.h>
> > > -#include <arpa/inet.h>
> > >  #include <netinet/in.h>
> > > +#include <arpa/inet.h>
> > >  #include <netinet/ip.h>
> > >  #endif
> > >
> >
> > Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> >
> > It would be interesting to know exact issue this solves for OVS.
> > Referenced commit only says FreeBSD "insists" on this include order,
> > but DPDK and standalone files with these includes build either way.
> 
> Indeed, I tried building with FreeBSD 13.0 and I can see no pb.
> This might be something that has been fixed in FreeBSD.
> 
> Ben, would you have details on this header inclusion order?

Using the wrong order caused a compiler error on whatever version of
FreeBSD was current at the time of those commits.  I think it was a
historical BSD issue, since I remember running into this for many years
across multiple BSD versions.

If it's fixed now, we can drop the constraint.
Ben Pfaff May 7, 2021, 7:13 p.m. UTC | #4 
On Fri, May 07, 2021 at 12:12:09PM -0700, Ben Pfaff wrote:
> On Fri, May 07, 2021 at 11:06:38AM +0200, David Marchand wrote:
> > On Fri, May 7, 2021 at 12:24 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> > >
> > > 2021-05-06 17:14 (UTC+0200), David Marchand:
> > > > Spotted by sparse in OVS build:
> > > > ../../lib/netdev-dpdk.c: note: in included file (through
> > > > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> > > > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> > > > ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> > > > <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> > > >
> > > > This is a check enforced by OVS itself.
> > > > See [1] for some context.
> > > >
> > > > 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> > > >
> > > > Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > > >  lib/net/rte_ip.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > > > index 8c189009b0..4b728969c1 100644
> > > > --- a/lib/net/rte_ip.h
> > > > +++ b/lib/net/rte_ip.h
> > > > @@ -22,8 +22,8 @@
> > > >  #else
> > > >  #include <sys/socket.h>
> > > >  #include <sys/types.h>
> > > > -#include <arpa/inet.h>
> > > >  #include <netinet/in.h>
> > > > +#include <arpa/inet.h>
> > > >  #include <netinet/ip.h>
> > > >  #endif
> > > >
> > >
> > > Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > >
> > > It would be interesting to know exact issue this solves for OVS.
> > > Referenced commit only says FreeBSD "insists" on this include order,
> > > but DPDK and standalone files with these includes build either way.
> > 
> > Indeed, I tried building with FreeBSD 13.0 and I can see no pb.
> > This might be something that has been fixed in FreeBSD.
> > 
> > Ben, would you have details on this header inclusion order?
> 
> Using the wrong order caused a compiler error on whatever version of
> FreeBSD was current at the time of those commits.  I think it was a
> historical BSD issue, since I remember running into this for many years
> across multiple BSD versions.
> 
> If it's fixed now, we can drop the constraint.

Oh, Mac OS X appears to also have this or a related problem:

commit ff6aa424ef1f46d2d1c468940219e187632ec894
Author: Borja Marcos EA2EKH <borjam@gmail.com>
Date:   Mon Nov 6 10:32:12 2017 +0100

    conntrack: Include <sys/types.h> before <netinet/icmp6.h>.
    
    FreeBSD and Mac OS X require this.
    
    Signed-off-by: Ben Pfaff <blp@ovn.org>
David Marchand May 10, 2021, 11:25 a.m. UTC | #5 
Hello Ben,

On Fri, May 7, 2021 at 9:13 PM Ben Pfaff <blp@ovn.org> wrote:
> > > > It would be interesting to know exact issue this solves for OVS.
> > > > Referenced commit only says FreeBSD "insists" on this include order,
> > > > but DPDK and standalone files with these includes build either way.
> > >
> > > Indeed, I tried building with FreeBSD 13.0 and I can see no pb.
> > > This might be something that has been fixed in FreeBSD.
> > >
> > > Ben, would you have details on this header inclusion order?
> >
> > Using the wrong order caused a compiler error on whatever version of
> > FreeBSD was current at the time of those commits.  I think it was a
> > historical BSD issue, since I remember running into this for many years
> > across multiple BSD versions.
> >
> > If it's fixed now, we can drop the constraint.

I reverted some bits on arpa/inet.h vs netinet/in.h headers in OVS but
did not reproduce the issue on FreeBSD 11 and 12 with Cirrus.


>
> Oh, Mac OS X appears to also have this or a related problem:

Reverting the patch on conntrack which deals with sys/types.h vs
netinet/icmp6.h, there is an issue on FreeBSD 11 and 12:
https://cirrus-ci.com/task/4786810854309888

In file included from lib/conntrack.c:libtool: compile: clang
-DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib
-I/usr/local/include -Wstrict-prototypes -Wall -Wextra
-Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
-Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
-Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
-Wmissing-field-initializers -Wthread-safety -fno-strict-aliasing
-Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument
-Wshift-negative-value -Qunused-arguments -Wshadow
-Wno-null-pointer-arithmetic -Warray-bounds-pointer-arithmetic -Werror
-Werror -g -O2 -Wall -DHAVE_AVX512F -DHAVE_LD_AVX512_GOOD -MT
lib/coverage.lo -MD -MP -MF lib/.deps/coverage.Tpo -c lib/coverage.c
-o lib/coverage.o
21:
/usr/include/netinet/icmp6.h:71:2: error: unknown type name 'u_int8_t'
u_int8_t icmp6_type; /* type field */
^
/usr/include/netinet/icmp6.h:72:2: error: unknown type name 'u_int8_t'
u_int8_t icmp6_code; /* code field */
^
...


I don't think it is worth dropping only the first constraint, so I
would keep it as is in OVS.
Thanks.
Olivier Matz May 11, 2021, 1:09 p.m. UTC | #6 
On Thu, May 06, 2021 at 05:14:26PM +0200, David Marchand wrote:
> Spotted by sparse in OVS build:
> ../../lib/netdev-dpdk.c: note: in included file (through
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> 
> This is a check enforced by OVS itself.
> See [1] for some context.
> 
> 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> 
> Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>
diff mbox series

Patch

diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 8c189009b0..4b728969c1 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -22,8 +22,8 @@ 
 #else
 #include <sys/socket.h>
 #include <sys/types.h>
-#include <arpa/inet.h>
 #include <netinet/in.h>
+#include <arpa/inet.h>
 #include <netinet/ip.h>
 #endif