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

[Phil Sutter <phil@nwl.cc>]

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