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

[Stefano Brivio <sbrivio@redhat.com>]

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?

> > Or should we just unconditionally #define __UAPI_DEF_ETHHDR=0 just
> > before including <linux/virtio_net.h>?  
> 
> If you do that, I think it will not work anymore with glibc

Right, discarded that option.

-- 
Stefano