xfsprogs: guard fsxattr definition for newer kernels

PATCH In review Eric Sandeen 3 participants 8 messages Feb 9, 2016

lore

Share & tools

lore

Eric Sandeen

author

2016-02-09 17:41 UTC #0 lore raw

PATCH 1/1 xfsprogs: guard fsxattr definition for newer kernels

After 334e580,
fs: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR promotion

the file include/linux/fs.h now defines struct fsxattr.

It defines FS_IOC_FSGETXATTR as well, so use that to wrap our local definition, and skip it if the kernel is providing it so that we don't get multiple definitions.

Signed-off-by Eric Sandeen <[email protected]>

---

Should the kernel also #define HAVE_FSXATTR to help existing xfsprogs-devel installations?

(And what if headers are included in the other order? Should we try to guard on the kernel side or no?)

libxfs/xfs_fs.h +3 -1

index d8b733a..f4858df 100644
@@ -37,8 +37,10 @@ struct dioattr {
 
 /*
  * Structure for XFS_IOC_FSGETXATTR[A] and XFS_IOC_FSSETXATTR.
+ * If FS_IOC_FSGETXATTR is defined, the kernel is now providing
+ * this structure definition.
  */
-#ifndef HAVE_FSXATTR
+#if (!defined(HAVE_FSXATTR) && !defined(FS_IOC_FSGETXATTR))
 struct fsxattr {
 	__u32		fsx_xflags;	/* xflags field value (get/set) */
 	__u32		fsx_extsize;	/* extsize field value (get/set)*/

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2016-02-09 19:55 UTC #1 lore raw

Quoted 9 lines · Eric Sandeen

After 334e580,
fs: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR promotion
the file include/linux/fs.h now defines struct fsxattr.
It defines FS_IOC_FSGETXATTR as well, so use that to wrap our local definition, and skip it if the kernel is providing it so that we don't get multiple definitions.

Quoted 8 lines

Signed-off-by Eric Sandeen <[email protected]>
---
Should the kernel also #define HAVE_FSXATTR to help existing xfsprogs-devel installations?
(And what if headers are included in the other order? Should we try to guard on the kernel side or no?)

I've already sent a patch to fix this - it was with the foreign filesystem xfs_quota patch....

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2016-02-09 19:57 UTC #2 lore raw

Quoted 13 lines · Dave Chinner

Quoted 12 lines · Eric Sandeen
After 334e580,
fs: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR promotion
the file include/linux/fs.h now defines struct fsxattr.
It defines FS_IOC_FSGETXATTR as well, so use that to wrap our local definition, and skip it if the kernel is providing it so that we don't get multiple definitions.
Signed-off-by Eric Sandeen <[email protected]>
---

Quoted 8 lines

Quoted 5 lines
Should the kernel also #define HAVE_FSXATTR to help existing xfsprogs-devel installations?
(And what if headers are included in the other order? Should we try to guard on the kernel side or no?)
I've already sent a patch to fix this - it was with the foreign filesystem xfs_quota patch....

Oh, sorry, spaced it.

What do you think of the HAVE_FSXATTR definition in fs.h?

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2016-02-09 21:10 UTC #3 lore raw

Quoted 20 lines · Eric Sandeen

Quoted 19 lines · Dave Chinner
Quoted 17 lines · Eric Sandeen
After 334e580,
fs: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR promotion
the file include/linux/fs.h now defines struct fsxattr.
It defines FS_IOC_FSGETXATTR as well, so use that to wrap our local definition, and skip it if the kernel is providing it so that we don't get multiple definitions.
Signed-off-by Eric Sandeen <[email protected]>
---
Should the kernel also #define HAVE_FSXATTR to help existing xfsprogs-devel installations?
(And what if headers are included in the other order? Should we try to guard on the kernel side or no?)

Quoted 6 lines

Quoted 2 lines
I've already sent a patch to fix this - it was with the foreign filesystem xfs_quota patch....
Oh, sorry, spaced it.
What do you think of the HAVE_FSXATTR definition in fs.h?

Which fs.h? The include/linux/fs.h file does not have such guards...

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2016-02-09 21:27 UTC #4 lore raw

Quoted 24 lines · Dave Chinner

Quoted 23 lines · Eric Sandeen
Quoted 21 lines · Dave Chinner
Quoted 9 lines · Eric Sandeen
After 334e580,
fs: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR promotion
the file include/linux/fs.h now defines struct fsxattr.
It defines FS_IOC_FSGETXATTR as well, so use that to wrap our local definition, and skip it if the kernel is providing it so that we don't get multiple definitions.
Quoted 8 lines
Signed-off-by Eric Sandeen <[email protected]>
---
Should the kernel also #define HAVE_FSXATTR to help existing xfsprogs-devel installations?
(And what if headers are included in the other order? Should we try to guard on the kernel side or no?)
I've already sent a patch to fix this - it was with the foreign filesystem xfs_quota patch....

Quoted 6 lines

Quoted 3 lines
Oh, sorry, spaced it.
What do you think of the HAVE_FSXATTR definition in fs.h?
Which fs.h? The include/linux/fs.h file does not have such guards...

If include/linux/fs.h defined HAVE_FSXATTR, a subsequent inclusion of xfs_fs.h would not redefine the structure, because it is guarded with that (for irix!)

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2016-02-09 21:44 UTC #5 lore raw

Quoted 29 lines · Eric Sandeen

Quoted 26 lines · Dave Chinner
Quoted 25 lines · Eric Sandeen
Quoted 13 lines · Dave Chinner
Quoted 12 lines · Eric Sandeen
After 334e580,
fs: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR promotion
the file include/linux/fs.h now defines struct fsxattr.
It defines FS_IOC_FSGETXATTR as well, so use that to wrap our local definition, and skip it if the kernel is providing it so that we don't get multiple definitions.
Signed-off-by Eric Sandeen <[email protected]>
---
Quoted 8 lines
Quoted 5 lines
Should the kernel also #define HAVE_FSXATTR to help existing xfsprogs-devel installations?
(And what if headers are included in the other order? Should we try to guard on the kernel side or no?)
I've already sent a patch to fix this - it was with the foreign filesystem xfs_quota patch....
Oh, sorry, spaced it.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
What do you think of the HAVE_FSXATTR definition in fs.h?
Which fs.h? The include/linux/fs.h file does not have such guards...
If include/linux/fs.h defined HAVE_FSXATTR, a subsequent inclusion of xfs_fs.h would not redefine the structure, because it is guarded with that (for irix!)

That's why I changed it to check if the ioctl is defined, rather than checking for HAVE_FSXATTR.

Looking at it, what I probably should do is something more "special" in the platform headers, so the xfs_fs.h is kept identical across kernel and userspace...

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2016-02-09 21:45 UTC #6 lore raw

Quoted 35 lines · Dave Chinner

Quoted 34 lines · Eric Sandeen
Quoted 30 lines · Dave Chinner
Quoted 20 lines · Eric Sandeen
Quoted 19 lines · Dave Chinner
Quoted 17 lines · Eric Sandeen
After 334e580,
fs: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR promotion
the file include/linux/fs.h now defines struct fsxattr.
It defines FS_IOC_FSGETXATTR as well, so use that to wrap our local definition, and skip it if the kernel is providing it so that we don't get multiple definitions.
Signed-off-by Eric Sandeen <[email protected]>
---
Should the kernel also #define HAVE_FSXATTR to help existing xfsprogs-devel installations?
(And what if headers are included in the other order? Should we try to guard on the kernel side or no?)
Quoted 6 lines
Quoted 2 lines
I've already sent a patch to fix this - it was with the foreign filesystem xfs_quota patch....
Oh, sorry, spaced it.
What do you think of the HAVE_FSXATTR definition in fs.h?
Which fs.h? The include/linux/fs.h file does not have such guards...

Quoted 6 lines

Quoted 3 lines
If include/linux/fs.h defined HAVE_FSXATTR, a subsequent inclusion of xfs_fs.h would not redefine the structure, because it is guarded with that (for irix!)
That's why I changed it to check if the ioctl is defined, rather than checking for HAVE_FSXATTR.

Right, but I'm talking about protecting older, existing versions of xfsprogs headers which use HAVE_FSXATTR as the guard.

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2016-02-09 22:38 UTC #7 lore raw

Quoted 42 lines · Eric Sandeen

Quoted 39 lines · Dave Chinner
Quoted 37 lines · Eric Sandeen
Quoted 24 lines · Dave Chinner
Quoted 23 lines · Eric Sandeen
Quoted 21 lines · Dave Chinner
Quoted 9 lines · Eric Sandeen
After 334e580,
fs: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR promotion
the file include/linux/fs.h now defines struct fsxattr.
It defines FS_IOC_FSGETXATTR as well, so use that to wrap our local definition, and skip it if the kernel is providing it so that we don't get multiple definitions.
Quoted 8 lines
Signed-off-by Eric Sandeen <[email protected]>
---
Should the kernel also #define HAVE_FSXATTR to help existing xfsprogs-devel installations?
(And what if headers are included in the other order? Should we try to guard on the kernel side or no?)
I've already sent a patch to fix this - it was with the foreign filesystem xfs_quota patch....
Quoted 6 lines
Quoted 3 lines
Oh, sorry, spaced it.
What do you think of the HAVE_FSXATTR definition in fs.h?
Which fs.h? The include/linux/fs.h file does not have such guards...
If include/linux/fs.h defined HAVE_FSXATTR, a subsequent inclusion of xfs_fs.h would not redefine the structure, because it is guarded with that (for irix!)

Quoted 5 lines

Quoted 2 lines
That's why I changed it to check if the ioctl is defined, rather than checking for HAVE_FSXATTR.
Right, but I'm talking about protecting older, existing versions of xfsprogs headers which use HAVE_FSXATTR as the guard.

Nothing we can really do about that. There's no way we can get random unused defines into general linux uapi header files.

If a distro updates their kernel to 4.5 and hence introduces this general definition, then they also be upgrading xfsprogs to match. If a user has upgraded their kernel and then tries to build xfsprogs from source, then they are also going to need to update xfsprogs...

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs