On 1/12/24 14:48, Ilya Maximets wrote:
> 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?
>
Sounds good.
>>> 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?
>
Even better. With that:
Acked-by: Dumitru Ceara <[email protected]>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
