mkfs.xfs: fix ftype-vs-crc option combination testing
PATCH 1/1 mkfs.xfs: fix ftype-vs-crc option combination testing
mkfs.xfs got weird along the way; today it has different outcomes depending on the order of option specification:
$ mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g cannot specify both crc and ftype $ mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g <succeeds>
Somehow the tests got written as being constrained on what options are specified - and in what order! - vs actually testing for incompatible feature sets.
It's fine to specify both crc & ftype options, as long as it's an allowed combination, so just test for the incompatible combination (crc=1 and ftype=0) after all options have been processed.
---
mkfs/xfs_mkfs.c +4 -10
@@ -1477,11 +1477,6 @@ main(if (c <0 || c >1 )illegal (value,"m crc" ); crcs_enabled = c; -if (nftype && crcs_enabled) { -fprintf (stderr, -_ ("cannot specify both crc and ftype\n" )); -usage (); - }break ;case M_FINOBT :if (!value || *value =='\0' )
@@ -1555,11 +1550,6 @@ _("cannot specify both crc and ftype\n"));if (nftype)respec ('n' , nopts,N_FTYPE ); dirftype =atoi (value); -if (crcs_enabled) { -fprintf (stderr, -_ ("cannot specify both crc and ftype\n" )); -usage (); - } nftype =1 ;break ;default :
@@ -1714,6 +1704,10 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"), XFS_MIN_CRC_BLOCKSIZE);usage (); } +if (crcs_enabled && !dirftype) { +fprintf (stderr,_ ("cannot disable ftype with crcs enabled\n" )); +usage (); + }memset (&ft,0 ,sizeof (ft));get_topology (&xi, &ft, force_overwrite);
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 12 lines · Eric Sandeen
mkfs.xfs got weird along the way; today it has different outcomes depending on the order of option specification:
$ mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g cannot specify both crc and ftype $ mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g <succeeds>Somehow the tests got written as being constrained on what options are specified - and in what order! - vs actually testing for incompatible feature sets.
IIRC, I think this is one of the core problems the big mkfs option parsing rework that Jan is working on is supposed to fix.
Quoted 6 lines
It's fine to specify both crc & ftype options, as long as it's an allowed combination, so just test for the incompatible combination (crc=1 and ftype=0) after all options have been processed.
Signed-off-by Eric Sandeen <[email protected]>---
Seems fine to me...
Quoted 41 lines
mkfs/xfs_mkfs.c +4
-10@@ -1477,11 +1477,6 @@ main(if (c <0 || c >1 )illegal (value,"m crc" ); crcs_enabled = c; -if (nftype && crcs_enabled) { -fprintf (stderr, -_ ("cannot specify both crc and ftype\n" )); -usage (); - }break ;case M_FINOBT :if (!value || *value =='\0' )@@ -1555,11 +1550,6 @@ _("cannot specify both crc and ftype\n"));if (nftype)respec ('n' , nopts,N_FTYPE ); dirftype =atoi (value); -if (crcs_enabled) { -fprintf (stderr, -_ ("cannot specify both crc and ftype\n" )); -usage (); - } nftype =1 ;break ;default :@@ -1714,6 +1704,10 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"), XFS_MIN_CRC_BLOCKSIZE);usage (); } +if (crcs_enabled && !dirftype) { +fprintf (stderr,_ ("cannot disable ftype with crcs enabled\n" )); +usage (); + }memset (&ft,0 ,sizeof (ft));get_topology (&xi, &ft, force_overwrite);
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 9 lines · Brian Foster
Quoted 8 lines · Eric Sandeen
mkfs.xfs got weird along the way; today it has different outcomes depending on the order of option specification:
$ mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g cannot specify both crc and ftype $ mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g <succeeds>
Quoted 7 lines
Quoted 4 lines
Somehow the tests got written as being constrained on what options are specified - and in what order! - vs actually testing for incompatible feature sets.
IIRC, I think this is one of the core problems the big mkfs option parsing rework that Jan is working on is supposed to fix.
Yeah, I think so - Jan, if this gets in your way, let us know - I didn't mean to make your life difficult by fixing little things while you work. :)
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 8 lines · Eric Sandeen
Quoted 4 lines
IIRC, I think this is one of the core problems the big mkfs option parsing rework that Jan is working on is supposed to fix.
Yeah, I think so - Jan, if this gets in your way, let us know - I didn't mean to make your life difficult by fixing little things while you work. :)
Well, I would not mind if the entire codebase froze... :-D
But realistically, every time I do git fetch I get so many collisions that one
more or less changes nothing. :-)
And yes, in my tests I'm trying to cover the arguments order issue too.
Cheers,
Jan
--
Jan Tulak
[email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 6 lines · Jan Tulak
Quoted 5 lines · Eric Sandeen
Quoted 4 lines
IIRC, I think this is one of the core problems the big mkfs option parsing rework that Jan is working on is supposed to fix.
Quoted 8 lines
Quoted 3 lines
Yeah, I think so - Jan, if this gets in your way, let us know - I didn't mean to make your life difficult by fixing little things while you work. :)
Well, I would not mind if the entire codebase froze... :-D
But realistically, every time I do git fetch I get so many collisions that one
more or less changes nothing. :-)
Well, I'm hoping all the big changes getting libxfs up to date with the kernel code are now done, and things will settle down for the next few months so this will be less of a problem. Also having a stable master branch and a moving for-next branch for xfsprogs should help with this, too....
Quoted 1 line
And yes, in my tests I'm trying to cover the arguments order issue too.
The structure of the table-based parsing should make the order of parsing irrelevant, as conflicts are defined in the table and so will be detected regardless of the order in which the options appear on the command line.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 15 lines · Dave Chinner
Quoted 1 line · Jan Tulak
On Fri, Aug 7, 2015 at 6:42 PM, Eric Sandeen <[email protected]>
Quoted 11 lines · wrote:
Quoted 8 lines
Quoted 4 lines
IIRC, I think this is one of the core problems the big mkfs option parsing rework that Jan is working on is supposed to fix.
Yeah, I think so - Jan, if this gets in your way, let us know - I didn't mean to make your life difficult by fixing little things while you work. :)
Well, I would not mind if the entire codebase froze... :-D But realistically, every time I do git fetch I get so many collisions
that
Quoted 9 lines
Quoted 2 lines
one
more or less changes nothing. :-)Well, I'm hoping all the big changes getting libxfs up to date with the kernel code are now done, and things will settle down for the next few months so this will be less of a problem. Also having a stable master branch and a moving for-next branch for xfsprogs should help with this, too...
That would be nice.
Quoted 7 lines
Quoted 1 line
And yes, in my tests I'm trying to cover the arguments order issue too.
The structure of the table-based parsing should make the order of parsing irrelevant, as conflicts are defined in the table and so will be detected regardless of the order in which the options appear on the command line.
However it still has to be tested if it works as supposed. Of course I'm not trying all combinations for all arguments. :-)
Cheers,
Jan
Signature · 2 lines
Jan Tulak [email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 12 lines · Eric Sandeen
mkfs.xfs got weird along the way; today it has different outcomes depending on the order of option specification:
$ mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g cannot specify both crc and ftype $ mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g <succeeds>Somehow the tests got written as being constrained on what options are specified - and in what order! - vs actually testing for incompatible feature sets.
Quoted 5 lines
It's fine to specify both crc & ftype options, as long as it's an allowed combination, so just test for the incompatible combination (crc=1 and ftype=0) after all options have been processed.
Signed-off-by Eric Sandeen <[email protected]>
Did you test this, Eric?
$ mkfs.xfs /dev/sdc cannot disable ftype with crcs enabled Usage: mkfs.xfs /* blocksize */ [-b log=n|size=num] .....
Quoted 31 lines
---
mkfs/xfs_mkfs.c +0
-10@@ -1477,11 +1477,6 @@ main(if (c <0 || c >1 )illegal (value,"m crc" ); crcs_enabled = c; -if (nftype && crcs_enabled) { -fprintf (stderr, -_ ("cannot specify both crc and ftype\n" )); -usage (); - }break ;case M_FINOBT :if (!value || *value =='\0' )@@ -1555,11 +1550,6 @@ _("cannot specify both crc and ftype\n"));if (nftype)respec ('n' , nopts,N_FTYPE ); dirftype =atoi (value); -if (crcs_enabled) { -fprintf (stderr, -_ ("cannot specify both crc and ftype\n" )); -usage (); - } nftype =1 ;break ;default :
mkfs/xfs_mkfs.c _("Minimum block size for CRC enabled filesystems is %d bytes.\n"), · line 1710
Quoted 7 lines
mkfs/xfs_mkfs.c +4
-0@@ -1714,6 +1704,10 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"), XFS_MIN_CRC_BLOCKSIZE);usage (); } +if (crcs_enabled && !dirftype) { +fprintf (stderr,_ ("cannot disable ftype with crcs enabled\n" )); +usage (); + }
nftype = dirftype = 0; /* inode type information in the dir */
So we enable crcs by default and do not enable dirftype/nftype by default, and so by default mkfs now fails. I'll fix it to always default to ftype=1 for both v4 and v5 filesystems as there is no go reason not to enable it.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 13 lines · Dave Chinner
Quoted 12 lines · Eric Sandeen
mkfs.xfs got weird along the way; today it has different outcomes depending on the order of option specification:
$ mkfs/mkfs.xfs -n ftype=1 -m crc=0 -dfile,name=fsfile,size=16g cannot specify both crc and ftype $ mkfs/mkfs.xfs -m crc=0 -n ftype=1 -dfile,name=fsfile,size=16g <succeeds>Somehow the tests got written as being constrained on what options are specified - and in what order! - vs actually testing for incompatible feature sets.
Quoted 7 lines
Quoted 5 lines
It's fine to specify both crc & ftype options, as long as it's an allowed combination, so just test for the incompatible combination (crc=1 and ftype=0) after all options have been processed.
Signed-off-by Eric Sandeen <[email protected]>Did you test this, Eric?
Yep...
Quoted 5 lines
$ mkfs.xfs /dev/sdc cannot disable ftype with crcs enabled Usage: mkfs.xfs /* blocksize */ [-b log=n|size=num] .....
God, but not with defaults! I tested w/ all combinations of ftype/crc specified. :(
...
Quoted 4 lines
So we enable crcs by default and do not enable dirftype/nftype by default, and so by default mkfs now fails. I'll fix it to always default to ftype=1 for both v4 and v5 filesystems as there is no go reason not to enable it.
Hell. Sorry about that. Thanks for catching ...
Thanks,
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs