struct fsxattr redefinition

struct fsxattr redefinition

[Jeffrey Bastian]

There was a discussion a few months ago about adding a guard for the
fsxattr struct [0] because it's defined in two places, the Linux kernel
header linux/fs.h [1] and xfsprogs header xfs/linux.h [2].

xfs/linux.h has a FS_IOC_FSGETXATTR guard around the struct fsxattr
definition, but this only works if linux/fs.h is included *before*
xfs/linux.h (or xfs/xfs.h).  If you include linux/fs.h after, then you
get a struct redefinition error.

Is it a requirement that linux/fs.h is included first?  If so, then
there is a bug in xfstests because it includes them in the wrong order
[3] and fails to build.  If there is not an order requirement, then both
header files should probably have a HAVE_FSXATTR guard around the struct
definition.

[0] http://oss.sgi.com/archives/xfs/2016-02/msg00306.html
[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fs.h?h=v4.6#n152
[2] http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/cmds/xfsprogs.git;a=blob;f=include/linux.h;h=cc0f70ceed72406822885d878b939b3e66c03ffd;hb=53cefc406eb1047ce9dc85906a0117f8da9f44ca#l177
[3] http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/cmds/xfstests.git;a=blob;f=src/t_immutable.c;h=87ffc75f2d0229941ad6c5235ce9deea272d66c9;hb=HEAD#l41

-- 
Jeff Bastian

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: struct fsxattr redefinition

[Eric Sandeen]

On 5/18/16 11:37 AM, Jeffrey Bastian wrote:
> There was a discussion a few months ago about adding a guard for the
> fsxattr struct [0] because it's defined in two places, the Linux kernel
> header linux/fs.h [1] and xfsprogs header xfs/linux.h [2].

> xfs/linux.h has a FS_IOC_FSGETXATTR guard around the struct fsxattr
> definition, but this only works if linux/fs.h is included *before*
> xfs/linux.h (or xfs/xfs.h).  If you include linux/fs.h after, then you
> get a struct redefinition error.
> 
> Is it a requirement that linux/fs.h is included first?  If so, then
> there is a bug in xfstests because it includes them in the wrong order
> [3] and fails to build.  If there is not an order requirement, then both
> header files should probably have a HAVE_FSXATTR guard around the struct
> definition.

It seems best to me to include fs.h first.  That may not be written in
stone, but it's at least common practice.

Having the same definition in both places, and guards going both ways,
seems a little odd though.

Maybe xfsprogs' include/linux.h should just directly include
the kernel's linux/fs.h at the top - would that make sense?

-Eric

> [0] http://oss.sgi.com/archives/xfs/2016-02/msg00306.html
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fs.h?h=v4.6#n152
> [2] http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/cmds/xfsprogs.git;a=blob;f=include/linux.h;h=cc0f70ceed72406822885d878b939b3e66c03ffd;hb=53cefc406eb1047ce9dc85906a0117f8da9f44ca#l177
> [3] http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/cmds/xfstests.git;a=blob;f=src/t_immutable.c;h=87ffc75f2d0229941ad6c5235ce9deea272d66c9;hb=HEAD#l41
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: struct fsxattr redefinition

[Jeffrey Bastian]

On Wed, May 18, 2016 at 03:44:46PM -0500, Eric Sandeen wrote:
> On 5/18/16 11:37 AM, Jeffrey Bastian wrote:
> > There was a discussion a few months ago about adding a guard for the
> > fsxattr struct [0] because it's defined in two places, the Linux kernel
> > header linux/fs.h [1] and xfsprogs header xfs/linux.h [2].
> 
> > xfs/linux.h has a FS_IOC_FSGETXATTR guard around the struct fsxattr
> > definition, but this only works if linux/fs.h is included *before*
> > xfs/linux.h (or xfs/xfs.h).  If you include linux/fs.h after, then you
> > get a struct redefinition error.
> > 
> > Is it a requirement that linux/fs.h is included first?  If so, then
> > there is a bug in xfstests because it includes them in the wrong order
> > [3] and fails to build.  If there is not an order requirement, then both
> > header files should probably have a HAVE_FSXATTR guard around the struct
> > definition.
> 
> It seems best to me to include fs.h first.  That may not be written in
> stone, but it's at least common practice.
> 
> Having the same definition in both places, and guards going both ways,
> seems a little odd though.
> 
> Maybe xfsprogs' include/linux.h should just directly include
> the kernel's linux/fs.h at the top - would that make sense?

I sent a patch to the fstests list to change the order of the headers
(two tests needed updating, src/t_immutable.c and ltp/fsstress.c).

I'm assuming the struct definition is in both spots because xfsprogs is
meant to be portable to Irix and FreeBSD and other *nix flavors?  If so,
then the double-definition isn't so weird, and wrapping both definitions
with the same HAVE_FSXATTR guard is the logical thing to do to prevent
double-definitions from accidentally including the files in the "wrong"
order.

On the other hand, xfsprogs' include/linux.h is obviously meant to be
for Linux alone, so including linux/fs.h would also solve the problem
since it will bring in FS_IOC_FSGETXATTR and thus skip the second
definition of fsxattr.  I like this idea.

A quick and simple test shows this works.  Before:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$ grep linux.fs.h /usr/include/xfs/linux.h

$ cat test-fsxattr.c
#define _LARGEFILE64_SOURCE
#include <sys/types.h>
#ifdef BAD
# include <xfs/xfs.h>
# include <linux/fs.h>
#else
# include <linux/fs.h>
# include <xfs/xfs.h>
#endif
int main(void)
{
  struct fsxattr f;
  f.fsx_xflags = 1;
  return 0;
}

$ gcc test-fsxattr.c

$ gcc -DBAD test-fsxattr.c
In file included from test-fsxattr.c:5:0:
/usr/include/linux/fs.h:157:8: error: redefinition of ‘struct fsxattr’
 struct fsxattr {
        ^
In file included from /usr/include/xfs/xfs.h:37:0,
                 from test-fsxattr.c:4:
/usr/include/xfs/linux.h:183:8: note: originally defined here
 struct fsxattr {
        ^
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Now include linux/fs.h in xfs/linux.h and try again:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$ sudo vi /usr/include/xfs/linux.h

$ grep linux.fs.h /usr/include/xfs/linux.h
#include <linux/fs.h>

$ gcc test-fsxattr.c

$ gcc -DBAD test-fsxattr.c

$
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The order of inclusion no longer matters!

-- 
Jeff Bastian

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: struct fsxattr redefinition

[Dave Chinner]

On Wed, May 18, 2016 at 03:44:46PM -0500, Eric Sandeen wrote:
> On 5/18/16 11:37 AM, Jeffrey Bastian wrote:
> > There was a discussion a few months ago about adding a guard for the
> > fsxattr struct [0] because it's defined in two places, the Linux kernel
> > header linux/fs.h [1] and xfsprogs header xfs/linux.h [2].
> 
> > xfs/linux.h has a FS_IOC_FSGETXATTR guard around the struct fsxattr
> > definition, but this only works if linux/fs.h is included *before*
> > xfs/linux.h (or xfs/xfs.h).  If you include linux/fs.h after, then you
> > get a struct redefinition error.
> > 
> > Is it a requirement that linux/fs.h is included first?  If so, then
> > there is a bug in xfstests because it includes them in the wrong order
> > [3] and fails to build.  If there is not an order requirement, then both
> > header files should probably have a HAVE_FSXATTR guard around the struct
> > definition.
> 
> It seems best to me to include fs.h first.  That may not be written in
> stone, but it's at least common practice.
> 
> Having the same definition in both places, and guards going both ways,
> seems a little odd though.
> 
> Maybe xfsprogs' include/linux.h should just directly include
> the kernel's linux/fs.h at the top - would that make sense?

That's the easiest solution - stops people wasting even more time on
this.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs