On Tue, May 17, 2022 at 1:17 AM Phil Sutter <phil@xxxxxx> wrote:
>
> 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@xxxxxx> 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@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
> >
> > 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
No idea... is there actually a standard's document somewhere that
actually describes which header file should declare what (and under
what #define conditions)?
Without it, I have absolutely no idea...
glibc also has tons of headers that require #define SOMETHING before
including them in order to get them to declare certain things.
I have no idea why the #ifdef BSD guard is there in bionic... maybe
it's wrong, maybe it's compatible with some other libc header files...
It doesn't really feel like the sort of thing that would be added by
mistake (ie. it feels very intentional)...
But changing bionic (for example to remove the #ifdef), feels scarier,
because it affects all things using bionic (including android app
native code I think), as opposed to just a single file in iptables...
