On Thu, Mar 09, 2017 at 06:57:51PM +0100, Luis R. Rodriguez wrote: > On Wed, Mar 08, 2017 at 10:41:52PM -0600, Eric Sandeen wrote: > > But are you proposing adding this reset_opt() to /every/ option? > > That would undo all of the respecification checks, which were > > put there for a reason (I assume?) ;) I don't really remember > > how all of the respecification and compatibility checking works > > tbh, I'd have to dig back into it. Maybe jtulak can help... > > > > But it makes little sense to have a framework to prevent > > respecification but then render it useless with reset_opt() > > after each option gets parsed. Or do I misunderstand? > > 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. 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. IOWs, the config file should set the default values in the option table, not set the options directly as happens on the command line.... > *Proper validation* is and should be done *after* all parameters are > set, and it sounds like jtulak's work helps shove this into helpers > *as should be done* to clear cruft out of main(). Having these params > in structs should help both efforts. Right, that was the whole point of the table based option parsing I started on and Jan has been continuing - factoring the option parsing code and getting rid of all the whacky variables used to say "this option is set" and move all the config state into the option table. Then all the whacky variables can be replaced with option table lookups, and so if the option has not been set on the command line, the default value can be picked up from the table (which was set from the config file). Only the first step (table based parsing) has been done so far - there's still plenty of work needed to get the option table and code to the point where we use it exclusively for config lookups. Once we get there, then using a config file for default values is easy to add... 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
