| Message ID | 20181005122741.5100-1-kilobyte@angband.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: move the define for superblock magic to uapi | expand |
On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote: > Needed by users of fstatfs(). NAK. The XFS superblock magic number is part of the on-disk format definition of XFS. It belongs with all the other on-disk format definitions in this file. I don't think it's a good idea for userspace to associate s_magic with userspace API feature sets, though the cat's long escaped the bag on that one. With that being reality, "the XFS superblock magic number" has a different semantic meaning than "the agreed upon statfs.f_type value for XFS", which means the latter should have a different symbol name to reflect that difference. --D > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > --- > fs/xfs/libxfs/xfs_format.h | 3 ++- > include/uapi/linux/magic.h | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 059bc44c27e8..837863c57e8c 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -14,6 +14,7 @@ > * xfs_da_format.h, which log and log item formats are defined in > * xfs_log_format.h. Everything else goes here. > */ > +#include <linux/magic.h> > > struct xfs_mount; > struct xfs_trans; > @@ -26,7 +27,7 @@ struct xfs_ifork; > * Fits into a sector-sized buffer at address 0 of each allocation group. > * Only the first of these is ever updated except during growfs. > */ > -#define XFS_SB_MAGIC 0x58465342 /* 'XFSB' */ > +#define XFS_SB_MAGIC XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */ > #define XFS_SB_VERSION_1 1 /* 5.3, 6.0.1, 6.1 */ > #define XFS_SB_VERSION_2 2 /* 6.2 - attributes */ > #define XFS_SB_VERSION_3 3 /* 6.2 - new inode version */ > diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h > index 1a6fee974116..96c24478d8ce 100644 > --- a/include/uapi/linux/magic.h > +++ b/include/uapi/linux/magic.h > @@ -29,6 +29,7 @@ > #define HPFS_SUPER_MAGIC 0xf995e849 > #define ISOFS_SUPER_MAGIC 0x9660 > #define JFFS2_SUPER_MAGIC 0x72b6 > +#define XFS_SUPER_MAGIC 0x58465342 /* "XFSB" */ > #define PSTOREFS_MAGIC 0x6165676C > #define EFIVARFS_MAGIC 0xde5e81e4 > #define HOSTFS_SUPER_MAGIC 0x00c0ffee > -- > 2.19.0 >
On Fri, Oct 05, 2018 at 09:06:00AM -0700, Darrick J. Wong wrote: > On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote: > > Needed by users of fstatfs(). > > NAK. > > The XFS superblock magic number is part of the on-disk format definition > of XFS. It belongs with all the other on-disk format definitions in this > file. > > I don't think it's a good idea for userspace to associate s_magic with > userspace API feature sets, though the cat's long escaped the bag on > that one. > > With that being reality, "the XFS superblock magic number" has a > different semantic meaning than "the agreed upon statfs.f_type value for > XFS", which means the latter should have a different symbol name to > reflect that difference. Ie, you'd be ok with the same value to be defined in two places -- do I understand this right? > > --- a/fs/xfs/libxfs/xfs_format.h > > -#define XFS_SB_MAGIC 0x58465342 /* 'XFSB' */ > > +#define XFS_SB_MAGIC XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */ > > --- a/include/uapi/linux/magic.h > > #define JFFS2_SUPER_MAGIC 0x72b6 > > +#define XFS_SUPER_MAGIC 0x58465342 /* "XFSB" */ > > #define PSTOREFS_MAGIC 0x6165676C Meow!
On Fri, Oct 05, 2018 at 08:10:32PM +0200, Adam Borowski wrote: > On Fri, Oct 05, 2018 at 09:06:00AM -0700, Darrick J. Wong wrote: > > On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote: > > > Needed by users of fstatfs(). > > > > NAK. > > > > The XFS superblock magic number is part of the on-disk format definition > > of XFS. It belongs with all the other on-disk format definitions in this > > file. > > > > I don't think it's a good idea for userspace to associate s_magic with > > userspace API feature sets, though the cat's long escaped the bag on > > that one. > > > > With that being reality, "the XFS superblock magic number" has a > > different semantic meaning than "the agreed upon statfs.f_type value for > > XFS", which means the latter should have a different symbol name to > > reflect that difference. > > Ie, you'd be ok with the same value to be defined in two places -- do I > understand this right? I'd be fine with it, but let's see what the other XFS maintainers think. I still don't like the practice of inferring behaviors from magic numbers, but at least we'd decouple the disk format from the UABI. :) > > > --- a/fs/xfs/libxfs/xfs_format.h > > > -#define XFS_SB_MAGIC 0x58465342 /* 'XFSB' */ > > > +#define XFS_SB_MAGIC XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */ > > > --- a/include/uapi/linux/magic.h > > > #define JFFS2_SUPER_MAGIC 0x72b6 > > > +#define XFS_SUPER_MAGIC 0x58465342 /* "XFSB" */ > > > #define PSTOREFS_MAGIC 0x6165676C You'd change xfs_fs_fill_super and xfs_fs_statfs too, right? --D > > > Meow! > -- > ⢀⣴⠾⠻⢶⣦⠀ > ⣾⠁⢰⠒⠀⣿⡁ 10 people enter a bar: 1 who understands binary, > ⢿⡄⠘⠷⠚⠋⠀ 1 who doesn't, D who prefer to write it as hex, > ⠈⠳⣄⠀⠀⠀⠀ and 1 who narrowly avoided an off-by-one error.
On Fri, Oct 05, 2018 at 08:10:32PM +0200, Adam Borowski wrote: > On Fri, Oct 05, 2018 at 09:06:00AM -0700, Darrick J. Wong wrote: > > On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote: > > > Needed by users of fstatfs(). > > > > NAK. > > > > The XFS superblock magic number is part of the on-disk format definition > > of XFS. It belongs with all the other on-disk format definitions in this > > file. > > > > I don't think it's a good idea for userspace to associate s_magic with > > userspace API feature sets, though the cat's long escaped the bag on > > that one. > > > > With that being reality, "the XFS superblock magic number" has a > > different semantic meaning than "the agreed upon statfs.f_type value for > > XFS", which means the latter should have a different symbol name to > > reflect that difference. > > Ie, you'd be ok with the same value to be defined in two places -- do I > understand this right? > > > > --- a/fs/xfs/libxfs/xfs_format.h > > > -#define XFS_SB_MAGIC 0x58465342 /* 'XFSB' */ > > > +#define XFS_SB_MAGIC XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */ > > > --- a/include/uapi/linux/magic.h > > > #define JFFS2_SUPER_MAGIC 0x72b6 > > > +#define XFS_SUPER_MAGIC 0x58465342 /* "XFSB" */ > > > #define PSTOREFS_MAGIC 0x6165676C Sorry, hit send too fast. include/uapi/linux/magic.h would get: #define XFS_STATFS_MAGIC 0x58465342 /* 'XFSB' */ fs/xfs/libxfs/xfs_format.h would not be changed at all: #define XFS_SB_MAGIC 0x58465342 /* 'XFSB' */ fs/xfs/xfs_super.c would then be changed to: xfs_fs_statfs(...) { statp->f_type = XFS_STATFS_MAGIC; } xfs_fs_fill_super(...) { sb->s_magic = XFS_STATFS_MAGIC; } We're making two separate promises here: "The on-disk superblock magic is XFS_SB_MAGIC." "When XFS is the filesystem, statfs will return XFS_STATFS_MAGIC." Two completely independent #defines. It just happens to be coincidental that they have the same numeric value. --D > > > Meow! > -- > ⢀⣴⠾⠻⢶⣦⠀ > ⣾⠁⢰⠒⠀⣿⡁ 10 people enter a bar: 1 who understands binary, > ⢿⡄⠘⠷⠚⠋⠀ 1 who doesn't, D who prefer to write it as hex, > ⠈⠳⣄⠀⠀⠀⠀ and 1 who narrowly avoided an off-by-one error.
On Fri, Oct 05, 2018 at 11:23:56AM -0700, Darrick J. Wong wrote: > On Fri, Oct 05, 2018 at 08:10:32PM +0200, Adam Borowski wrote: > > On Fri, Oct 05, 2018 at 09:06:00AM -0700, Darrick J. Wong wrote: > > > On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote: > > > > Needed by users of fstatfs(). > > > > > > NAK. > > > > > > The XFS superblock magic number is part of the on-disk format definition > > > of XFS. It belongs with all the other on-disk format definitions in this > > > file. > > > > > > I don't think it's a good idea for userspace to associate s_magic with > > > userspace API feature sets, though the cat's long escaped the bag on > > > that one. > > > > > > With that being reality, "the XFS superblock magic number" has a > > > different semantic meaning than "the agreed upon statfs.f_type value for > > > XFS", which means the latter should have a different symbol name to > > > reflect that difference. > > > > Ie, you'd be ok with the same value to be defined in two places -- do I > > understand this right? > > > > > > --- a/fs/xfs/libxfs/xfs_format.h > > > > -#define XFS_SB_MAGIC 0x58465342 /* 'XFSB' */ > > > > +#define XFS_SB_MAGIC XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */ > > > > --- a/include/uapi/linux/magic.h > > > > #define JFFS2_SUPER_MAGIC 0x72b6 > > > > +#define XFS_SUPER_MAGIC 0x58465342 /* "XFSB" */ > > > > #define PSTOREFS_MAGIC 0x6165676C > > Sorry, hit send too fast. > > include/uapi/linux/magic.h would get: > > #define XFS_STATFS_MAGIC 0x58465342 /* 'XFSB' */ $ man statfs |grep 0x5846 XFS_SUPER_MAGIC 0x58465342 $ -Dave.
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 059bc44c27e8..837863c57e8c 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -14,6 +14,7 @@ * xfs_da_format.h, which log and log item formats are defined in * xfs_log_format.h. Everything else goes here. */ +#include <linux/magic.h> struct xfs_mount; struct xfs_trans; @@ -26,7 +27,7 @@ struct xfs_ifork; * Fits into a sector-sized buffer at address 0 of each allocation group. * Only the first of these is ever updated except during growfs. */ -#define XFS_SB_MAGIC 0x58465342 /* 'XFSB' */ +#define XFS_SB_MAGIC XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */ #define XFS_SB_VERSION_1 1 /* 5.3, 6.0.1, 6.1 */ #define XFS_SB_VERSION_2 2 /* 6.2 - attributes */ #define XFS_SB_VERSION_3 3 /* 6.2 - new inode version */ diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h index 1a6fee974116..96c24478d8ce 100644 --- a/include/uapi/linux/magic.h +++ b/include/uapi/linux/magic.h @@ -29,6 +29,7 @@ #define HPFS_SUPER_MAGIC 0xf995e849 #define ISOFS_SUPER_MAGIC 0x9660 #define JFFS2_SUPER_MAGIC 0x72b6 +#define XFS_SUPER_MAGIC 0x58465342 /* "XFSB" */ #define PSTOREFS_MAGIC 0x6165676C #define EFIVARFS_MAGIC 0xde5e81e4 #define HOSTFS_SUPER_MAGIC 0x00c0ffee
Needed by users of fstatfs(). Signed-off-by: Adam Borowski <kilobyte@angband.pl> --- fs/xfs/libxfs/xfs_format.h | 3 ++- include/uapi/linux/magic.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)