diff mbox

the patch "bridge: export multicast database via netlink" broke kernel 3.8 uapi (was: Re: [libvirt] if_bridge.h: include in6.h for struct in6_addr use)

Message ID 1358244688.4264.7.camel@cr0
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Amerigo Wang Jan. 15, 2013, 10:11 a.m. UTC
On Tue, 2013-01-15 at 12:03 +0200, Thomas Backlund wrote:
> Eric Blake skrev 15.1.2013 01:57:
> > On 01/13/2013 01:05 PM, Thomas Backlund wrote:
> >> Thomas Backlund skrev 13.1.2013 20:38:
> >>> patch both inline and attached as thunderbird tends to mess up ...
> >>
> >>> -----
> >>>
> >>> if_bridge.h uses struct in6_addr ip6; but does not include the in6.h
> >>> header.
> >>>
> >>> Found by trying to build libvirt and connman against 3.8-rc3 headers.
> >>>
> >>
> >> Ok,
> >> ignore this patch, it's not the correct fix as it introduces
> >> redefinitions...
> >>
> >> Btw, the error that I hit that made me suggest this fix was libvirt
> >> config check bailing out:
> >>
> >> config.log:/usr/include/linux/if_bridge.h:173:20: error: field 'ip6' has
> >> incomplete type
> >
> > Hmm, just now noticing this thread, after independently hitting and and
> > having to work around the same problem in libvirt:
> > https://www.redhat.com/archives/libvir-list/2013-January/msg00930.html
> > https://bugzilla.redhat.com/show_bug.cgi?id=895141
> 
> 
> Yep,
> 
> and the commit breaking uapi headers is by using "struct in6_addr ip6" is:
> 
> 
>  From ee07c6e7a6f8a25c18f0a6b18152fbd7499245f6 Mon Sep 17 00:00:00 2001
> From: Cong Wang <amwang@redhat.com>
> Date: Fri, 7 Dec 2012 00:04:48 +0000
> Subject: [PATCH] bridge: export multicast database via netlink

Does the following patch help?

$ git diff include/uapi/linux/if_bridge.h

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Thomas Backlund Jan. 15, 2013, 10:55 a.m. UTC | #1
Cong Wang skrev 15.1.2013 12:11:
> On Tue, 2013-01-15 at 12:03 +0200, Thomas Backlund wrote:
>> Eric Blake skrev 15.1.2013 01:57:
>>> On 01/13/2013 01:05 PM, Thomas Backlund wrote:
>>>> Thomas Backlund skrev 13.1.2013 20:38:
>>>>> patch both inline and attached as thunderbird tends to mess up ...
>>>>
>>>>> -----
>>>>>
>>>>> if_bridge.h uses struct in6_addr ip6; but does not include the in6.h
>>>>> header.
>>>>>
>>>>> Found by trying to build libvirt and connman against 3.8-rc3 headers.
>>>>>
>>>>
>>>> Ok,
>>>> ignore this patch, it's not the correct fix as it introduces
>>>> redefinitions...
>>>>
>>>> Btw, the error that I hit that made me suggest this fix was libvirt
>>>> config check bailing out:
>>>>
>>>> config.log:/usr/include/linux/if_bridge.h:173:20: error: field 'ip6' has
>>>> incomplete type
>>>
>>> Hmm, just now noticing this thread, after independently hitting and and
>>> having to work around the same problem in libvirt:
>>> https://www.redhat.com/archives/libvir-list/2013-January/msg00930.html
>>> https://bugzilla.redhat.com/show_bug.cgi?id=895141
>>
>>
>> Yep,
>>
>> and the commit breaking uapi headers is by using "struct in6_addr ip6" is:
>>
>>
>>   From ee07c6e7a6f8a25c18f0a6b18152fbd7499245f6 Mon Sep 17 00:00:00 2001
>> From: Cong Wang <amwang@redhat.com>
>> Date: Fri, 7 Dec 2012 00:04:48 +0000
>> Subject: [PATCH] bridge: export multicast database via netlink
>
> Does the following patch help?
>
> $ git diff include/uapi/linux/if_bridge.h
> diff --git a/include/uapi/linux/if_bridge.h
> b/include/uapi/linux/if_bridge.h
> index 5db2975..653db23 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -14,6 +14,7 @@
>   #define _UAPI_LINUX_IF_BRIDGE_H
>
>   #include <linux/types.h>
> +#include <linux/in6.h>
>
>   #define SYSFS_BRIDGE_ATTR      "bridge"
>   #define SYSFS_BRIDGE_FDB       "brforward"
>

Well, I suggested the same fix in the beginning of the thread
on netdev and lkml: "if_bridge.h: include in6.h for struct in6_addr use"

as it seemed to fix the libvirt case

but then asked it to be ignored after I tried to build connman,
and hit this conflict with glibc-2.17:

In file included from /usr/include/arpa/inet.h:22:0,
                  from ./include/connman/inet.h:25,
                  from src/connman.h:128,
                  from src/tethering.c:40:
/usr/include/netinet/in.h:35:5: error: expected identifier before 
numeric constant
/usr/include/netinet/in.h:197:8: error: redefinition of 'struct in6_addr'
In file included from /usr/include/linux/if_bridge.h:17:0,
                  from src/tethering.c:38:
/usr/include/linux/in6.h:30:8: note: originally defined here
In file included from /usr/include/arpa/inet.h:22:0,
                  from ./include/connman/inet.h:25,
                  from src/connman.h:128,
                  from src/tethering.c:40:
/usr/include/netinet/in.h:238:8: error: redefinition of 'struct 
sockaddr_in6'
In file included from /usr/include/linux/if_bridge.h:17:0,
                  from src/tethering.c:38:
/usr/include/linux/in6.h:46:8: note: originally defined here
In file included from /usr/include/arpa/inet.h:22:0,
                  from ./include/connman/inet.h:25,
                  from src/connman.h:128,
                  from src/tethering.c:40:
/usr/include/netinet/in.h:274:8: error: redefinition of 'struct ipv6_mreq'
In file included from /usr/include/linux/if_bridge.h:17:0,
                  from src/tethering.c:38:
/usr/include/linux/in6.h:54:8: note: originally defined here
make[1]: *** [src/src_connmand-tethering.o] Error 1


So I'm not sure it's the right one...


--
Thomas

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang Jan. 16, 2013, 5:51 a.m. UTC | #2
On Tue, 2013-01-15 at 12:55 +0200, Thomas Backlund wrote:
> 
> as it seemed to fix the libvirt case
> 
> but then asked it to be ignored after I tried to build connman,
> and hit this conflict with glibc-2.17:
> 
> In file included from /usr/include/arpa/inet.h:22:0,
>                   from ./include/connman/inet.h:25,
>                   from src/connman.h:128,
>                   from src/tethering.c:40:
> /usr/include/netinet/in.h:35:5: error: expected identifier before 
> numeric constant
> /usr/include/netinet/in.h:197:8: error: redefinition of 'struct in6_addr'
> In file included from /usr/include/linux/if_bridge.h:17:0,
>                   from src/tethering.c:38:
> /usr/include/linux/in6.h:30:8: note: originally defined here
> In file included from /usr/include/arpa/inet.h:22:0,
>                   from ./include/connman/inet.h:25,
>                   from src/connman.h:128,
>                   from src/tethering.c:40:
> /usr/include/netinet/in.h:238:8: error: redefinition of 'struct 
> sockaddr_in6'
> In file included from /usr/include/linux/if_bridge.h:17:0,
>                   from src/tethering.c:38:
> /usr/include/linux/in6.h:46:8: note: originally defined here
> In file included from /usr/include/arpa/inet.h:22:0,
>                   from ./include/connman/inet.h:25,
>                   from src/connman.h:128,
>                   from src/tethering.c:40:
> /usr/include/netinet/in.h:274:8: error: redefinition of 'struct ipv6_mreq'
> In file included from /usr/include/linux/if_bridge.h:17:0,
>                   from src/tethering.c:38:
> /usr/include/linux/in6.h:54:8: note: originally defined here
> make[1]: *** [src/src_connmand-tethering.o] Error 1
> 
> 
> So I'm not sure it's the right one...

% rpm -q --whatprovides /usr/include/netinet/in.h 
glibc-headers-2.14.90-24.fc16.9.x86_64

