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@xxxxxxxxxxxxx> 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@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
