On Mon, Jan 8, 2024 at 8:17 AM Ilya Maximets <i.maxim...@ovn.org> wrote: > > On 1/5/24 21:26, Terry Wilson wrote: > > On Wed, Dec 13, 2023 at 7:05 PM Ilya Maximets <i.maxim...@ovn.org> wrote: > > > >> -/* Clears and replaces 'remotes' and 'dbnames' by a configuration read > >> from > >> - * 'config_file', which must have been previously written by > >> save_config(). */ > >> -static void > >> +/* Clears and replaces 'remotes' and 'db_conf' by a configuration read > >> from > >> + * 'config_file', which must have been previously written by save_config() > >> + * or provided by the user with --config-file. > >> + * Returns 'true', if parsing was successful, 'false' otherwise. */ > >> +static bool > >> load_config(FILE *config_file, struct shash *remotes, > >> struct shash *db_conf, char **sync_from, > >> char **sync_exclude, bool *is_backup) > >> @@ -2890,17 +3117,34 @@ load_config(FILE *config_file, struct shash > >> *remotes, > >> struct json *json; > > > > I'm wondering if having an argument for everything we parse out of a > > config file might get a little unwieldy down the road. Say we add > > configuration of SSL stuff, etc. Maybe it's something we could modify > > as it becomes an issue, but it might be nice to have something for > > config options that is similar to what we have for registering unixctl > > commands or struct ctl_command_syntax. Documentation could be added as > > part of the registration/definition of the config option, there could > > be a .get() that parses the values out of the json, and a .run() that > > gets called after all of the parsing is shown to succeed. > > > > Terry > > > > Hi, Terry. Yes, I agree that the current interface is far from being > ideal, and I actually tried to rework it multiple times while working > on this patch set. That's one of the reasons it took so long. :) > Unfortunately it ended up with a huge amount of refactoring every time. > The main reason for that, I think, is the fact that the same function > is used for loading the legacy internal configuration file and the new > user-provided file. And the code around legacy internal configuration > is not easy to adapt. > > I think, it would be much easier to do this kind of refactoring once > we deprecate and remove all the appctl commands that can change the > server configuration. For now I decided to keep the interface as it is > without this patch set to avoid making the patch set even bigger. > I hope that makes sense. > > The idea of registering the options with a common parsing logic sounds > interesting though and definitely worth exploring in the future. > > Best regards, Ilya Maximets.
Yeah, I'm all for merging as-is and prettying up as needed. In case you have entirely too much free time on your hands (ha!), here's an ini-style config loading/reloading framework I wrote many many years ago (Warning: I'd only been programming professionally for ~3 years or so at that point). Asterisk was very modular and had over 100 different config files, and many of them used ini files in completely different ways so it's definitely more generic and overkill for what we are needing, but there may be some useful ideas [1-5] if you ignore all of the macro/varargs magic. ;) The general idea was a per-module `aco_info` structure that defined the information that was obtained from configs, `aco_type` structures that defined what parts of the config file matched up to what fields of the `aco_info` struc, and aco_option_register() which would define the specific options set under each [category], how they were matched, what fields in a struct they set, etc. aco_option_register_custom() just used custom callback functions to handle setting things up. aco_process_config() would be called on load/reload and would parse the config file and create an internal 'pending' representation of the user-defined aco_info and then if parsing succeeded would then atomically swap the pending and live aco_info pointers. There were also pre_apply and post_apply callbacks so modules could do special things if needed during reloads, etc. Prior to the patch modules would often just modify their internal structures as the config file was being processed, so if there was a parse error things would just be left in a state where configs were partially applied. Anyway, it may or may not be interesting and certainly isn't something we need to worry about right now. Just food for thought. Terry [1] https://github.com/asterisk/asterisk/blob/master/include/asterisk/config_options.h [2] https://github.com/asterisk/asterisk/blob/master/main/config_options.c [3] https://github.com/asterisk/asterisk/blob/master/configs/samples/app_skel.conf.sample [4] https://github.com/asterisk/asterisk/blob/master/apps/app_skel.c [5] https://github.com/asterisk/asterisk/commit/d54717c39e62f4cc8b290ac4836c4d4469d87c24 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
