Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Phil Sutter <phil@nwl.cc>]

On Sun, May 15, 2022 at 06:40:51AM -0700, Maciej Żenczykowski wrote:
> On Sun, May 15, 2022 at 5:05 AM Phil Sutter <phil@nwl.cc> wrote:
> >
> > 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@nwl.cc> 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
> 
> Any thoughts about the rest of my email - wrt. #define __USE_BSD
> - do you know how that is supposed to work?

No, but isn't this a detail of bionic header layout?

Cheers, Phil