Re: [PATCH 01/17] xfsprogs: use common code for multi-disk detection

[Dave Chinner <david@fromorbit.com>]

[Please line wrap your responses at 68-72 columns]

On Wed, Jul 08, 2015 at 12:14:56PM -0400, Jan Tulak wrote:
> ----- Original Message -----
> > From: "Dave Chinner" <david@fromorbit.com>
> 
> > At one point during development of this patch set I started writing
> > an xfstest to validate that mkfs did all the right input validation
> > things and set parameters appropriately so that we didn't
> > inadvertently change behaviour. I never really finished it off (like
> > the patch set), but I've attached it below to give an idea of where
> > I was going with it. It was based on validating the input and CLI
> > parameters for the new code, so is guaranteed to fail on an existing
> > mkfs binary.
> 
> I'm using and extending it, but I'm not sure about some tests,
> whether it is a change from current behaviour, or if it is rather
> an issue in the test.

Remember, the point of the patchset was to sanitise and clean up the
CLI interface, not just the code. i.e. the CLI will change, not just
the code.

> > +
> > +# basic "should fail" options
> > +# logarithm based options are no longer valid
> > +do_mkfs_fail -s log=9 $SCRATCH_DEV
> 
> There are some changes in logarithm input (mkfs: validate
> logarithmic parameters sanely), but it is still supported in the
> patches. Is there some issue, why to remove them?

They are redundant and almost nobody uses them. The size options are
what people use, and even they have so many different units that it
confuses people...

> Otherwise, it should rather test for (in)valid input for log=xxx, right?

No, it's indicative of the fact I wanted to remove the log scale
options for variables.  As I've said before - I didn't ever finish
the patchset off.

Essentially, once all the options are in a table, we only want to
look in one place for things like ag size, fs size, log size, block
sizes etc. Right now the table has multiple entries or multiple
global variables for these things and we have to work out which is
valid and correct and handle conflicts and incompatibilities, etc.
We don't *need* that complexity to specify sizes of things, so
getting rid of the rarely used redundant options makes a lot of
sense as it simplifies the code and the user interface without
reducing functionality...

> > +rm -f $fsimg
> > +$XFS_IO_PROG -f -c "truncate $fssize" $fsimg
> > +do_mkfs_pass -d file $fsimg
> > +do_mkfs_pass -d file,name=$fsimg
> > +rm -f $fsimg
> > +do_mkfs_pass -d size=$fssize,file $fsimg
> > +rm -f $fsimg
> > +do_mkfs_pass -d size=$fssize,file,name=$fsimg
> > +do_mkfs_pass -d file,name=$fsimg
> 
> Should all these inputs really pass?

Yes, they should, because ....

> What is the expected
> behaviour for example on -d file,name=$fsimg if the file exists,
> and what if there is no such file? 

....in all cases the file either:

	a) exists and is of non-zero size and hence defines the
	   size of the filesystem to be created,
	b) does not exist but the size of the filesystem to be
	   created is specified on the CLI allowing us to create
	   the image file correctly.

We should only fail to create the image file if it doesn't exist
and we haven't been given enough information to calculate the size
of the filesystem via CLI parameters.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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