Re: [PATCH] common/config: Fix use of MKFS_OPTIONS

["Theodore Ts'o" <tytso@mit.edu>]

On Fri, Jun 24, 2022 at 12:08:25AM +0800, An Long wrote:
>  
> +# check the size value in $MKFS_OPTIONS is an integer
> +if [[ $MKFS_OPTIONS =~ [0-9]+[^0-9\ ] ]] ; then
> +        _fatal "error: \$MKFS_OPTIONS: Only number in bytes is allowed, no units."
> +fi

This will break configs, since ext4 has a feature 64bit, so there
might be MKFS_OPTIONS that contain "-O 64bit", or "-O ^64bit".

For that matter, this wll also break things like "-E encoding=12.1"
which sets the Unicode encoding to 12.1.

... or "-E lazy_itable_init=1,lazy_journal_init=1".

Bottom line, blindly assuming that any number followed by a non-number
is a unit, is just not going to work.

I think the real problem is that we have fstests trying to parse
MKFS_OPTIONS at all in the first place.  For that matter, I'm not fond
of tests that try to override MKFS_OPTIONS for their own nefarious
purposes, because sometimes certain MKFS_OPTIONS imply something about
what must be in MOUNT_OPTIONS, or vice versa.  I do understand why
that has to be done for certain tests, but... yelch.  Especially in
centralized functions like _scratch_mkfs_sized, it just leads to tech
debt.

I'll note that generic tests and functions in common/rc that do unholy
things with MKFS_OPTIONS already have to have per-file system hacks as
it is.  So having more per-file system hacks for check_block_options()
is par for the course, so long as we accept that we want to have
generic tests and generic code violate the abstraction barrier and try
to read or override MKFS_OPTIONS.

If we want to go down that approach, perhaps another approach would be
to specify the desired blocksize diretly in the fs config.  Something
like "FS_BLOCK_SIZE=1024"?  We can then have the fs-specific mkfs
commands translate that into the appropriate "-b 1024" or whatever
else might be appropriate for a particular file system.  And then
tests that want to override the block size can just override
FS_BLOCK_SIZE and then the _mkfs function can use it as appropriate.

(BTW, this has been less important for ext4, since we allow the block
size to be specified more than once, with the last option being the
one which controls.  So "mkfs.ext4 -b 4096 ... -b 1024 ..." is OK for
ext4, but it results in an error for mkfs.xfs.  Otherwise we could
probably solve this problem more easily.)

I know my proposed approach will require all of the fstests wrapers to
be changed, and may require auditing a lot of tests to see what
breaks.  But quite frankly, what we have write now with tests messing
with MKFS_OPTIONS is kind of a mess already, and if we're going to do
clean up the tech debt, it's going to requrie some pain.

Or we could use the suggestion of more per-fs case in a
_check_mkfs_options function.  Which is a bit ugly, but IMHO, not as
ugly as the existing hacks where we have generic code trying to mess
with MKFS_OPTIONS....

						- Ted