On Wed, Mar 08, 2017 at 10:41:52PM -0600, Eric Sandeen wrote: > > On 3/8/17 6:51 PM, Luis R. Rodriguez wrote: > > On Wed, Mar 08, 2017 at 06:16:57PM -0600, Eric Sandeen wrote: > >> On 3/3/17 5:13 PM, Luis R. Rodriguez wrote: > >>> This series adds mkfs.xfs.conf support, so that options can now be > >>> shoved into a configuration file. This enables certain defaults to be > >>> saved for folks sticking to certain values, but more importantly it > >>> also enables distributions to override certain defaults so that new > >>> filesystems remain compatible with older distributions. > >>> > >>> This has been based on top of xfsprogs-dev v4.9.0-rc1. > >>> > >>> Given we already have an existinsg infrastructure to validate argument > >>> values this reuses that infrastructure by first adding helpers and porting > >>> over the argument parsing suppor to use these helpers. > >> > >> Hm, one functional problem with this, aside from Dave's concerns and > >> suggestions, is that many options in the config file can't actually > >> be overridden on the commandline because they are treated as having > >> been respecified, which is not allowed: > >> > >> # mkfs.xfs -m crc=1 -m crc=0 -f fsfile > >> -m crc option respecified > >> Usage: mkfs.xfs > > > > This was dealt by enabling the last option taken to override, and > > this mechanism was also taken to enable the config file to take > > the first value but let the command line to override. Refer to > > usage of reset_opt(). Granted I had only done this on B_LOG but > > this can easily be made to enable us to reset for all options. > > Well, the above test was with your full patchset applied, > so my point is that it's not currently working properly as posted... Sorry yes I failed to reset_opt() for each config entry on the config file. I had meant to move reset_opt() generically on parse_subopts() so that "last entry specified wins" always. I had only added this for B_LOG. > 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. *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. > > If we don't want to enable subsequent command line entries to > > override (to keep old behaviour) but still allow at least the > > command line to override the config file options, that's also > > doable. > > Well, I'm going to need to refamiliarize myself with how the > conflict checking works, and why respecification is prohibited. > If respecification matters, it matters just as much whether the > first specification came from the config file or from the command > line. Agreed. The above is what I concluded based on my inspection of the code. It would be of course great to get more eyeballs on it. Luis -- 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
