Re: [PATCH 0/9] mkfs.xfs: add mkfs.xfs.conf support

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Thu, May 11, 2017 at 4:00 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Fri, May 12, 2017 at 12:46:03AM +0200, Luis R. Rodriguez wrote:
>> On Mon, Apr 24, 2017 at 01:25:03AM -0700, Luis R. Rodriguez wrote:
>> > On Mon, Apr 24, 2017 at 12:26 AM, Jan Tulak <jtulak@redhat.com> wrote:
>> > > On Mon, Apr 24, 2017 at 7:00 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > >> On Fri, Mar 10, 2017 at 09:34:35AM +1100, Dave Chinner wrote:
>> > >>> On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote:
>> > >>> > I used reset_opt() and went with "last entry specified wins". From my
>> > >>> > review the goal of the respecification was to ensure each opt param
>> > >>> > parsed would not reset a prior set param, a paranoid measure, however
>> > >>> > this clearly does not work well if we want to allow for "last entry
>> > >>> > specified wins", or re-use the validators for a config file parsing
>> > >>> > for a first shot a parsing entries.
>> > >>>
>> > >>> Which is essentially broken, because doing something like:
>> > >>>
>> > >>> -m crc =1 -m reflink=1 -m crc=0
>> > >>>
>> > >>> leaves you with an /invalid config/ because of the respecification
>> > >>> of -m crc=0 and the order in which options are parsed and verified.
>> > >>>
>> > >>> Indeed, things like block and sector sizes are particularly nasty in
>> > >>> this respect, because other options can be specified in block or
>> > >>> sector units. SO things like:
>> > >>>
>> > >>> -s size=4k -b size=1s -s size=512 -d size=1000000s
>> > >>>
>> > >>> were considered valid. respecification of options like this is just
>> > >>> borken, and even if we take "last specification wins" it still means
>> > >>> that the block size specification is ambiguous and potentially
>> > >>> incorrect depending on other options. Hence respecification of
>> > >>> options is simply not allowed and post-processing of the options
>> > >>> doesn't change that.
>> > >>
>> > >> We have to pick an approach and stick with, the above seems sensible.
>> > >>
>> > >>> i.e., the biggest issue with reusing the existing parsing code for
>> > >>> the "default config" is that is doesn't just set default values - it
>> > >>> prevents other options from being used.
>> > >>
>> > >> Right as per original design.
>> > >>
>> > >>> IOWs, the config file should
>> > >>> set the default values in the option table, not set the options
>> > >>> directly as happens on the command line....
>> > >>
>> > >> As I respin my patches addressing concerns an issue I see with this is current
>> > >> semantics for "defaultval" is not that they will be the defaults, but rather
>> > >> they will be the defaults *iff* the user did specify the option on the command
>> > >> line but did not provide an explicit value. This for example would not allow
>> > >
>> > > Yes, a confusing name. I'm aware of it and have a fix to rename it to .flagval.
>> > > It was part of my big set before and now I moved it into the smaller
>> > > set I submitted
>> > > on this Sunday. The same set adds a new field .value, which can be used to
>> > > specify default as in "if the user does not specify this option at all".
>> >
>> > Terrific, thanks will use that.
>>
>> FWIW, I've looked at ways to address this without your future work Jan, ie
>> backporting this feature, and ultimately have decided to *not* allow any
>> command line overwrite for options specified in the configuration file. So
>> for the backported versions of this feature a user will only be able to
>> overwrite if the config file is commented out or removed.
>
> Waitaminute, config file directives lock out command line options??

Not for upstream, IMHO command line should always be able to overwrite
a config file as is traditionally done.

> I suppose that would make sense if one of the config file options was:
>
> disable_overrides = true
>
> ...but I think you're talking about /never/ allowing overrides, right?

Not for upstream, the question is how to backport this feature without
Jan's big replacement of the kitchen sink, and bathroom decor going
on.

> I am under the impression that we have (a) mkfs defaults in the source
> code that can be overridden in

Not clearly, the variables on the struct are actually "defaults if the
user supplies an argument but does not specify a value"! Without an
command line argument specified its actually a bit tricky to decipher
what is the default option used for a parameter.

With Jan's work this should be clearer later upstream, buts coming in
after the bathroom decor gets a facelift. Without Jan's work this is a
code mystery.

> (b) the config file

Only once the bathroom decor goes in.

>  which in turn can be overridden by the administrator via (c) the command line.

Sure, that should be the case upstream later. Modulo I think we some
additional checks typically done today only after main() should be
done after each (a), (b) and (c).

> In other words, we (upstream) set whatever defaults we think are sane,
> then distros can set the defaults they want to support, and the admin
> can change things as they see fit for their site.

Right, upstream-wise I agree.

> If the administrator
> wants to use non-default settings, they're welcome to support that
> themselves (or hire someone to do it for them).  We don't prohibit that.

Sure.

> The way I describe is the way that mke2fs works, and afaict most other
> programs operate that way too.  mkfs.xfs has a long history of "things
> you put on the command line are what you get in the fs", and changing it
> now is breaking peoples' mental models of how mkfs.xfs works, and in a
> way that runs counter to most other programs.

Sure, we don't want to change that.

 Luis