[PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[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.

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 <sandeen@redhat.com>
---

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 69d61c7..9042796 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[Brian Foster]

On Thu, Aug 06, 2015 at 10:18:50PM -0700, Eric Sandeen wrote:
> 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.

> 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 <sandeen@redhat.com>
> ---

Seems fine to me...

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 69d61c7..9042796 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[Eric Sandeen]

On 8/7/15 4:37 AM, Brian Foster wrote:
> On Thu, Aug 06, 2015 at 10:18:50PM -0700, Eric Sandeen wrote:
>> 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.

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[Jan Tulak]

[-- Attachment #1.1: Type: text/plain, Size: 662 bytes --]

On Fri, Aug 7, 2015 at 6:42 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>
> >
> > 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
jtulak@redhat.com

[-- Attachment #1.2: Type: text/html, Size: 932 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

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

Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[Dave Chinner]

On Thu, Aug 13, 2015 at 01:14:25PM +0200, Jan Tulak wrote:
> On Fri, Aug 7, 2015 at 6:42 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> >
> > >
> > > 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. :-)

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....

> 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.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[Jan Tulak]

[-- Attachment #1.1: Type: text/plain, Size: 1549 bytes --]

On Fri, Aug 14, 2015 at 3:57 AM, Dave Chinner <david@fromorbit.com> wrote:

> On Thu, Aug 13, 2015 at 01:14:25PM +0200, Jan Tulak wrote:
> > On Fri, Aug 7, 2015 at 6:42 PM, Eric Sandeen <sandeen@sandeen.net>
> wrote:
> > >
> > > >
> > > > 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. :-)
>
> 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.


> > 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

-- 
Jan Tulak
jtulak@redhat.com

[-- Attachment #1.2: Type: text/html, Size: 2469 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

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

Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[Dave Chinner]

On Thu, Aug 06, 2015 at 10:18:50PM -0700, Eric Sandeen wrote:
> 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.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Did you test this, Eric?

$  mkfs.xfs /dev/sdc
cannot disable ftype with crcs enabled
Usage: mkfs.xfs
/* blocksize */         [-b log=n|size=num]
.....

> ---
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 69d61c7..9042796 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -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();
> +	}

        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.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] mkfs.xfs: fix ftype-vs-crc option combination testing

[Eric Sandeen]

On 8/18/15 1:53 AM, Dave Chinner wrote:
> On Thu, Aug 06, 2015 at 10:18:50PM -0700, Eric Sandeen wrote:
>> 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.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Did you test this, Eric?

Yep...

> $  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.  :(

...

> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs