| Message ID | 20170818103932.24607-1-jtulak@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 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.... Cheers, Dave.
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. 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. So this field ended up being used only when the option is used without a value, and my current set adds the "default when the option is not specified" field with another name, and for the sake of clarity, I renamed this one. And this name already caused an issue - when rmapbt was added, the person went through the "it's a default" line of thoughts, set it to 0 and then it was not possible to enable it with a flag. -m rmapbt was equivalent to -m rmapbt=0, and the only way to enable it was an explicit -m rmapbt=1. At which point, we can't speak about flags. > 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. Cheers, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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. > > So this field ended up being used only when the option is used without > a value, and my current set adds the "default when the option is not > specified" field with another name, and for the sake of clarity, I > renamed this one. > > And this name already caused an issue - when rmapbt was added, the > person went through the "it's a default" line of thoughts, set it to 0 > and then it was not possible to enable it with a flag. -m rmapbt was > equivalent to -m rmapbt=0, and the only way to enable it was an > explicit -m rmapbt=1. At which point, we can't speak about flags. As "that person", I confirm that I set defaultval to zero for rmapbt thinking that meant "off by default if no options given", not "if the user specifies -m rmapbt without explicitly setting a value, then set the value to zero". It doesn't make sense that adding "-m rmapbt" actually has the effect of /disabling/ rmap. That said, I also don't remember if one of the side goals of cramming all the config option data into the huge struct is to eliminate the difference between the default value of an option if "" vs. if "-m rmapbt". > > 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. <bikeshedding> default_flagval? :D --D > > Cheers, > Jan > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
On Sun, Aug 20, 2017 at 3:56 AM, Dave Chinner <david@fromorbit.com> wrote: > 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. All right, that makes sense. > >> 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. :) > Except the previous description was not giving you a rope, it was directly throwing a loop around your neck. :-) Now, with the "doesn't specify the option" part removed, this comment is out of date. > [....] > >> > 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... > Agreed. Cheers, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
On Fri, Aug 25, 2017 at 09:58:43AM +1000, Dave Chinner wrote: > 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. I'm not connecting defaults with CLI. I realize these are separate. The above relationship was for CLI and configuration file given there is a 1-1 mapping. If the configuration file was not to allow to specify each and every CLI argument, then surely there was a disconnect from the start. I think breaking things down into modules as you did below however is important and agree that it could be split up nicely. > 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. I see, so the configuration file purpose is to be able to override the smaller set of defaults, not supplement the CLI with all the bells and whistles the CLI allows. I'll yield to you for this, it sounds reasonable to me, however I suppose I should point out that at least ext[2-4] do allow for CLI options, so its not clear to me what that should implicate about mke2fs.conf. > 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. Sure. This was expected. > 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. Great! > 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. Thanks, this helps. FWIW the way I split things out was CLI ------------ \ ---> params / config file --- It would seem from what you are saying CLI ------------ \ ---> struct mkfs_default_params / config file --- Is that right? > 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. Alrighty. > 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. The details helped, thanks. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 25, 2017 at 02:39:18AM +0200, Luis R. Rodriguez wrote: > On Fri, Aug 25, 2017 at 09:58:43AM +1000, Dave Chinner wrote: > > 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. > > I see, so the configuration file purpose is to be able to override > the smaller set of defaults, not supplement the CLI with all the > bells and whistles the CLI allows. I'll yield to you for this, it > sounds reasonable to me, however I suppose I should point out that > at least ext[2-4] do allow for CLI options, so its not clear to me > what that should implicate about mke2fs.conf. I don't really care what mke2fs does with it's config file - it's got different constraints and behaviours, and likely different ways of setting defaults. If they have to set them via using CLI options in the confg file, then that's what they thought best. I'm working on the model that if something can have a sane default defined, then it should be a known key/value pair in the config file which has a matching entry in the struct mkfs_default_params that is then used in the appropriate place in the code to apply that default. Using CLI option syntax for this just makes the config file format more complex and parsing it harder for no good reason. > > 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. > > Thanks, this helps. FWIW the way I split things out was > > CLI ------------ > \ > ---> params > / > config file --- > > It would seem from what you are saying > > > CLI ------------ > \ > ---> struct mkfs_default_params > / > config file --- > > Is that right? No. It's: Build defaults --\ ---> mkfs_default_params -> CLI -> mkfs_params config file -----/ Cheers, Dave.
On Tue, Aug 29, 2017 at 08:36:53AM +1000, Dave Chinner wrote: > On Fri, Aug 25, 2017 at 02:39:18AM +0200, Luis R. Rodriguez wrote: > > On Fri, Aug 25, 2017 at 09:58:43AM +1000, Dave Chinner wrote: > > > 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. > > > > I see, so the configuration file purpose is to be able to override > > the smaller set of defaults, not supplement the CLI with all the > > bells and whistles the CLI allows. I'll yield to you for this, it > > sounds reasonable to me, however I suppose I should point out that > > at least ext[2-4] do allow for CLI options, so its not clear to me > > what that should implicate about mke2fs.conf. > > I don't really care what mke2fs does with it's config file - it's > got different constraints and behaviours, and likely different ways > of setting defaults. If they have to set them via using CLI options > in the confg file, then that's what they thought best. > > I'm working on the model that if something can have a sane default > defined, then it should be a known key/value pair in the config > file which has a matching entry in the struct mkfs_default_params > that is then used in the appropriate place in the code to apply that > default. Using CLI option syntax for this just makes the config file > format more complex and parsing it harder for no good reason. Sounds good to me! > > > 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. > > > > Thanks, this helps. FWIW the way I split things out was > > > > CLI ------------ > > \ > > ---> params > > / > > config file --- > > > > It would seem from what you are saying > > > > > > CLI ------------ > > \ > > ---> struct mkfs_default_params > > / > > config file --- > > > > Is that right? > > No. It's: > > Build defaults --\ > ---> mkfs_default_params -> CLI -> mkfs_params > config file -----/ Perfect, thanks for the clarification. Full steam ahead! Luis -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 7bb6408f..091ca530 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -101,7 +101,7 @@ unsigned int sectorsize; * to zero. But if one value is different: minval=0 and maxval=1, * then it is OK.) * - * defaultval MANDATORY + * flagval MANDATORY * The value used if user specifies the subopt, but no value. * If the subopt accepts some values (-d file=[1|0]), then this * sets what is used with simple specifying the subopt (-d file). @@ -121,7 +121,7 @@ struct opt_params { int conflicts[MAX_CONFLICTS]; long long minval; long long maxval; - long long defaultval; + long long flagval; } subopt_params[MAX_SUBOPTS]; }; @@ -140,7 +140,7 @@ struct opt_params bopts = { LAST_CONFLICT }, .minval = XFS_MIN_BLOCKSIZE_LOG, .maxval = XFS_MAX_BLOCKSIZE_LOG, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = B_SIZE, .convert = true, @@ -149,7 +149,7 @@ struct opt_params bopts = { LAST_CONFLICT }, .minval = XFS_MIN_BLOCKSIZE, .maxval = XFS_MAX_BLOCKSIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, }, }; @@ -195,24 +195,24 @@ struct opt_params dopts = { LAST_CONFLICT }, .minval = 1, .maxval = XFS_MAX_AGNUMBER, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_FILE, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = D_NAME, .conflicts = { LAST_CONFLICT }, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_SIZE, .conflicts = { LAST_CONFLICT }, .convert = true, .minval = XFS_AG_MIN_BYTES, .maxval = LLONG_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_SUNIT, .conflicts = { D_NOALIGN, @@ -221,7 +221,7 @@ struct opt_params dopts = { LAST_CONFLICT }, .minval = 0, .maxval = UINT_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_SWIDTH, .conflicts = { D_NOALIGN, @@ -230,7 +230,7 @@ struct opt_params dopts = { LAST_CONFLICT }, .minval = 0, .maxval = UINT_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_AGSIZE, .conflicts = { D_AGCOUNT, @@ -238,7 +238,7 @@ struct opt_params dopts = { .convert = true, .minval = XFS_AG_MIN_BYTES, .maxval = XFS_AG_MAX_BYTES, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_SU, .conflicts = { D_NOALIGN, @@ -248,7 +248,7 @@ struct opt_params dopts = { .convert = true, .minval = 0, .maxval = UINT_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_SW, .conflicts = { D_NOALIGN, @@ -257,14 +257,14 @@ struct opt_params dopts = { LAST_CONFLICT }, .minval = 0, .maxval = UINT_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_SECTLOG, .conflicts = { D_SECTSIZE, LAST_CONFLICT }, .minval = XFS_MIN_SECTORSIZE_LOG, .maxval = XFS_MAX_SECTORSIZE_LOG, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_SECTSIZE, .conflicts = { D_SECTLOG, @@ -273,7 +273,7 @@ struct opt_params dopts = { .is_power_2 = true, .minval = XFS_MIN_SECTORSIZE, .maxval = XFS_MAX_SECTORSIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_NOALIGN, .conflicts = { D_SU, @@ -283,25 +283,25 @@ struct opt_params dopts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = D_RTINHERIT, .conflicts = { LAST_CONFLICT }, .minval = 1, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = D_PROJINHERIT, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = UINT_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = D_EXTSZINHERIT, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = UINT_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, }, }; @@ -333,7 +333,7 @@ struct opt_params iopts = { .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = I_LOG, .conflicts = { I_PERBLOCK, @@ -341,13 +341,13 @@ struct opt_params iopts = { LAST_CONFLICT }, .minval = XFS_DINODE_MIN_LOG, .maxval = XFS_DINODE_MAX_LOG, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = I_MAXPCT, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 100, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = I_PERBLOCK, .conflicts = { I_LOG, @@ -356,7 +356,7 @@ struct opt_params iopts = { .is_power_2 = true, .minval = XFS_MIN_INODE_PERBLOCK, .maxval = XFS_MAX_BLOCKSIZE / XFS_DINODE_MIN_SIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = I_SIZE, .conflicts = { I_PERBLOCK, @@ -365,25 +365,25 @@ struct opt_params iopts = { .is_power_2 = true, .minval = XFS_DINODE_MIN_SIZE, .maxval = XFS_DINODE_MAX_SIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = I_ATTR, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 2, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = I_PROJID32BIT, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = I_SPINODES, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, }, }; @@ -423,7 +423,7 @@ struct opt_params lopts = { LAST_CONFLICT }, .minval = 0, .maxval = UINT_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_INTERNAL, .conflicts = { L_FILE, @@ -431,27 +431,27 @@ struct opt_params lopts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = L_SIZE, .conflicts = { LAST_CONFLICT }, .convert = true, .minval = 2 * 1024 * 1024LL, /* XXX: XFS_MIN_LOG_BYTES */ .maxval = XFS_MAX_LOG_BYTES, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_VERSION, .conflicts = { LAST_CONFLICT }, .minval = 1, .maxval = 2, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_SUNIT, .conflicts = { L_SU, LAST_CONFLICT }, .minval = 1, .maxval = BTOBB(XLOG_MAX_RECORD_BSIZE), - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_SU, .conflicts = { L_SUNIT, @@ -459,20 +459,20 @@ struct opt_params lopts = { .convert = true, .minval = BBTOB(1), .maxval = XLOG_MAX_RECORD_BSIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_DEV, .conflicts = { L_AGNUM, L_INTERNAL, LAST_CONFLICT }, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_SECTLOG, .conflicts = { L_SECTSIZE, LAST_CONFLICT }, .minval = XFS_MIN_SECTORSIZE_LOG, .maxval = XFS_MAX_SECTORSIZE_LOG, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_SECTSIZE, .conflicts = { L_SECTLOG, @@ -481,26 +481,26 @@ struct opt_params lopts = { .is_power_2 = true, .minval = XFS_MIN_SECTORSIZE, .maxval = XFS_MAX_SECTORSIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_FILE, .conflicts = { L_INTERNAL, LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = L_NAME, .conflicts = { L_AGNUM, L_INTERNAL, LAST_CONFLICT }, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = L_LAZYSBCNTR, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, }, }; @@ -524,7 +524,7 @@ struct opt_params nopts = { LAST_CONFLICT }, .minval = XFS_MIN_REC_DIRSIZE, .maxval = XFS_MAX_BLOCKSIZE_LOG, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = N_SIZE, .conflicts = { N_LOG, @@ -533,19 +533,19 @@ struct opt_params nopts = { .is_power_2 = true, .minval = 1 << XFS_MIN_REC_DIRSIZE, .maxval = XFS_MAX_BLOCKSIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = N_VERSION, .conflicts = { LAST_CONFLICT }, .minval = 2, .maxval = 2, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = N_FTYPE, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, }, }; @@ -573,33 +573,33 @@ struct opt_params ropts = { .convert = true, .minval = XFS_MIN_RTEXTSIZE, .maxval = XFS_MAX_RTEXTSIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = R_SIZE, .conflicts = { LAST_CONFLICT }, .convert = true, .minval = 0, .maxval = LLONG_MAX, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = R_DEV, .conflicts = { LAST_CONFLICT }, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = R_FILE, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, .conflicts = { LAST_CONFLICT }, }, { .index = R_NAME, .conflicts = { LAST_CONFLICT }, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = R_NOALIGN, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, .conflicts = { LAST_CONFLICT }, }, }, @@ -625,7 +625,7 @@ struct opt_params sopts = { LAST_CONFLICT }, .minval = XFS_MIN_SECTORSIZE_LOG, .maxval = XFS_MAX_SECTORSIZE_LOG, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = S_SECTLOG, .conflicts = { S_SIZE, @@ -633,7 +633,7 @@ struct opt_params sopts = { LAST_CONFLICT }, .minval = XFS_MIN_SECTORSIZE_LOG, .maxval = XFS_MAX_SECTORSIZE_LOG, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = S_SIZE, .conflicts = { S_LOG, @@ -643,7 +643,7 @@ struct opt_params sopts = { .is_power_2 = true, .minval = XFS_MIN_SECTORSIZE, .maxval = XFS_MAX_SECTORSIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = S_SECTSIZE, .conflicts = { S_LOG, @@ -653,7 +653,7 @@ struct opt_params sopts = { .is_power_2 = true, .minval = XFS_MIN_SECTORSIZE, .maxval = XFS_MAX_SECTORSIZE, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, }, }; @@ -678,29 +678,29 @@ struct opt_params mopts = { .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = M_FINOBT, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = M_UUID, .conflicts = { LAST_CONFLICT }, - .defaultval = SUBOPT_NEEDS_VAL, + .flagval = SUBOPT_NEEDS_VAL, }, { .index = M_RMAPBT, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, { .index = M_REFLINK, .conflicts = { LAST_CONFLICT }, .minval = 0, .maxval = 1, - .defaultval = 1, + .flagval = 1, }, }, }; @@ -1329,9 +1329,9 @@ getnum( check_opt(opts, index, false); /* empty strings might just return a default value */ if (!str || *str == '\0') { - if (sp->defaultval == SUBOPT_NEEDS_VAL) + if (sp->flagval == SUBOPT_NEEDS_VAL) reqval(opts->name, (char **)opts->subopts, index); - return sp->defaultval; + return sp->flagval; } if (sp->minval == 0 && sp->maxval == 0) {