On Thu, Aug 24, 2017 at 09:15:02PM +0200, Luis R. Rodriguez wrote: > On Sun, Aug 20, 2017 at 11:56:24AM +1000, Dave Chinner wrote: > > Let me finish factoring all the code into a set of usable > > structures, then we can start looking at the option table and config > > files from a fresh perspective. This time, we need to come up with a > > clean design and clear direction before any patches are written... > > While you're on it, I should mention I ran across a nice feature as part of one > of the libraries I evaluated when considering configuration file parsing. The > library libini_config has a rules checker which allows one to build rules from > yet-another file, it uses ini_rules_read_from_file() to let you pass your > rules, you then use ini_rules_check() to check if the configuration file > applies to rules previously loaded. An example INI file for rules is that > of the upstream SSSD: > > https://github.com/SSSD/sssd/blob/master/src/config/cfg_rules.ini > > I haven't yet dove too much into this but from what I gather the > validator can be a built-in one or a local one. > > A curious thing about our use is we'd want a validator for both configuration > file and command line parameters. But such language above may allow us to > easily specify and handle both. > > Food for thought as we're trying to come up with a clean design now. Conflating config files to change defaults with command line parsing is what got us into this mess in the first place. It's not a clean design - it's like trying to fit a square peg into a round hole. The result is stupidly complicated and needlessly bleeds config file implementation all through the rest of the mkfs code. A clean design isolates the different functionality into self-contained modules and connects them with simple, easy to understand structures and minimal APIs. The default behaviours we can modify are a much smaller subset of options than the CLI can modify. Hence if we define a structure that contains all the default options we can set, we also define the required config file contents. And it also defines the interface between built in defaults, config file defaults and how that feeds into command line parsing. The command line parsing can remain entirely unchanged from where it is now, except for the structure it stores the configured options in. And, finally, we have a set of validated and configured values that mkfs uses to create the on-disk structures - the mkfs configuration if you like. This is currently splayed across single variables used to hold the defaults, cli options and validated mkfs values at different points in time. So we need to separate these out into a set of validated "mkfs parameters" structure that we can feed to the on disk formatting. IOWs, we have 4 clear modules and a set of data that is needed as inputs into each module: Module Connecting structure Setting defaults struct mkfs_default_params CLI parsing struct cli_params Validation + calculation struct mkfs_params On disk formatting Note that there's nothing here that dictates how each module is implemented - all I've done is define modules and the data that needs to flow between the modules. I've simply applied a basic software engineering technique (SADT - structured analysis and design technique) and made a ruidmentary data flow diagram in my head to get to this. We have a nasty habit of engineering software around here, and a key engineering process is understanding why something we tried to build failed catastrophically. And once a failure has been understood, we put in place processes that make sure that mistake doesn't happen again. The last 3 modules in the above list were what I originally intended to do with mkfs - the CLI options table was simply an implementation detail inside the CLI parsing module. Then we got feature creep in the middle of all this - config file based defaults, json config input/output, etc. We tried to factor that into the *CLI parsing module implementation*, rather than stepping back and looking at the overall design. That's where it all went wrong and that's the reason we need to reset: we made a mistake at the design level and no amount of implementation complexity was going to fix that. We failed and had to reset here because I didn't sufficiently communicate the original goals, design, etc, to the people who took up the work. And to make matters worse, I then allowed feature creep to take hold without really considering how those features should fit into the bigger picture. A year ago I was pretty much beyond caring about this work because I was >this< close to burning out so I just let it all go. That's not a problem for me anymore, so now I'm cleaning up the mess I let happen on my watch. And that means I need to make sure everyone understands: 1. the reasons we have to reset this work, 2. the design of the new code and the reasons for it; and 3. make damn sure that feature creep doesn't derail this work again So, once the refactoring is done and I've added the stubs to allow config files to be used to override build time defaults, we can bikeshed about the config file format used in the "setting defaults" module all we want because it won't affect the design or implementation of any other part of mkfs. This is a lot longer and a *lot* stronger than I was originally intending to write, but it comes down to one fundamental principle: Never make the same mistake twice. 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