Seems we have conflicts with glibc headers...


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang Jan. 16, 2013, 6:06 a.m. UTC | #3
(Cc'ing some glibc developers...)

Hello,

In glibc source file inet/netinet/in.h and kernel source file
include/uapi/linux/in6.h, both define struct in6_addr, and both are
visible to user applications. Thomas reported a conflict below.

So, how can we handle this? /me is wondering why we didn't see this
before.

Thanks.

On Tue, 2013-01-15 at 12:55 +0200, Thomas Backlund wrote:
> Cong Wang skrev 15.1.2013 12:11:
> >
> > Does the following patch help?
> >
> > $ git diff include/uapi/linux/if_bridge.h
> > diff --git a/include/uapi/linux/if_bridge.h
> > b/include/uapi/linux/if_bridge.h
> > index 5db2975..653db23 100644
> > --- a/include/uapi/linux/if_bridge.h
> > +++ b/include/uapi/linux/if_bridge.h
> > @@ -14,6 +14,7 @@
> >   #define _UAPI_LINUX_IF_BRIDGE_H
> >
> >   #include <linux/types.h>
> > +#include <linux/in6.h>
> >
> >   #define SYSFS_BRIDGE_ATTR      "bridge"
> >   #define SYSFS_BRIDGE_FDB       "brforward"
> >
> 
> Well, I suggested the same fix in the beginning of the thread
> on netdev and lkml: "if_bridge.h: include in6.h for struct in6_addr use"
> 
> as it seemed to fix the libvirt case
> 
> but then asked it to be ignored after I tried to build connman,
> and hit this conflict with glibc-2.17:
> 
> In file included from /usr/include/arpa/inet.h:22:0,
>                   from ./include/connman/inet.h:25,
>                   from src/connman.h:128,
>                   from src/tethering.c:40:
> /usr/include/netinet/in.h:35:5: error: expected identifier before 
> numeric constant
> /usr/include/netinet/in.h:197:8: error: redefinition of 'struct in6_addr'
> In file included from /usr/include/linux/if_bridge.h:17:0,
>                   from src/tethering.c:38:
> /usr/include/linux/in6.h:30:8: note: originally defined here
> In file included from /usr/include/arpa/inet.h:22:0,
>                   from ./include/connman/inet.h:25,
>                   from src/connman.h:128,
>                   from src/tethering.c:40:
> /usr/include/netinet/in.h:238:8: error: redefinition of 'struct 
> sockaddr_in6'
> In file included from /usr/include/linux/if_bridge.h:17:0,
>                   from src/tethering.c:38:
> /usr/include/linux/in6.h:46:8: note: originally defined here
> In file included from /usr/include/arpa/inet.h:22:0,
>                   from ./include/connman/inet.h:25,
>                   from src/connman.h:128,
>                   from src/tethering.c:40:
> /usr/include/netinet/in.h:274:8: error: redefinition of 'struct ipv6_mreq'
> In file included from /usr/include/linux/if_bridge.h:17:0,
>                   from src/tethering.c:38:
> /usr/include/linux/in6.h:54:8: note: originally defined here
> make[1]: *** [src/src_connmand-tethering.o] Error 1
> 
> 
> So I'm not sure it's the right one...
> 



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
YOSHIFUJI Hideaki / 吉藤英明 Jan. 16, 2013, 2:21 p.m. UTC | #4
Cong Wang wrote:
> (Cc'ing some glibc developers...)
> 
> Hello,
> 
> In glibc source file inet/netinet/in.h and kernel source file
> include/uapi/linux/in6.h, both define struct in6_addr, and both are
> visible to user applications. Thomas reported a conflict below.
> 
> So, how can we handle this? /me is wondering why we didn't see this
> before.
> 
> Thanks.
> 
> On Tue, 2013-01-15 at 12:55 +0200, Thomas Backlund wrote:
>> Cong Wang skrev 15.1.2013 12:11:
>>>
>>> Does the following patch help?
>>>
>>> $ git diff include/uapi/linux/if_bridge.h
>>> diff --git a/include/uapi/linux/if_bridge.h
>>> b/include/uapi/linux/if_bridge.h
>>> index 5db2975..653db23 100644
>>> --- a/include/uapi/linux/if_bridge.h
>>> +++ b/include/uapi/linux/if_bridge.h
>>> @@ -14,6 +14,7 @@
>>>   #define _UAPI_LINUX_IF_BRIDGE_H
>>>
>>>   #include <linux/types.h>
>>> +#include <linux/in6.h>
>>>
>>>   #define SYSFS_BRIDGE_ATTR      "bridge"
>>>   #define SYSFS_BRIDGE_FDB       "brforward"
>>>
>>
>> Well, I suggested the same fix in the beginning of the thread
>> on netdev and lkml: "if_bridge.h: include in6.h for struct in6_addr use"
>>
>> as it seemed to fix the libvirt case
>>
>> but then asked it to be ignored after I tried to build connman,
>> and hit this conflict with glibc-2.17:
>>
>> In file included from /usr/include/arpa/inet.h:22:0,
>>                   from ./include/connman/inet.h:25,
>>                   from src/connman.h:128,
>>                   from src/tethering.c:40:
>> /usr/include/netinet/in.h:35:5: error: expected identifier before 
>> numeric constant
>> /usr/include/netinet/in.h:197:8: error: redefinition of 'struct in6_addr'
>> In file included from /usr/include/linux/if_bridge.h:17:0,
>>                   from src/tethering.c:38:
>> /usr/include/linux/in6.h:30:8: note: originally defined here
>> In file included from /usr/include/arpa/inet.h:22:0,
>>                   from ./include/connman/inet.h:25,
>>                   from src/connman.h:128,
>>                   from src/tethering.c:40:
>> /usr/include/netinet/in.h:238:8: error: redefinition of 'struct 
>> sockaddr_in6'
>> In file included from /usr/include/linux/if_bridge.h:17:0,
>>                   from src/tethering.c:38:
>> /usr/include/linux/in6.h:46:8: note: originally defined here
>> In file included from /usr/include/arpa/inet.h:22:0,
>>                   from ./include/connman/inet.h:25,
>>                   from src/connman.h:128,
>>                   from src/tethering.c:40:
>> /usr/include/netinet/in.h:274:8: error: redefinition of 'struct ipv6_mreq'
>> In file included from /usr/include/linux/if_bridge.h:17:0,
>>                   from src/tethering.c:38:
>> /usr/include/linux/in6.h:54:8: note: originally defined here
>> make[1]: *** [src/src_connmand-tethering.o] Error 1
>>
>>
>> So I'm not sure it's the right one...

This is not a new issue.  In addition to this,
netinet/in.h also conflits with linux/in.h.

We might have
 #if !defined(__GLIBC__) || !defined(_NETINET_IN_H)
 :
 #endif
around those conflicting definitions in uapi/linux/in{,6}.h.

--yoshfuji
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Jan. 16, 2013, 3:47 p.m. UTC | #5
On Wed, 2013-01-16 at 23:21 +0900, YOSHIFUJI Hideaki wrote:
> Cong Wang wrote:
> > (Cc'ing some glibc developers...)
> > 
> > Hello,
> > 
> > In glibc source file inet/netinet/in.h and kernel source file
> > include/uapi/linux/in6.h, both define struct in6_addr, and both are
> > visible to user applications. Thomas reported a conflict below.
> > 
> > So, how can we handle this? /me is wondering why we didn't see this
> > before.
[...]
> This is not a new issue.  In addition to this,
> netinet/in.h also conflits with linux/in.h.
> 
> We might have
>  #if !defined(__GLIBC__) || !defined(_NETINET_IN_H)
>  :
>  #endif
> around those conflicting definitions in uapi/linux/in{,6}.h.

This only solves half the problem, as <netinet/in.h> might be included
after <linux/in.h>.  Also, not all Linux userland uses glibc.

Ben.
Mike Frysinger Jan. 16, 2013, 5:04 p.m. UTC | #6
On Wednesday 16 January 2013 10:47:12 Ben Hutchings wrote:
> On Wed, 2013-01-16 at 23:21 +0900, YOSHIFUJI Hideaki wrote:
> > Cong Wang wrote:
> > > (Cc'ing some glibc developers...)
> > > 
> > > Hello,
> > > 
> > > In glibc source file inet/netinet/in.h and kernel source file
> > > include/uapi/linux/in6.h, both define struct in6_addr, and both are
> > > visible to user applications. Thomas reported a conflict below.
> > > 
> > > So, how can we handle this? /me is wondering why we didn't see this
> > > before.
> 
> [...]
> 
> > This is not a new issue.  In addition to this,
> > netinet/in.h also conflits with linux/in.h.
> > 
> > We might have
> > 
> >  #if !defined(__GLIBC__) || !defined(_NETINET_IN_H)
> >  
> >  #endif
> > 
> > around those conflicting definitions in uapi/linux/in{,6}.h.
> 
> This only solves half the problem, as <netinet/in.h> might be included
> after <linux/in.h>.  Also, not all Linux userland uses glibc.

certainly true, but the current expectation is that you don't mix your ABIs.  
if you're programming with the C library API, then use the C library headers.  
if you're banging directly on the kernel, then use the kernel headers.  not 
saying it's a perfect solution, but it works for the vast majority of use 
cases.
-mike
Ben Hutchings Jan. 16, 2013, 5:10 p.m. UTC | #7
On Wed, 2013-01-16 at 12:04 -0500, Mike Frysinger wrote:
> On Wednesday 16 January 2013 10:47:12 Ben Hutchings wrote:
> > On Wed, 2013-01-16 at 23:21 +0900, YOSHIFUJI Hideaki wrote:
> > > Cong Wang wrote:
> > > > (Cc'ing some glibc developers...)
> > > > 
> > > > Hello,
> > > > 
> > > > In glibc source file inet/netinet/in.h and kernel source file
> > > > include/uapi/linux/in6.h, both define struct in6_addr, and both are
> > > > visible to user applications. Thomas reported a conflict below.
> > > > 
> > > > So, how can we handle this? /me is wondering why we didn't see this
> > > > before.
> > 
> > [...]
> > 
> > > This is not a new issue.  In addition to this,
> > > netinet/in.h also conflits with linux/in.h.
> > > 
> > > We might have
> > > 
> > >  #if !defined(__GLIBC__) || !defined(_NETINET_IN_H)
> > >  
> > >  #endif
> > > 
> > > around those conflicting definitions in uapi/linux/in{,6}.h.
> > 
> > This only solves half the problem, as <netinet/in.h> might be included
> > after <linux/in.h>.  Also, not all Linux userland uses glibc.
> 
> certainly true, but the current expectation is that you don't mix your ABIs.

Whose expectation?  Which ABIs are being mixed?

> if you're programming with the C library API, then use the C library headers.  
> if you're banging directly on the kernel, then use the kernel headers.  not 
> saying it's a perfect solution, but it works for the vast majority of use 
> cases.

In practice most C programs for Linux will use a mixture of thinly
wrapped system calls and higher-level APIs from the C library, and never
really call the kernel directly (as that requires inline assembler).
Userland programmers will work around this historical mess by tweaking
the #include order or splitting source files.  But they shouldn't have
to.

Ben.
Mike Frysinger Jan. 16, 2013, 5:28 p.m. UTC | #8
On Wednesday 16 January 2013 12:10:11 Ben Hutchings wrote:
> On Wed, 2013-01-16 at 12:04 -0500, Mike Frysinger wrote:
> > On Wednesday 16 January 2013 10:47:12 Ben Hutchings wrote:
> > > On Wed, 2013-01-16 at 23:21 +0900, YOSHIFUJI Hideaki wrote:
> > > > Cong Wang wrote:
> > > > > (Cc'ing some glibc developers...)
> > > > > 
> > > > > Hello,
> > > > > 
> > > > > In glibc source file inet/netinet/in.h and kernel source file
> > > > > include/uapi/linux/in6.h, both define struct in6_addr, and both are
> > > > > visible to user applications. Thomas reported a conflict below.
> > > > > 
> > > > > So, how can we handle this? /me is wondering why we didn't see this
> > > > > before.
> > > 
> > > [...]
> > > 
> > > > This is not a new issue.  In addition to this,
> > > > netinet/in.h also conflits with linux/in.h.
> > > > 
> > > > We might have
> > > > 
> > > >  #if !defined(__GLIBC__) || !defined(_NETINET_IN_H)
> > > >  
> > > >  #endif
> > > > 
> > > > around those conflicting definitions in uapi/linux/in{,6}.h.
> > > 
> > > This only solves half the problem, as <netinet/in.h> might be included
> > > after <linux/in.h>.  Also, not all Linux userland uses glibc.
> > 
> > certainly true, but the current expectation is that you don't mix your
> > ABIs.
> 
> Whose expectation?  Which ABIs are being mixed?

the kernel's view of the world and the C library's view of the world.  there 
is a lot of overlap, but it's the C library's job to provide a POSIX compliant 
interface (and then some).

obvious examples:
 - the stat structures are not the same and require translation
 - ptrace structures are not the same and you really need to use the C lib one
 - the readdir function is completely different

but to this specific question, if you're calling the kernel's network functions 
directly, then you need to include the kernel's headers for its definition of 
how things are passed.  if you're calling the C library's network functions, 
then use the C library's headers.

> > if you're programming with the C library API, then use the C library
> > headers. if you're banging directly on the kernel, then use the kernel
> > headers.  not saying it's a perfect solution, but it works for the vast
> > majority of use cases.
> 
> In practice most C programs for Linux will use a mixture of thinly
> wrapped system calls and higher-level APIs from the C library, and never
> really call the kernel directly (as that requires inline assembler).
> Userland programmers will work around this historical mess by tweaking
> the #include order or splitting source files.  But they shouldn't have
> to.

if you're not calling the kernel directly, why are you including the kernel 
headers ?  what is the problem people are actually trying to address here (and 
no, "i want to include both headers" is not the answer) ?
-mike
David Miller Jan. 16, 2013, 6:57 p.m. UTC | #9
From: Mike Frysinger <vapier@gentoo.org>
Date: Wed, 16 Jan 2013 12:04:56 -0500

> certainly true, but the current expectation is that you don't mix your ABIs.  
> if you're programming with the C library API, then use the C library headers.  
> if you're banging directly on the kernel, then use the kernel headers.  not 
> saying it's a perfect solution, but it works for the vast majority of use 
> cases.

This isn't how real life works.

GLIBC itself brings in some of the kernel headers, as do various library
headers for libraries other than glibc.

So you can get these conflicting headers included indirectly, and it is
of no fault of any of the various parties involved.

We have to make them work when included at the same time somehow, and
this is totally unavoidable.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 16, 2013, 6:59 p.m. UTC | #10
From: Mike Frysinger <vapier@gentoo.org>
Date: Wed, 16 Jan 2013 12:28:39 -0500

> if you're not calling the kernel directly, why are you including the kernel 
> headers ?  what is the problem people are actually trying to address here (and 
> no, "i want to include both headers" is not the answer) ?

When GLIBC doesn't provide it's own definition of some networking
macros or interfaces that the kernel provides, people include the
kernel header.

This has been done for decades, wake up.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Frysinger Jan. 16, 2013, 7:22 p.m. UTC | #11
On Wednesday 16 January 2013 13:59:59 David Miller wrote:
> From: Mike Frysinger <vapier@gentoo.org>
> > if you're not calling the kernel directly, why are you including the
> > kernel headers ?  what is the problem people are actually trying to
> > address here (and no, "i want to include both headers" is not the
> > answer) ?
> 
> When GLIBC doesn't provide it's own definition of some networking
> macros or interfaces that the kernel provides, people include the
> kernel header.

sounds like glibc's headers are out of date and we should update them to 
include the missing definitions

but this is still too vague.  what headers/definitions do people want to see 
simultaneously included ?  changes would be needed on both sides (kernel & C 
library).

> This has been done for decades, wake up.

and it's been broken for just as long.  no need to be a dick.
-mike
David Miller Jan. 16, 2013, 7:25 p.m. UTC | #12
From: Mike Frysinger <vapier@gentoo.org>
Date: Wed, 16 Jan 2013 14:22:16 -0500

> On Wednesday 16 January 2013 13:59:59 David Miller wrote:
>> This has been done for decades, wake up.
> 
> and it's been broken for just as long.  no need to be a dick.

By being ignorant and having such a simplistic view of the situation,
you're the one who's actually the dick.
 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Frysinger Jan. 16, 2013, 7:29 p.m. UTC | #13
On Wednesday 16 January 2013 13:57:44 David Miller wrote:
> From: Mike Frysinger <vapier@gentoo.org>
> > certainly true, but the current expectation is that you don't mix your
> > ABIs. if you're programming with the C library API, then use the C
> > library headers. if you're banging directly on the kernel, then use the
> > kernel headers.  not saying it's a perfect solution, but it works for
> > the vast majority of use cases.
> 
> This isn't how real life works.
> 
> GLIBC itself brings in some of the kernel headers, as do various library
> headers for libraries other than glibc.
> 
> So you can get these conflicting headers included indirectly, and it is
> of no fault of any of the various parties involved.

the headers glibc includes tend to be pretty stand alone specifically so that 
it doesn't matter

> We have to make them work when included at the same time somehow, and
> this is totally unavoidable.

"them" is vague.  saying that every kernel header has to be usable in the same 
compilation unit as every C library header regardless of order is unrealistic 
(at least it is today).  there are cases where they define the same structure 
different because the structure as the C library expects is different from what 
the kernel syscall expects.  you could avoid that on the kernel side by giving 
them all prefixes (like __kernel_), but that didn't seem entirely palpable to 
the kernel folks.  i couldn't even get them to remove crap that breaks non-
glibc C libraries (e.g. uapi/linux/stat.h -- looks like someone inadvertently 
fixed uapi/linux/socket.h finally).

for many networking headers, the C library will provide enums & defines while 
the kernel only provides enums.  including the kernel after the C library one 
leads to parsing errors as the defines expand in the enum and kill it.  like 
linux/in.h and netinet/in.h and IPPROTO_*.
-mike
David Miller Jan. 16, 2013, 9:45 p.m. UTC | #14
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 16 Jan 2013 15:47:12 +0000

> On Wed, 2013-01-16 at 23:21 +0900, YOSHIFUJI Hideaki wrote:
>> Cong Wang wrote:
>> > (Cc'ing some glibc developers...)
>> > 
>> > Hello,
>> > 
>> > In glibc source file inet/netinet/in.h and kernel source file
>> > include/uapi/linux/in6.h, both define struct in6_addr, and both are
>> > visible to user applications. Thomas reported a conflict below.
>> > 
>> > So, how can we handle this? /me is wondering why we didn't see this
>> > before.
> [...]
>> This is not a new issue.  In addition to this,
>> netinet/in.h also conflits with linux/in.h.
>> 
>> We might have
>>  #if !defined(__GLIBC__) || !defined(_NETINET_IN_H)
>>  :
>>  #endif
>> around those conflicting definitions in uapi/linux/in{,6}.h.
> 
> This only solves half the problem, as <netinet/in.h> might be included
> after <linux/in.h>.  Also, not all Linux userland uses glibc.

So I've been looking at reasonable ways to fix this.

What would be really nice is if GCC treated multiple identical
definitions of structures the same way it handles multiple identical
definitions of CPP defines.  Which is to silently accept them.

But that's not the case, so we need a different solution.

Another thing to do is to use a new structure for in6_addr in kernel
headers exported to userland.

If we were to make the structure members and accessor macros identical
we could avoid breaking most if not all apps.

However the type name is different so:

	struct in6_addr *p = &kern_struct->member;

wouldn't work so well.  And you couldn't fix up the sources to these
kinds of accesses in a way that work cleanly both before and after
changing the kernel headers.

I'm out of ideas for today.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Carlos O'Donell Jan. 17, 2013, 1:58 a.m. UTC | #15
On 01/16/2013 04:45 PM, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Wed, 16 Jan 2013 15:47:12 +0000
> 
>> On Wed, 2013-01-16 at 23:21 +0900, YOSHIFUJI Hideaki wrote:
>>> Cong Wang wrote:
>>>> (Cc'ing some glibc developers...)
>>>>
>>>> Hello,
>>>>
>>>> In glibc source file inet/netinet/in.h and kernel source file
>>>> include/uapi/linux/in6.h, both define struct in6_addr, and both are
>>>> visible to user applications. Thomas reported a conflict below.
>>>>
>>>> So, how can we handle this? /me is wondering why we didn't see this
>>>> before.
>> [...]
>>> This is not a new issue.  In addition to this,
>>> netinet/in.h also conflits with linux/in.h.
>>>
>>> We might have
>>>  #if !defined(__GLIBC__) || !defined(_NETINET_IN_H)
>>>  :
>>>  #endif
>>> around those conflicting definitions in uapi/linux/in{,6}.h.
>>
>> This only solves half the problem, as <netinet/in.h> might be included
>> after <linux/in.h>.  Also, not all Linux userland uses glibc.
> 
> So I've been looking at reasonable ways to fix this.
> 
> What would be really nice is if GCC treated multiple identical
> definitions of structures the same way it handles multiple identical
> definitions of CPP defines.  Which is to silently accept them.
> 
> But that's not the case, so we need a different solution.
> 
> Another thing to do is to use a new structure for in6_addr in kernel
> headers exported to userland.
> 
> If we were to make the structure members and accessor macros identical
> we could avoid breaking most if not all apps.
> 
> However the type name is different so:
> 
> 	struct in6_addr *p = &kern_struct->member;
> 
> wouldn't work so well.  And you couldn't fix up the sources to these
> kinds of accesses in a way that work cleanly both before and after
> changing the kernel headers.
> 
> I'm out of ideas for today.

So I just went down the rabbit hole, and the further I get the
closer I get to having two exact copies of the same definitions
in both glibc and the kernel and using whichever one was included
first.

Is anyone opposed to that kind of solution?

There is some ugliness there.

Cheers,
Carlos.
 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 17, 2013, 2:05 a.m. UTC | #16
From: Carlos O'Donell <carlos@systemhalted.org>
Date: Wed, 16 Jan 2013 20:58:47 -0500

> So I just went down the rabbit hole, and the further I get the
> closer I get to having two exact copies of the same definitions
> in both glibc and the kernel and using whichever one was included
> first.
> 
> Is anyone opposed to that kind of solution?

Sounds interesting, please share :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang Jan. 17, 2013, 3:40 a.m. UTC | #17
On Wed, 2013-01-16 at 14:22 -0500, Mike Frysinger wrote:
> 
> but this is still too vague.  what headers/definitions do people want to see 
> simultaneously included ?  changes would be needed on both sides (kernel & C 
> library).
> 

Hi, Mike,

Please take a look at my first email in this thread. The user
application includes <linux/if_bridge.h> and <netinet/in.h>.

<linux/if_bridge.h> uses struct_in6 but doesn't include <linux/in6.h>
(this is my bad, sorry), an obvious fix is just including <linux/in6.h>.
But this immediately breaks applications which include
<linux/if_bridge.h> and <netinet/in.h>, just as what Thomas reported.

And if_bridge.h is kernel-specific, there is no corresponding glibc one,
so you can't blame applications which include both of them.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang Jan. 17, 2013, 6:59 a.m. UTC | #18
On Thu, 2013-01-17 at 11:55 +0800, Jike Song wrote:
> On Thu, Jan 17, 2013 at 2:59 AM, David Miller <davem@davemloft.net> wrote:
> 
> >
> > When GLIBC doesn't provide it's own definition of some networking
> > macros or interfaces that the kernel provides, people include the
> > kernel header.
> >
> 
> Recently I got a problem when copying a structure from kernel to userspace,
> after debugging I found:
> 
> kernel:   include/linux/inet.h
> 
> #define INET6_ADDRSTRLEN        (48)
> 
> glibc:  /usr/include/netinet/in.h
> 
> #define INET6_ADDRSTRLEN 46
> 
> 
> Any reason to differentiate them from each other?
> 

I see no reason, even although I don't know why it is 46 instead of 40.

But include/linux/inet.h is not exported to user-space, AFAIK.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang Jan. 17, 2013, 7:02 a.m. UTC | #19
----- Original Message -----
> 
> I see no reason, even although I don't know why it is 46 instead of
> 40.

Ok, for "0000:0000:0000:0000:0000:0000:255.255.255.255".
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt Jan. 17, 2013, 10:57 a.m. UTC | #20
On Thursday 2013-01-17 03:05, David Miller wrote:

>From: Carlos O'Donell <carlos@systemhalted.org>
>Date: Wed, 16 Jan 2013 20:58:47 -0500
>
>> So I just went down the rabbit hole, and the further I get the
>> closer I get to having two exact copies of the same definitions
>> in both glibc and the kernel and using whichever one was included
>> first.
>> 
>> Is anyone opposed to that kind of solution?
>
>Sounds interesting, please share :-)

iptables has the same issue, and solved it its way. 
(uapi/)linux/netfilter.h is used to get at things like union 
nf_inet_addr. This union contains struct in6_addr. There is no include 
for in6_addr in netfilter.h itself. This may break the "standalone 
compilation" test, but at least allows for specifying the 
environment-specific header for in6_addr in the C file:

a. userspace: #include <netinet/in.h> before <linux/netfilter.h>
b. kernel parts: #include <linux/in6.h> before <linux/netfilter.h>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Frysinger Jan. 18, 2013, 4:14 a.m. UTC | #21
On Wednesday 16 January 2013 16:45:11 David Miller wrote:
> What would be really nice is if GCC treated multiple identical
> definitions of structures the same way it handles multiple identical
> definitions of CPP defines.  Which is to silently accept them.
> 
> But that's not the case, so we need a different solution.
> 
> Another thing to do is to use a new structure for in6_addr in kernel
> headers exported to userland.

the kernel already exports many types with a __kernel_ prefix.  i changed the 
kernel headers in Gentoo for a few releases (2.6.28 - 2.6.34) to do the same 
thing to pretty much all the networking headers.  a few packages broke, but 
the number was low, and trivial to fix (a sed would do it most of the time).

it's also trivial for userland packages to detect that they need to do this 
sort of thing in a local header by using linux/version.h and a set of defines 
to redirect the new structure name back to the old one.

would be a lot cleaner to just break it and be done.
-mike
David Miller Jan. 18, 2013, 4:55 a.m. UTC | #22
From: Mike Frysinger <vapier@gentoo.org>
Date: Thu, 17 Jan 2013 23:14:31 -0500

> the kernel already exports many types with a __kernel_ prefix.  i changed the 
> kernel headers in Gentoo for a few releases (2.6.28 - 2.6.34) to do the same 
> thing to pretty much all the networking headers.  a few packages broke, but 
> the number was low, and trivial to fix (a sed would do it most of the time).
> 
> it's also trivial for userland packages to detect that they need to do this 
> sort of thing in a local header by using linux/version.h and a set of defines 
> to redirect the new structure name back to the old one.
> 
> would be a lot cleaner to just break it and be done.

We are not at liberty to break something that has legitimately
compiled successfully for two decades.

Your __kernel_ prefix idea breaks compilation just as equally.

One thing you certainly don't have to be is happy about this header
file situation, but you cannot use that dissatisfaction as
justification to make the situation worse by breaking the build for
people outside of the world you directly control.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Frysinger Jan. 18, 2013, 5:27 a.m. UTC | #23
On Thursday 17 January 2013 23:55:24 David Miller wrote:
> From: Mike Frysinger <vapier@gentoo.org>
> > the kernel already exports many types with a __kernel_ prefix.  i changed
> > the kernel headers in Gentoo for a few releases (2.6.28 - 2.6.34) to do
> > the same thing to pretty much all the networking headers.  a few
> > packages broke, but the number was low, and trivial to fix (a sed would
> > do it most of the time).
> > 
> > it's also trivial for userland packages to detect that they need to do
> > this sort of thing in a local header by using linux/version.h and a set
> > of defines to redirect the new structure name back to the old one.
> > 
> > would be a lot cleaner to just break it and be done.
> 
> We are not at liberty to break something that has legitimately
> compiled successfully for two decades.

i doubt there are code bases that have compiled successfully for that long 
w/out being changed.  in fact, the kernel user headers (until recently) have 
been a volatile mess requiring constant care & feeding of user programs to 
keep compiling.  if anything, this would simply be maintaining the status quo 
for a bit longer.

> One thing you certainly don't have to be is happy about this header
> file situation, but you cannot use that dissatisfaction as
> justification to make the situation worse by breaking the build for
> people outside of the world you directly control.

i think you're making wrong assumptions about my position on the topic.  i 
never said i was against cleaning up the kernel user headers (and in fact, you 
can find many commits from me doing exactly that over the years in the kernel).
-mike
diff mbox

Patch

diff --git a/include/uapi/linux/if_bridge.h
b/include/uapi/linux/if_bridge.h
index 5db2975..653db23 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -14,6 +14,7 @@ 
 #define _UAPI_LINUX_IF_BRIDGE_H
 
 #include <linux/types.h>
+#include <linux/in6.h>
 
 #define SYSFS_BRIDGE_ATTR      "bridge"
 #define SYSFS_BRIDGE_FDB       "brforward"