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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
