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

[Dave Chinner <david@fromorbit.com>]

On Sat, Aug 19, 2017 at 08:40:57AM +0200, Jan Tulak wrote:
> On Sat, Aug 19, 2017 at 3:03 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Fri, Aug 18, 2017 at 12:39:32PM +0200, Jan Tulak wrote:
> >> The old name 'defaultval' was misleading - it is not the default value,
> >> but the value the option has when used as a flag by an user.
> >
> > Hmmm - ok, what we have here is the difference between design intent
> > and the current use of the field.
> >
> > The design intent is that the defaultval field can contain the
> > default value for any type of config field. It gets used when a user
> > either doesn't specify the option or doesn't specify a value for the
> > option that is being parsed. The special "need value" value tells
> 
> These are two things that can't be done in one field.

Drop the "doesn't specify the option" part of what I said - it's so
long ago and I've been out of the loop for more than half a year
and I've conflated several different things into one statement
and ended up implying something I shouldn't have.

e.g. I found an note in an old hacked up prototype patch I'd written
and discarded way back in 2013, and it said:

.....
	*
	* If a value is not supplied with an option, the option table entry
	* for that CLI option will tell the parser whether a value is
	* required. If a value is not require, it will contain the default
	* value that should be used for that CLI option.
	*
.....

The "store all defaults in the table" came later - IIRC (*cough*) it
came about from wanting to support config files which needed a store
for all the mkfs default values so they could be overriden by both
config file and CLI options.

As it is, since I originally started this options table rework, we
now store most mkfs defaults in a separate structure:

/*
 * Default values for superblock features
 */
struct sb_feat_args     sb_feat = {
        .finobt = 1,
        .spinodes = 0,
        .log_version = 2,
        .attr_version = 2,
        .dir_version = XFS_DFL_DIR_VERSION,
        .inode_align = XFS_IFLAG_ALIGN,
        .nci = false,
        .lazy_sb_counters = true,
        .projid16bit = false,
        .crcs_enabled = true,
        .dirftype = true,
        .parent_pointers = false,
        .rmapbt = false,
        .reflink = false,
};

Or we calculate them from underlying geometry. Hence the need to
store "if not specified" mkfs defaults in the option table doesn't
exist. It didn't exist years ago when I started on this, it doesn't
exist now and AFAICT it doesn't need to exist in the future to
support config files.

> If it worked
> that way, imagine what would happen with a flag that is disabled by
> default, like -m rmapbt? If you put here the default value for
> "disabled when not specified", you can't enable it with a flag. If you
> put here a value for "enable when used as a flag", you have just
> enabled it by default as well.

The parser and table design is supposed to be flexible enough to
give you enough rope to hang yourself. :)

[....]

> > the code that there isn't a defined default that can be used, so the
> > option must be specified with a value.
> >
> > The current implementation only contains default values for flag
> > fields, but that doesn't mean we can't use it for fields that are
> > not flags. And if that's the case, then renaming the field "flagval"
> > isn't the right thing to do....
> 
> The field is used only when no value is provided, but the option is
> specified --> when the option is used as a flag. So "flagval" sounds
> ok to me.

Again, you're using the implementation details to define a limit
the scope of a variable that was designed to have a very wide
scope and flexible usage.

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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com