Hi Maciej,
On Sat, May 14, 2022 at 12:14:27PM -0700, Maciej Żenczykowski wrote:
> On Sat, May 14, 2022 at 10:04 AM Phil Sutter <phil@xxxxxx> wrote:
> > On Sat, May 14, 2022 at 06:33:24PM +0200, Nick Hainke wrote:
> > > Only include <linux/if_ether.h> if glibc is used.
> >
> > This looks like a bug in musl? OTOH explicit include of linux/if_ether.h
> > was added in commit c5d9a723b5159 ("fix build for missing ETH_ALEN
> > definition"), despite netinet/ether.h being included in line 2248 of
> > libxtables/xtables.c. So maybe *also* a bug in bionic?!
>
> You stripped the email you're replying to, and while I'm on lkml and
> netdev - with my personal account - I'm not (apparently) subscribed to
> netfilter-devel (or I'm not subscribed from work account).
Oh, sorry for the caused inconvenience.
> Either way, if my search-fu is correct you're replying to
> https://marc.info/?l=netfilter-devel&m=165254651011397&w=2
>
> +#if defined(__GLIBC__)
> #include <linux/if_ether.h> /* ETH_ALEN */
> +#endif
>
> and you're presumably CC'ing me due to
>
> https://git.netfilter.org/iptables/commit/libxtables/xtables.c?id=c5d9a723b5159a28f547b577711787295a14fd84
>
> which added the include in the first place...:
That's correct. I assumed that you added the include for a reason and
it's breaking Nick's use-case, the two of you want to have a word with
each other. :)
> fix build for missing ETH_ALEN definition
> (this is needed at least with bionic)
>
> +#include <linux/if_ether.h> /* ETH_ALEN */
>
> Based on the above, clearly adding an 'if defined GLIBC' wrapper will
> break bionic...
> and presumably glibc doesn't care whether the #include is done one way
> or the other?
With glibc, netinet/ether.h includes netinet/if_ether.h which in turn
includes linux/if_ether.h where finally ETH_ALEN is defined.
In xtables.c we definitely need netinet/ether.h for ether_aton()
declaration.
> Obviously it could be '#if !defined MUSL' instead...
Could ...
> As for the fix? And whether glibc or musl or bionic are wrong or not...
> Utterly uncertain...
>
> Though, I will point out #include's 2000 lines into a .c file are kind of funky.
ACK!
> Ultimately I find
> https://android.git.corp.google.com/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b%5E%21/
>
> +#ifdef __BIONIC__
> +#include <linux/if_ether.h> /* ETH_ALEN */
> +#endif
While I think musl not catching the "double" include is a bug, I'd
prefer the ifdef __BIONIC__ solution since it started the "but my libc
needs this" game.
Nick, if the above change fixes musl builds for you, would you mind
submitting it formally along with a move of the netinet/ether.h include
from mid-file to top?
Thanks, Phil
