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... 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. > > 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, 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. 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: 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. 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. > > > I'm also not convinced we should ship a "default.template" file, > > > either. I'd much prefer we ship a real config file with all the > > > options defined but commented out as a "start here template". > > > > As the code stands today, if a 100% commented-out config file > > exists, then mkfs will tell you that it's overriding built-in > > defaults with the config file which contains ... nothing. > > If we ship such a thing by default, that's what will happen by > > default. > > Then that's a bug in the new parsing code and needs fixing. :) > > > (Actually today it fails to parse and errors out but a patch > > to fix that is on the list). > > > > I guess we could behave as if "no config file" if a config file > > contained no parseable tokens ... but that gets weird too. > > Don't see why that is wierd, either - it's pretty common behaviour > for package shipped config files to have all options are present but > commented out. Agreed, but I think Eric wanted to be cautious and not have mkfs parsing the default config file until this settles a little more, so I designed it to spit out a default.template file. For now users will continue to use the same old code paths unless they explicitly light up the new ones by providing /etc/xfs/mkfs/default or -c on their own. --D > > Putting a blank config file in place to be parsed by default > > is messy, so a .template approach seems reasonable to me, if we > > need such a thing at all. > > If we really need a separate template, then perhaps it should be in > the mkfs config file man page where we document all the supported > options? > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx > -- > 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 -- 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
