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().
> 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()?
Regards,
Dumitru
> }
> }
>
> @@ -1015,7 +1025,7 @@ add_manager_options(struct shash *remotes, const struct
> ovsdb_row *row)
> return;
> }
>
> - options = add_remote(remotes, target);
> + options = add_remote(remotes, target, NULL);
> if (ovsdb_util_read_integer_column(row, "max_backoff", &max_backoff)) {
> options->max_backoff = max_backoff;
> }
> @@ -1075,7 +1085,7 @@ query_db_remotes(const char *name, const struct shash
> *all_dbs,
>
> datum = &row->fields[column->index];
> for (i = 0; i < datum->n; i++) {
> - add_remote(remotes, json_string(datum->keys[i].s));
> + add_remote(remotes, json_string(datum->keys[i].s), NULL);
> }
> }
> } else if (column->type.key.type == OVSDB_TYPE_UUID
> @@ -1223,19 +1233,24 @@ commit_txn(struct ovsdb_txn *txn, const char *name)
>
> static void
> update_remote_status(const struct ovsdb_jsonrpc_server *jsonrpc,
> - const struct sset *remotes,
> + const struct shash *remotes,
> struct shash *all_dbs)
> {
> - struct shash_node *node;
> - SHASH_FOR_EACH (node, all_dbs) {
> - struct db *db = node->data;
> + struct shash_node *db_node;
> +
> + SHASH_FOR_EACH (db_node, all_dbs) {
> + struct db *db = db_node->data;
> +
> if (!db->db || ovsdb_storage_is_clustered(db->db->storage)) {
> continue;
> }
>
> struct ovsdb_txn *txn = ovsdb_txn_create(db->db);
> - const char *remote;
> - SSET_FOR_EACH (remote, remotes) {
> + const struct shash_node *remote_node;
> +
> + SHASH_FOR_EACH (remote_node, remotes) {
> + const char *remote = remote_node->name;
> +
> update_remote_rows(all_dbs, db, remote, jsonrpc, txn);
> }
> commit_txn(txn, "remote status");
> @@ -1342,19 +1357,22 @@ update_server_status(struct shash *all_dbs)
> /* Reconfigures ovsdb-server's remotes based on information in the database.
> */
> static char *
> reconfigure_remotes(struct ovsdb_jsonrpc_server *jsonrpc,
> - const struct shash *all_dbs, struct sset *remotes)
> + const struct shash *all_dbs, struct shash *remotes)
> {
> struct ds errors = DS_EMPTY_INITIALIZER;
> struct shash resolved_remotes;
> - const char *name;
> + struct shash_node *node;
>
> /* Configure remotes. */
> shash_init(&resolved_remotes);
> - SSET_FOR_EACH (name, remotes) {
> + SHASH_FOR_EACH (node, remotes) {
> + const struct ovsdb_jsonrpc_options *options = node->data;
> + const char *name = node->name;
> +
> if (!strncmp(name, "db:", 3)) {
> query_db_remotes(name, all_dbs, &resolved_remotes, &errors);
> } else {
> - add_remote(&resolved_remotes, name);
> + add_remote(&resolved_remotes, name, options);
> }
> }
> ovsdb_jsonrpc_server_set_remotes(jsonrpc, &resolved_remotes);
> @@ -1719,7 +1737,7 @@ ovsdb_server_add_remote(struct unixctl_conn *conn, int
> argc OVS_UNUSED,
> : parse_db_column(config->all_dbs, remote,
> &db, &table, &column));
> if (!retval) {
> - if (sset_add(config->remotes, remote)) {
> + if (add_remote(config->remotes, remote, NULL)) {
> save_config(config);
> }
> unixctl_command_reply(conn, NULL);
> @@ -1736,11 +1754,12 @@ ovsdb_server_remove_remote(struct unixctl_conn *conn,
> int argc OVS_UNUSED,
> const char *argv[], void *config_)
> {
> struct server_config *config = config_;
> - struct sset_node *node;
> + struct ovsdb_jsonrpc_options *options;
>
> - node = sset_find(config->remotes, argv[1]);
> - if (node) {
> - sset_delete(config->remotes, node);
> + options = shash_find_and_delete(config->remotes, argv[1]);
> + if (options) {
> + free(options->role);
> + free(options);
> save_config(config);
> unixctl_command_reply(conn, NULL);
> } else {
> @@ -1753,15 +1772,15 @@ static void
> ovsdb_server_list_remotes(struct unixctl_conn *conn, int argc OVS_UNUSED,
> const char *argv[] OVS_UNUSED, void *remotes_)
> {
> - struct sset *remotes = remotes_;
> - const char **list, **p;
> + const struct shash *remotes = remotes_;
> + const struct shash_node **list;
> struct ds s;
>
> ds_init(&s);
>
> - list = sset_sort(remotes);
> - for (p = list; *p; p++) {
> - ds_put_format(&s, "%s\n", *p);
> + list = shash_sort(remotes);
> + for (size_t i = 0; i < shash_count(remotes); i++) {
> + ds_put_format(&s, "%s\n", list[i]->name);
> }
> free(list);
>
> @@ -1996,7 +2015,7 @@ ovsdb_server_get_db_storage_status(struct unixctl_conn
> *conn,
>
> static void
> parse_options(int argc, char *argv[],
> - 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 *active)
> {
> @@ -2047,7 +2066,7 @@ parse_options(int argc, char *argv[],
> *sync_from = NULL;
> *sync_exclude = NULL;
> sset_init(db_filenames);
> - sset_init(remotes);
> + shash_init(remotes);
> for (;;) {
> int c;
>
> @@ -2058,7 +2077,7 @@ parse_options(int argc, char *argv[],
>
> switch (c) {
> case OPT_REMOTE:
> - sset_add(remotes, optarg);
> + add_remote(remotes, optarg, NULL);
> break;
>
> case OPT_UNIXCTL:
> @@ -2197,10 +2216,24 @@ sset_to_json(const struct sset *sset)
> return array;
> }
>
> +static struct json *
> +remotes_to_json(const struct shash *remotes)
> +{
> + const struct shash_node *node;
> + struct json *json;
> +
> + json = json_object_create();
> + SHASH_FOR_EACH (node, remotes) {
> + json_object_put(json, node->name,
> + ovsdb_jsonrpc_options_to_json(node->data));
> + }
> + return json;
> +}
> +
> /* Truncates and replaces the contents of 'config_file' by a representation
> of
> * 'remotes' and 'db_filenames'. */
> static void
> -save_config__(FILE *config_file, const struct sset *remotes,
> +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)
> {
> @@ -2213,7 +2246,7 @@ save_config__(FILE *config_file, const struct sset
> *remotes,
> }
>
> obj = json_object_create();
> - json_object_put(obj, "remotes", sset_to_json(remotes));
> + json_object_put(obj, "remotes", remotes_to_json(remotes));
> json_object_put(obj, "db_filenames", sset_to_json(db_filenames));
> if (sync_from) {
> json_object_put(obj, "sync_from", json_string_create(sync_from));
> @@ -2273,11 +2306,32 @@ sset_from_json(struct sset *sset, const struct json
> *array)
> }
> }
>
> +static void
> +remotes_from_json(struct shash *remotes, const struct json *json)
> +{
> + struct ovsdb_jsonrpc_options *options;
> + const struct shash_node *node;
> + const struct shash *object;
> +
> + free_remotes(remotes);
> +
> + ovs_assert(json);
> + ovs_assert(json->type == JSON_OBJECT);
> +
> + object = json_object(json);
> + SHASH_FOR_EACH (node, object) {
> + options = ovsdb_jsonrpc_default_options(node->name);
> + ovsdb_jsonrpc_options_update_from_json(options, node->data);
> + shash_add(remotes, node->name, options);
> + }
> +}
> +
> /* Clears and replaces 'remotes' and 'dbnames' by a configuration read from
> * 'config_file', which must have been previously written by save_config().
> */
> static void
> -load_config(FILE *config_file, struct sset *remotes, struct sset
> *db_filenames,
> - char **sync_from, char **sync_exclude, bool *is_backup)
> +load_config(FILE *config_file, struct shash *remotes,
> + struct sset *db_filenames, char **sync_from,
> + char **sync_exclude, bool *is_backup)
> {
> struct json *json;
>
> @@ -2290,7 +2344,7 @@ load_config(FILE *config_file, struct sset *remotes,
> struct sset *db_filenames,
> }
> ovs_assert(json->type == JSON_OBJECT);
>
> - sset_from_json(remotes, shash_find_data(json_object(json), "remotes"));
> + remotes_from_json(remotes, shash_find_data(json_object(json),
> "remotes"));
> sset_from_json(db_filenames,
> shash_find_data(json_object(json), "db_filenames"));
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
