Re: [PATCH] musl: fix conflict on ethhdr definition

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 27 Nov 2024 16:37:05 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 27/11/2024 16:05, Stefano Brivio wrote:
> > On Wed, 27 Nov 2024 15:55:00 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> On 27/11/2024 15:48, Stefano Brivio wrote:  
> >>> On Wed, 27 Nov 2024 15:41:30 +0100
> >>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>      
> >>>> there is a conflict between netinet/if_ether.h provided by musl and
> >>>> linux/if_ether.h provided by the linux headers:
> >>>>
> >>>>     In file included from passt.h:185,
> >>>>                      from tcp_vu.c:21:
> >>>>     /usr/include/netinet/if_ether.h:115:8: error: redefinition of 'struct ethhdr'
> >>>>       115 | struct ethhdr {
> >>>>           |        ^~~~~~
> >>>>     In file included from /usr/include/linux/virtio_net.h:32,
> >>>>                      from tcp_vu.c:17:
> >>>>     /usr/include/linux/if_ether.h:173:8: note: originally defined here
> >>>>       173 | struct ethhdr {
> >>>>           |        ^~~~~~
> >>>>
> >>>> The kernel headers provide a flag to disable the definition in this case,
> >>>> __UAPI_DEF_ETHHDR (see /usr/include/linux/if_ether.h comment).
> >>>>
> >>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>>> ---
> >>>>    Makefile | 3 +++
> >>>>    1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/Makefile b/Makefile
> >>>> index cb7448079de5..2aa56ada65fd 100644
> >>>> --- a/Makefile
> >>>> +++ b/Makefile
> >>>> @@ -33,6 +33,9 @@ FLAGS +=  $(FORTIFY_FLAG) -O2 -pie -fPIE
> >>>>    FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE)
> >>>>    FLAGS += -DVERSION=\"$(VERSION)\"
> >>>>    FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
> >>>> +ifeq (musl, $(word 4,$(subst -, ,$(TARGET))))
> >>>> +FLAGS += -D__UAPI_DEF_ETHHDR=0
> >>>> +endif  
> >>>
> >>> Uh oh, I just solved this locally (I was about to push, but I stopped
> >>> in time) by simply including <linux/if_ether.h> instead of
> >>> <netinet/if_ether.h> from passt.h.
> >>>
> >>> I'm not sure what's the best solution: mine is simpler, but I'm not
> >>> really fond of including Linux-specific stuff if there's no need for
> >>> it, so I'd slightly prefer yours.
> >>>
> >>> On the other hand, it's not just for musl. If somebody tries to build
> >>> this against, say, uClibc-ng (no reports about that yet), they will
> >>> probably run into trouble as well.  
> >>
> >> In fact, in /usr/include/linux/if_ether.h they say:
> >>
> >> /* allow libcs like musl to deactivate this, glibc does not implement this. */
> >>
> >> So I think musl should set the value and then I found:
> >>
> >> #define __UAPI_DEF_ETHHDR       0
> >>
> >> in /usr/include/netinet/if_ether.h  
> > 
> > Ah, I hadn't checked that yet.
> >   
> >> So I think the solution is to include netinet/if_ether.h before linux/if_ether.h  
> > 
> > Just in tcp_vu.c you mean? That's where we include <linux/if_ether.h>
> > now.
> > 
> > Any advantage over simply including linux/if_ether.h in passt.h as I
> > did?  
> 
> passt.h remains generic by not including linux specific headers.
> 
> Something like that:
> 
> diff --git a/tcp_vu.c b/tcp_vu.c
> index 05e2d1d..943b6d9 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -13,7 +13,7 @@
>   #include <netinet/tcp.h>
> 
>   #include <sys/socket.h>
> -
> +#include <netinet/if_ether.h>
>   #include <linux/virtio_net.h>
> 
>   #include "util.h"
> diff --git a/vu_common.c b/vu_common.c
> index 0763ec0..299b5a3 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -8,6 +8,7 @@
>   #include <unistd.h>
>   #include <sys/uio.h>
>   #include <sys/eventfd.h>
> +#include <netinet/if_ether.h>
>   #include <linux/virtio_net.h>
> 
>   #include "util.h"
> 

Okay, that works as well, just tested on glibc and musl. I'll simply
fold it in 7/9 so that we don't break bisection with musl.

-- 
Stefano