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

[Laurent Vivier <lvivier@redhat.com>]

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"

Thanks,
Laurent