diff mbox

misc/sys/xattr.h: guard against linux uapi header inclusion

Message ID 20140311041707.GA6064@sergelap
State Committed
Headers

Commit Message

Serge Hallyn March 11, 2014, 4:17 a.m. UTC 
  If the glibc xattr.h header is included after the uapi header,
compilation fails due to an enum re-using a #define from the
uapi header.  Protect against this by guarding the define and
enum inclusions against each other.

(A corresponding kernel patch has been sent here:
http://lkml.org/lkml/2014/3/7/331 )

(See https://lists.debian.org/debian-glibc/2014/03/msg00029.html
and https://sourceware.org/glibc/wiki/Synchronizing_Headers
for more information.)

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 misc/sys/xattr.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Ondrej Bilka March 18, 2014, 1:27 p.m. UTC | #1 
On Mon, Mar 10, 2014 at 11:17:07PM -0500, Serge Hallyn wrote:
> If the glibc xattr.h header is included after the uapi header,
> compilation fails due to an enum re-using a #define from the
> uapi header.  Protect against this by guarding the define and
> enum inclusions against each other.
> 
> (A corresponding kernel patch has been sent here:
> http://lkml.org/lkml/2014/3/7/331 )
> 
> (See https://lists.debian.org/debian-glibc/2014/03/msg00029.html
> and https://sourceware.org/glibc/wiki/Synchronizing_Headers
> for more information.)
> 
> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
>
Looks ok, I commited that with appropriate ChangeLog.
 
> ---
> misc/sys/xattr.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/misc/sys/xattr.h b/misc/sys/xattr.h
> index 929cd87..796df90 100644
> --- a/misc/sys/xattr.h
> +++ b/misc/sys/xattr.h
> @@ -26,6 +26,7 @@ __BEGIN_DECLS
>  
>  /* The following constants should be used for the fifth parameter of
>     `*setxattr'.  */
> +#ifndef __USE_KERNEL_XATTR_DEFS
>  enum
>  {
>    XATTR_CREATE = 1,	/* set value, fail if attr already exists.  */
> @@ -33,6 +34,7 @@ enum
>    XATTR_REPLACE = 2	/* set value, fail if attr does not exist.  */
>  #define XATTR_REPLACE	XATTR_REPLACE
>  };
> +#endif
>  
>  /* Set the attribute NAME of the file pointed to by PATH to VALUE (which
>     is SIZE bytes long).  Return 0 on success, -1 for errors.  */
Allan McRae March 20, 2014, 11:46 p.m. UTC | #2 
On 18/03/14 23:27, Ondřej Bílka wrote:
> On Mon, Mar 10, 2014 at 11:17:07PM -0500, Serge Hallyn wrote:
>> If the glibc xattr.h header is included after the uapi header,
>> compilation fails due to an enum re-using a #define from the
>> uapi header.  Protect against this by guarding the define and
>> enum inclusions against each other.
>>
>> (A corresponding kernel patch has been sent here:
>> http://lkml.org/lkml/2014/3/7/331 )
>>
>> (See https://lists.debian.org/debian-glibc/2014/03/msg00029.html
>> and https://sourceware.org/glibc/wiki/Synchronizing_Headers
>> for more information.)
>>
>> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
>>
> Looks ok, I commited that with appropriate ChangeLog.
>  

Has the kernel patch been reviewed?  This should not have been committed
before there was agreement between both parties that the patches were good.

The kernel patch in the link looks wrong to me. The define
__UAPI_DEF_XATTR is set to 0 whenever any glibc header is included, not
just sys/xattr.h. It should be wrapped in "#ifdef _SYS_XATTR_H".

The glibc patch is fine, but pointless until is the kernel patch is
accepted.

>> ---
>> misc/sys/xattr.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/misc/sys/xattr.h b/misc/sys/xattr.h
>> index 929cd87..796df90 100644
>> --- a/misc/sys/xattr.h
>> +++ b/misc/sys/xattr.h
>> @@ -26,6 +26,7 @@ __BEGIN_DECLS
>>  
>>  /* The following constants should be used for the fifth parameter of
>>     `*setxattr'.  */
>> +#ifndef __USE_KERNEL_XATTR_DEFS
>>  enum
>>  {
>>    XATTR_CREATE = 1,	/* set value, fail if attr already exists.  */
>> @@ -33,6 +34,7 @@ enum
>>    XATTR_REPLACE = 2	/* set value, fail if attr does not exist.  */
>>  #define XATTR_REPLACE	XATTR_REPLACE
>>  };
>> +#endif
>>  
>>  /* Set the attribute NAME of the file pointed to by PATH to VALUE (which
>>     is SIZE bytes long).  Return 0 on success, -1 for errors.  */
> 
> 
>
Serge Hallyn March 21, 2014, 2:26 a.m. UTC | #3 
Quoting Allan McRae (allan@archlinux.org):
> On 18/03/14 23:27, Ondřej Bílka wrote:
> > On Mon, Mar 10, 2014 at 11:17:07PM -0500, Serge Hallyn wrote:
> >> If the glibc xattr.h header is included after the uapi header,
> >> compilation fails due to an enum re-using a #define from the
> >> uapi header.  Protect against this by guarding the define and
> >> enum inclusions against each other.
> >>
> >> (A corresponding kernel patch has been sent here:
> >> http://lkml.org/lkml/2014/3/7/331 )
> >>
> >> (See https://lists.debian.org/debian-glibc/2014/03/msg00029.html
> >> and https://sourceware.org/glibc/wiki/Synchronizing_Headers
> >> for more information.)
> >>
> >> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
> >>
> > Looks ok, I commited that with appropriate ChangeLog.
> >  
> 
> Has the kernel patch been reviewed?  This should not have been committed

Nope, no feedback yet.

> before there was agreement between both parties that the patches were good.
> 
> The kernel patch in the link looks wrong to me. The define
> __UAPI_DEF_XATTR is set to 0 whenever any glibc header is included, not
> just sys/xattr.h. It should be wrapped in "#ifdef _SYS_XATTR_H".

Good point, thanks!  I missed that part of the network patch I
was emulating.  I'll fix it up, resend, and cc: akpm.

> The glibc patch is fine, but pointless until is the kernel patch is
> accepted.
> >> ---
> >> misc/sys/xattr.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/misc/sys/xattr.h b/misc/sys/xattr.h
> >> index 929cd87..796df90 100644
> >> --- a/misc/sys/xattr.h
> >> +++ b/misc/sys/xattr.h
> >> @@ -26,6 +26,7 @@ __BEGIN_DECLS
> >>  
> >>  /* The following constants should be used for the fifth parameter of
> >>     `*setxattr'.  */
> >> +#ifndef __USE_KERNEL_XATTR_DEFS
> >>  enum
> >>  {
> >>    XATTR_CREATE = 1,	/* set value, fail if attr already exists.  */
> >> @@ -33,6 +34,7 @@ enum
> >>    XATTR_REPLACE = 2	/* set value, fail if attr does not exist.  */
> >>  #define XATTR_REPLACE	XATTR_REPLACE
> >>  };
> >> +#endif
> >>  
> >>  /* Set the attribute NAME of the file pointed to by PATH to VALUE (which
> >>     is SIZE bytes long).  Return 0 on success, -1 for errors.  */
> > 
> > 
> > 
>
diff mbox

Patch

diff --git a/misc/sys/xattr.h b/misc/sys/xattr.h
index 929cd87..796df90 100644
--- a/misc/sys/xattr.h
+++ b/misc/sys/xattr.h
@@ -26,6 +26,7 @@  __BEGIN_DECLS
 
 /* The following constants should be used for the fifth parameter of
    `*setxattr'.  */
+#ifndef __USE_KERNEL_XATTR_DEFS
 enum
 {
   XATTR_CREATE = 1,	/* set value, fail if attr already exists.  */
@@ -33,6 +34,7 @@  enum
   XATTR_REPLACE = 2	/* set value, fail if attr does not exist.  */
 #define XATTR_REPLACE	XATTR_REPLACE
 };
+#endif
 
 /* Set the attribute NAME of the file pointed to by PATH to VALUE (which
    is SIZE bytes long).  Return 0 on success, -1 for errors.  */