On Wed, Jun 13, 2018 at 11:29:49PM -0700, Darrick J. Wong wrote: > On Thu, Jun 14, 2018 at 03:08:44PM +1000, Dave Chinner wrote: > > On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote: > > > On 6/13/18 11:06 PM, Dave Chinner wrote: > > > > With this change, we'll have code to maintain it to ensure that the > > > > file gets updated properly, and it will probably take more time and > > > > effort to validate that the generated file is correct (and debug if > > > > it's not!) compared to the 30s it will take to hand edit the > > > > template file to change or add a new default... If we had an xfstest to *test* that same generated file, this would not be an issue and from what I gather we need quite a bit of work to get there. I'm working on a test for config stuff but that will just test for now (and this reveals some future work needed): a) a set of config files we know should work and ensure they produce the same filesystem as if we had used CLI params. We can use xfs_db -c version against both filesystems and check that each differences. Since this would use the same xfsprogs for the results of a config based filesystem and the CLI based filesystem the diff would only generate if there really was a change between both runs, and you can use any xfsprogs version for it. It does however leave actual expected results on the filesystem up to a separate test, it assumes that xfs/191-input-validation is doing its job, but word is that needs some love. b) a set of invalid config files and ensure they never work c) test to ensure cli can override config params To test that a self generated config file works would be a next step (d), and I think we can validate it by also making sure it yields the same filesystem as if we just ran mkfs.xfs with no options, so same strategy as in a). > Counterpoint: the mkfs config file template in mkfs.xfs.8 is already > incorrect; sparse inodes was enabled by default in 4.16 but the manpage > cfg file says it isn't. This is exactly what happens when we rely on > ourselves to hand edit the template file. > > Put the defaults in *one* place in the source code tree and generate > everything else off of that, instead of keeping multiple copies that > will then get out of sync. We clearly suck at this maintenance. I personally prefer this automation, so with this is mind I think this is a change in the right direction. > > > Well, yeah. /if/ we need a template, as well as text in a man page, > > > then this gets it down to editing 1 file instead of two, I guess. > > > But it really seems like we need to rethink these structures to get it > > > all tied together, not requiring 2 or 3 manual updates across several files. > > > It's bound to get out of sync. I guess that can wait, but right now this > > > dispersal isn't good. > > > > So let's get the basic conig file stuff in first, then cosolidate, > > then add all the bells and whistles. > > That's where I started from -- fix the existing mess with the manpage, That should be a separate patch first. > then fix the problems in the parsing code, then move all the CLI parsing and > validation code out of xfs_mkfs.c because it's just too long. Yay! > Granted, I haven't gotten all the way there yet, so perhaps it was a bad > idea to post just the first of those three parts. > > > Too many cooks trying to add all their own bells and whistles before > > the core behaviour, infrastructure and implementation was nailed > > down was pretty much what lead all the tablised CLI option parsing > > code. And we're doing it again with this config file stuff... > > Yeah. TBH I feel at a loss for what exactly your vision is for how mkfs > option parsing is supposed to work in the end. How close is this: BTW it seems we may need to document this vision as otherwise others may run into issues later as well. > 1. First we have the mkfs_default_params, which are the builtin > defaults. > > 2. Configuration file parameters are supposed to change the > mkfs_default_params, yes? > > 3. The mkfs_default_params are used to seed the cli_params. > > 4. The getopt loop pulls the CLI option data into the cli_params. > > 5. At this point we've melded together the builtin options > with whatever the config file told us about, then folded in the CLI > options, right? > > 6. Now we start calling the validate_* and calculate_* functions. These > functions check the full set of configuration data and copy them into > the mkfs_params; and they calculate whatever other bits are needed to > finish the mkfs_params. > > 7. Finally the validated mkfs_params are fed to the initialisation > functions to set up the actual filesystem. FWIW this at least matches my own expectations so far. > So AFAICT what we have now is a big collection of ops_params that > contain all the cli parsing options and a bunch of functions that take > the cli strings and fill out the appropriate parts of cli_params. > > Luis built a confopts_tab(le) that mapped cfgfile options to parsing > functions that know how to take the cfgfile strings and fill out the > appropriate parts of mkfs_default_params. I understand now that from > these first five patches it sure looks like I'm building yet a third > structure, but this isn't the endgame -- I want a data structure that > maps cfgfile options directly to the mkfs default params. I'm doing > this by building the new direct-mapped structure on the side and > migrating the parser code to use that. > > Ås for "how do I check that the distro provided default template even > works?", I'm working on building an xfstest to make sure that the > default templates we ship (and whatever might be in > /etc/xfs/mkfs/default) actually produce a functioning filesystem. With proper testing in place for regular configs stuff (the tests I have to write) and this I see your changes as desirable but I'm wondering if this should just wait until the next cycle so we at least have some effort on the testing started already? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
