Re: [PATCH] mkfs: rename defaultval to flagval in opts

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com