On 1/12/24 12:22, Dumitru Ceara wrote:
> On 1/9/24 23:49, Ilya Maximets wrote:
>> Store JSON-RPC options for each remote separately, so it will be
>> possible to have different configurations per remote in the future.
>>
>> These are also stored to and loaded from the temporary file that
>> OVSDB is using to restore runtime configuration of the server
>> restarted by the monitor process after a crash.
>>
>> Signed-off-by: Ilya Maximets <[email protected]>
>> ---
>> ovsdb/jsonrpc-server.c | 11 ++++
>> ovsdb/jsonrpc-server.h | 6 +-
>> ovsdb/ovsdb-server.c | 136 ++++++++++++++++++++++++++++-------------
>> 3 files changed, 110 insertions(+), 43 deletions(-)
>>
>> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
>> index 51b7db886..299afbb1d 100644
>> --- a/ovsdb/jsonrpc-server.c
>> +++ b/ovsdb/jsonrpc-server.c
>> @@ -219,6 +219,17 @@ ovsdb_jsonrpc_default_options(const char *target)
>> return options;
>> }
>>
>> +struct ovsdb_jsonrpc_options *
>> +ovsdb_jsonrpc_options_clone(const struct ovsdb_jsonrpc_options *options)
>> +{
>> + struct ovsdb_jsonrpc_options *clone;
>> +
>> + clone = xmemdup(options, sizeof *options);
>> + clone->role = nullable_xstrdup(options->role);
>> +
>> + return clone;
>> +}
>> +
>> struct json *
>> ovsdb_jsonrpc_options_to_json(const struct ovsdb_jsonrpc_options *options)
>> {
>> diff --git a/ovsdb/jsonrpc-server.h b/ovsdb/jsonrpc-server.h
>> index 9c49966c1..39366ad70 100644
>> --- a/ovsdb/jsonrpc-server.h
>> +++ b/ovsdb/jsonrpc-server.h
>> @@ -39,8 +39,10 @@ struct ovsdb_jsonrpc_options {
>> int dscp; /* Dscp value for manager connections */
>> char *role; /* Role, for role-based access controls */
>> };
>> -struct ovsdb_jsonrpc_options *
>> -ovsdb_jsonrpc_default_options(const char *target);
>> +struct ovsdb_jsonrpc_options *ovsdb_jsonrpc_default_options(
>> + const char *target);
>> +struct ovsdb_jsonrpc_options *ovsdb_jsonrpc_options_clone(
>> + const struct ovsdb_jsonrpc_options *);
>>
>> struct json *ovsdb_jsonrpc_options_to_json(
>> const struct ovsdb_jsonrpc_options *)
>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
>> index c294ebe67..7f65cadfe 100644
>> --- a/ovsdb/ovsdb-server.c
>> +++ b/ovsdb/ovsdb-server.c
>> @@ -101,7 +101,7 @@ static unixctl_cb_func ovsdb_server_get_sync_status;
>> static unixctl_cb_func ovsdb_server_get_db_storage_status;
>>
>> struct server_config {
>> - struct sset *remotes;
>> + struct shash *remotes;
>> struct shash *all_dbs;
>> FILE *config_tmpfile;
>> char **sync_from;
>> @@ -130,29 +130,34 @@ static void remove_db(struct server_config *, struct
>> shash_node *db, char *);
>> static void close_db(struct server_config *, struct db *, char *);
>>
>> static void parse_options(int argc, char *argvp[],
>> - struct sset *db_filenames, struct sset *remotes,
>> + struct sset *db_filenames, struct shash *remotes,
>> char **unixctl_pathp, char **run_command,
>> char **sync_from, char **sync_exclude,
>> bool *is_backup);
>> OVS_NO_RETURN static void usage(void);
>>
>> +static struct ovsdb_jsonrpc_options *add_remote(
>> + struct shash *remotes, const char *target,
>> + const struct ovsdb_jsonrpc_options *);
>> +static void free_remotes(struct shash *remotes);
>> +
>> static char *reconfigure_remotes(struct ovsdb_jsonrpc_server *,
>> const struct shash *all_dbs,
>> - struct sset *remotes);
>> + struct shash *remotes);
>> static char *reconfigure_ssl(const struct shash *all_dbs);
>> static void report_error_if_changed(char *error, char **last_errorp);
>>
>> static void update_remote_status(const struct ovsdb_jsonrpc_server *jsonrpc,
>> - const struct sset *remotes,
>> + const struct shash *remotes,
>> struct shash *all_dbs);
>> static void update_server_status(struct shash *all_dbs);
>>
>> -static void save_config__(FILE *config_file, const struct sset *remotes,
>> +static void save_config__(FILE *config_file, const struct shash *remotes,
>> const struct sset *db_filenames,
>> const char *sync_from, const char *sync_exclude,
>> bool is_backup);
>> static void save_config(struct server_config *);
>> -static void load_config(FILE *config_file, struct sset *remotes,
>> +static void load_config(FILE *config_file, struct shash *remotes,
>> struct sset *db_filenames, char **sync_from,
>> char **sync_exclude, bool *is_backup);
>>
>> @@ -184,7 +189,7 @@ log_and_free_error(struct ovsdb_error *error)
>> static void
>> main_loop(struct server_config *config,
>> struct ovsdb_jsonrpc_server *jsonrpc, struct shash *all_dbs,
>> - struct unixctl_server *unixctl, struct sset *remotes,
>> + struct unixctl_server *unixctl, struct shash *remotes,
>> struct process *run_process, bool *exiting, bool *is_backup)
>> {
>> char *remotes_error, *ssl_error;
>> @@ -318,7 +323,8 @@ main(int argc, char *argv[])
>> char *run_command = NULL;
>> struct unixctl_server *unixctl;
>> struct ovsdb_jsonrpc_server *jsonrpc;
>> - struct sset remotes, db_filenames;
>> + struct sset db_filenames;
>> + struct shash remotes;
>> char *sync_from, *sync_exclude;
>> bool is_backup;
>> const char *db_filename;
>> @@ -514,7 +520,7 @@ main(int argc, char *argv[])
>> }
>> ovsdb_jsonrpc_server_destroy(jsonrpc);
>> shash_destroy(&all_dbs);
>> - sset_destroy(&remotes);
>> + free_remotes(&remotes);
>
> Nit: I know it's not really introduced by your patch but we're changing
> this code so might as well..
>
> I had to search for where 'remotes' gets properly initialized (inside
> parse_options()). It feels weird that we pass an unitialized variable
> and then free it in the caller. It's clear that parse_options() always
> initializes 'remotes' (and also 'db_filenames') but we have to read the
> implementation of the function for that.
>
> I'd just use SSET_INITIALIZER()/SHASH_INITIALIZER() when we declare them
> in main_loop().
>
In this case we also need to initialize sync_from and sync_exclude,
to be consistent. I can add those. What do you think?
>> sset_destroy(&db_filenames);
>> free(sync_from);
>> free(sync_exclude);
>> @@ -971,13 +977,16 @@ query_db_string(const struct shash *all_dbs, const
>> char *name,
>> }
>>
>> static struct ovsdb_jsonrpc_options *
>> -add_remote(struct shash *remotes, const char *target)
>> +add_remote(struct shash *remotes, const char *target,
>> + const struct ovsdb_jsonrpc_options *options_)
>> {
>> struct ovsdb_jsonrpc_options *options;
>>
>> options = shash_find_data(remotes, target);
>> if (!options) {
>> - options = ovsdb_jsonrpc_default_options(target);
>> + options = options_
>> + ? ovsdb_jsonrpc_options_clone(options_)
>> + : ovsdb_jsonrpc_default_options(target);
>> shash_add(remotes, target, options);
>> }
>>
>> @@ -995,6 +1004,7 @@ free_remotes(struct shash *remotes)
>> free(options->role);
>> }
>> shash_destroy_free_data(remotes);
>> + shash_init(remotes);
>
> Hmm, this feels awkward. I guess it's because remotes_from_json() must
> maintain its 'remotes' argument as a valid shash. But then isn't it
> better to just add a shash_init(remotes) immediately after
> free_remotes() in remotes_from_json()?
Good point, but it might be better to to replace above two lines with
shash_clear_free_data() instead. This way the hash map will be preserved.
We would need to add shash_destroy() to the end of a main function, but
that probably should be done anyway.
What do you think?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
