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
