diff options
| -rw-r--r-- | isisd/isis_cli.c | 111 | ||||
| -rw-r--r-- | lib/command.c | 16 | ||||
| -rw-r--r-- | lib/if.c | 19 | ||||
| -rw-r--r-- | lib/libfrr.c | 7 | ||||
| -rw-r--r-- | lib/northbound.c | 152 | ||||
| -rw-r--r-- | lib/northbound.h | 11 | ||||
| -rw-r--r-- | lib/northbound_cli.c | 139 | ||||
| -rw-r--r-- | lib/northbound_confd.c | 6 | ||||
| -rw-r--r-- | lib/northbound_sysrepo.c | 6 | ||||
| -rw-r--r-- | lib/vty.c | 21 |
10 files changed, 305 insertions, 183 deletions
diff --git a/isisd/isis_cli.c b/isisd/isis_cli.c index 0094852825..0334b98a12 100644 --- a/isisd/isis_cli.c +++ b/isisd/isis_cli.c @@ -188,10 +188,14 @@ DEFPY(ip_router_isis, ip_router_isis_cmd, "ip router isis WORD$tag", } /* check if the interface is a loopback and if so set it as passive */ - ifp = nb_running_get_entry(NULL, VTY_CURR_XPATH, false); - if (ifp && if_is_loopback(ifp)) - nb_cli_enqueue_change(vty, "./frr-isisd:isis/passive", - NB_OP_MODIFY, "true"); + pthread_rwlock_rdlock(&running_config->lock); + { + ifp = nb_running_get_entry(NULL, VTY_CURR_XPATH, false); + if (ifp && if_is_loopback(ifp)) + nb_cli_enqueue_change(vty, "./frr-isisd:isis/passive", + NB_OP_MODIFY, "true"); + } + pthread_rwlock_unlock(&running_config->lock); return nb_cli_apply_changes(vty, NULL); } @@ -258,10 +262,14 @@ DEFPY(ip6_router_isis, ip6_router_isis_cmd, "ipv6 router isis WORD$tag", } /* check if the interface is a loopback and if so set it as passive */ - ifp = nb_running_get_entry(NULL, VTY_CURR_XPATH, false); - if (ifp && if_is_loopback(ifp)) - nb_cli_enqueue_change(vty, "./frr-isisd:isis/passive", - NB_OP_MODIFY, "true"); + pthread_rwlock_rdlock(&running_config->lock); + { + ifp = nb_running_get_entry(NULL, VTY_CURR_XPATH, false); + if (ifp && if_is_loopback(ifp)) + nb_cli_enqueue_change(vty, "./frr-isisd:isis/passive", + NB_OP_MODIFY, "true"); + } + pthread_rwlock_unlock(&running_config->lock); return nb_cli_apply_changes(vty, NULL); } @@ -368,20 +376,26 @@ DEFPY(no_is_type, no_is_type_cmd, "Act as both a station router and an area router\n" "Act as an area router only\n") { - const char *value = NULL; - struct isis_area *area; + const char *value; - area = nb_running_get_entry(NULL, VTY_CURR_XPATH, false); + pthread_rwlock_rdlock(&running_config->lock); + { + struct isis_area *area; + + area = nb_running_get_entry(NULL, VTY_CURR_XPATH, false); + + /* + * Put the is-type back to defaults: + * - level-1-2 on first area + * - level-1 for the rest + */ + if (area && listgetdata(listhead(isis->area_list)) == area) + value = "level-1-2"; + else + value = NULL; + } + pthread_rwlock_unlock(&running_config->lock); - /* - * Put the is-type back to defaults: - * - level-1-2 on first area - * - level-1 for the rest - */ - if (area && listgetdata(listhead(isis->area_list)) == area) - value = "level-1-2"; - else - value = NULL; nb_cli_enqueue_change(vty, "./is-type", NB_OP_MODIFY, value); return nb_cli_apply_changes(vty, NULL); @@ -1769,50 +1783,43 @@ DEFPY(no_isis_circuit_type, no_isis_circuit_type_cmd, "Level-1-2 adjacencies are formed\n" "Level-2 only adjacencies are formed\n") { - struct interface *ifp; - struct isis_circuit *circuit; - int is_type; - const char *circ_type; + const char *circ_type = NULL; /* * Default value depends on whether the circuit is part of an area, * and the is-type of the area if there is one. So we need to do this * here. */ - ifp = nb_running_get_entry(NULL, VTY_CURR_XPATH, false); - if (!ifp) - goto def_val; + pthread_rwlock_rdlock(&running_config->lock); + { + struct interface *ifp; + struct isis_circuit *circuit; - circuit = circuit_scan_by_ifp(ifp); - if (!circuit) - goto def_val; + ifp = nb_running_get_entry(NULL, VTY_CURR_XPATH, false); + if (!ifp) + goto unlock; - if (circuit->state == C_STATE_UP) - is_type = circuit->area->is_type; - else - goto def_val; + circuit = circuit_scan_by_ifp(ifp); + if (!circuit || circuit->state != C_STATE_UP) + goto unlock; - switch (is_type) { - case IS_LEVEL_1: - circ_type = "level-1"; - break; - case IS_LEVEL_2: - circ_type = "level-2"; - break; - case IS_LEVEL_1_AND_2: - circ_type = "level-1-2"; - break; - default: - return CMD_ERR_NO_MATCH; + switch (circuit->area->is_type) { + case IS_LEVEL_1: + circ_type = "level-1"; + break; + case IS_LEVEL_2: + circ_type = "level-2"; + break; + case IS_LEVEL_1_AND_2: + circ_type = "level-1-2"; + break; + } } - nb_cli_enqueue_change(vty, "./frr-isisd:isis/circuit-type", - NB_OP_MODIFY, circ_type); - - return nb_cli_apply_changes(vty, NULL); +unlock: + pthread_rwlock_unlock(&running_config->lock); -def_val: nb_cli_enqueue_change(vty, "./frr-isisd:isis/circuit-type", - NB_OP_MODIFY, NULL); + NB_OP_MODIFY, circ_type); return nb_cli_apply_changes(vty, NULL); } diff --git a/lib/command.c b/lib/command.c index 559457c119..b3ef028004 100644 --- a/lib/command.c +++ b/lib/command.c @@ -1705,12 +1705,16 @@ static int vty_write_config(struct vty *vty) vty_out(vty, "frr defaults %s\n", DFLT_NAME); vty_out(vty, "!\n"); - for (i = 0; i < vector_active(cmdvec); i++) - if ((node = vector_slot(cmdvec, i)) && node->func - && (node->vtysh || vty->type != VTY_SHELL)) { - if ((*node->func)(vty)) - vty_out(vty, "!\n"); - } + pthread_rwlock_rdlock(&running_config->lock); + { + for (i = 0; i < vector_active(cmdvec); i++) + if ((node = vector_slot(cmdvec, i)) && node->func + && (node->vtysh || vty->type != VTY_SHELL)) { + if ((*node->func)(vty)) + vty_out(vty, "!\n"); + } + } + pthread_rwlock_unlock(&running_config->lock); if (vty->type == VTY_TERM) { vty_out(vty, "end\n"); @@ -187,18 +187,21 @@ void if_update_to_new_vrf(struct interface *ifp, vrf_id_t vrf_id) if (yang_module_find("frr-interface")) { struct lyd_node *if_dnode; - if_dnode = yang_dnode_get( - running_config->dnode, - "/frr-interface:lib/interface[name='%s'][vrf='%s']/vrf", - ifp->name, old_vrf->name); - if (if_dnode) { - yang_dnode_change_leaf(if_dnode, vrf->name); - running_config->version++; + pthread_rwlock_wrlock(&running_config->lock); + { + if_dnode = yang_dnode_get( + running_config->dnode, + "/frr-interface:lib/interface[name='%s'][vrf='%s']/vrf", + ifp->name, old_vrf->name); + if (if_dnode) { + yang_dnode_change_leaf(if_dnode, vrf->name); + running_config->version++; + } } + pthread_rwlock_unlock(&running_config->lock); } } - /* Delete interface structure. */ void if_delete_retain(struct interface *ifp) { diff --git a/lib/libfrr.c b/lib/libfrr.c index 0d4c8d6c0f..5970e70a6b 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -830,7 +830,12 @@ static int frr_config_read_in(struct thread *t) /* * Update the shared candidate after reading the startup configuration. */ - nb_config_replace(vty_shared_candidate_config, running_config, true); + pthread_rwlock_rdlock(&running_config->lock); + { + nb_config_replace(vty_shared_candidate_config, running_config, + true); + } + pthread_rwlock_unlock(&running_config->lock); return 0; } diff --git a/lib/northbound.c b/lib/northbound.c index 58dcc02ac3..6c68772cf8 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -264,6 +264,7 @@ struct nb_config *nb_config_new(struct lyd_node *dnode) else config->dnode = yang_dnode_new(ly_native_ctx, true); config->version = 0; + pthread_rwlock_init(&config->lock, NULL); return config; } @@ -272,6 +273,7 @@ void nb_config_free(struct nb_config *config) { if (config->dnode) yang_dnode_free(config->dnode); + pthread_rwlock_destroy(&config->lock); XFREE(MTYPE_NB_CONFIG, config); } @@ -282,6 +284,7 @@ struct nb_config *nb_config_dup(const struct nb_config *config) dup = XCALLOC(MTYPE_NB_CONFIG, sizeof(*dup)); dup->dnode = yang_dnode_dup(config->dnode); dup->version = config->version; + pthread_rwlock_init(&dup->lock, NULL); return dup; } @@ -529,17 +532,28 @@ int nb_candidate_edit(struct nb_config *candidate, bool nb_candidate_needs_update(const struct nb_config *candidate) { - if (candidate->version < running_config->version) - return true; + bool ret = false; + + pthread_rwlock_rdlock(&running_config->lock); + { + if (candidate->version < running_config->version) + ret = true; + } + pthread_rwlock_unlock(&running_config->lock); - return false; + return ret; } int nb_candidate_update(struct nb_config *candidate) { struct nb_config *updated_config; - updated_config = nb_config_dup(running_config); + pthread_rwlock_rdlock(&running_config->lock); + { + updated_config = nb_config_dup(running_config); + } + pthread_rwlock_unlock(&running_config->lock); + if (nb_config_merge(updated_config, candidate, true) != NB_OK) return NB_ERR; @@ -591,9 +605,13 @@ int nb_candidate_validate(struct nb_config *candidate) return NB_ERR_VALIDATION; RB_INIT(nb_config_cbs, &changes); - nb_config_diff(running_config, candidate, &changes); - ret = nb_candidate_validate_changes(candidate, &changes); - nb_config_diff_del_changes(&changes); + pthread_rwlock_rdlock(&running_config->lock); + { + nb_config_diff(running_config, candidate, &changes); + ret = nb_candidate_validate_changes(candidate, &changes); + nb_config_diff_del_changes(&changes); + } + pthread_rwlock_unlock(&running_config->lock); return ret; } @@ -613,26 +631,36 @@ int nb_candidate_commit_prepare(struct nb_config *candidate, } RB_INIT(nb_config_cbs, &changes); - nb_config_diff(running_config, candidate, &changes); - if (RB_EMPTY(nb_config_cbs, &changes)) - return NB_ERR_NO_CHANGES; + pthread_rwlock_rdlock(&running_config->lock); + { + nb_config_diff(running_config, candidate, &changes); + if (RB_EMPTY(nb_config_cbs, &changes)) { + pthread_rwlock_unlock(&running_config->lock); + return NB_ERR_NO_CHANGES; + } - if (nb_candidate_validate_changes(candidate, &changes) != NB_OK) { - flog_warn(EC_LIB_NB_CANDIDATE_INVALID, - "%s: failed to validate candidate configuration", - __func__); - nb_config_diff_del_changes(&changes); - return NB_ERR_VALIDATION; - } + if (nb_candidate_validate_changes(candidate, &changes) + != NB_OK) { + flog_warn( + EC_LIB_NB_CANDIDATE_INVALID, + "%s: failed to validate candidate configuration", + __func__); + nb_config_diff_del_changes(&changes); + pthread_rwlock_unlock(&running_config->lock); + return NB_ERR_VALIDATION; + } - *transaction = - nb_transaction_new(candidate, &changes, client, user, comment); - if (*transaction == NULL) { - flog_warn(EC_LIB_NB_TRANSACTION_CREATION_FAILED, - "%s: failed to create transaction", __func__); - nb_config_diff_del_changes(&changes); - return NB_ERR_LOCKED; + *transaction = nb_transaction_new(candidate, &changes, client, + user, comment); + if (*transaction == NULL) { + flog_warn(EC_LIB_NB_TRANSACTION_CREATION_FAILED, + "%s: failed to create transaction", __func__); + nb_config_diff_del_changes(&changes); + pthread_rwlock_unlock(&running_config->lock); + return NB_ERR_LOCKED; + } } + pthread_rwlock_unlock(&running_config->lock); return nb_transaction_process(NB_EV_PREPARE, *transaction); } @@ -651,7 +679,11 @@ void nb_candidate_commit_apply(struct nb_transaction *transaction, /* Replace running by candidate. */ transaction->config->version++; - nb_config_replace(running_config, transaction->config, true); + pthread_rwlock_wrlock(&running_config->lock); + { + nb_config_replace(running_config, transaction->config, true); + } + pthread_rwlock_unlock(&running_config->lock); /* Record transaction. */ if (save_transaction @@ -931,40 +963,52 @@ static int nb_transaction_process(enum nb_event event, { struct nb_config_cb *cb; - RB_FOREACH (cb, nb_config_cbs, &transaction->changes) { - struct nb_config_change *change = (struct nb_config_change *)cb; - int ret; - - /* - * Only try to release resources that were allocated - * successfully. - */ - if (event == NB_EV_ABORT && change->prepare_ok == false) - break; + /* + * Need to lock the running configuration since transaction->changes + * can contain pointers to data nodes from the running configuration. + */ + pthread_rwlock_rdlock(&running_config->lock); + { + RB_FOREACH (cb, nb_config_cbs, &transaction->changes) { + struct nb_config_change *change = + (struct nb_config_change *)cb; + int ret; - /* Call the appropriate callback. */ - ret = nb_callback_configuration(event, change); - switch (event) { - case NB_EV_PREPARE: - if (ret != NB_OK) - return ret; - change->prepare_ok = true; - break; - case NB_EV_ABORT: - case NB_EV_APPLY: /* - * At this point it's not possible to reject the - * transaction anymore, so any failure here can lead to - * inconsistencies and should be treated as a bug. - * Operations prone to errors, like validations and - * resource allocations, should be performed during the - * 'prepare' phase. + * Only try to release resources that were allocated + * successfully. */ - break; - default: - break; + if (event == NB_EV_ABORT && change->prepare_ok == false) + break; + + /* Call the appropriate callback. */ + ret = nb_callback_configuration(event, change); + switch (event) { + case NB_EV_PREPARE: + if (ret != NB_OK) { + pthread_rwlock_unlock( + &running_config->lock); + return ret; + } + change->prepare_ok = true; + break; + case NB_EV_ABORT: + case NB_EV_APPLY: + /* + * At this point it's not possible to reject the + * transaction anymore, so any failure here can + * lead to inconsistencies and should be treated + * as a bug. Operations prone to errors, like + * validations and resource allocations, should + * be performed during the 'prepare' phase. + */ + break; + default: + break; + } } } + pthread_rwlock_unlock(&running_config->lock); return NB_OK; } diff --git a/lib/northbound.h b/lib/northbound.h index dca33b905f..909bb08ebe 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -422,8 +422,19 @@ enum nb_client { /* Northbound configuration. */ struct nb_config { + /* Configuration data. */ struct lyd_node *dnode; + + /* Configuration version. */ uint32_t version; + + /* + * Lock protecting this structure. The use of this lock is always + * necessary when reading or modifying the global running configuration. + * For candidate configurations, use of this lock is optional depending + * on the threading scheme of the northbound plugin. + */ + pthread_rwlock_t lock; }; /* Northbound configuration callback. */ diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index cd79c0608e..ae1b0578a0 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -193,8 +193,13 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) "Please check the logs for more details.\n"); /* Regenerate candidate for consistency. */ - nb_config_replace(vty->candidate_config, running_config, - true); + pthread_rwlock_rdlock(&running_config->lock); + { + nb_config_replace(vty->candidate_config, + running_config, true); + } + pthread_rwlock_unlock(&running_config->lock); + return CMD_WARNING_CONFIG_FAILED; } } @@ -302,7 +307,12 @@ static int nb_cli_commit(struct vty *vty, bool force, /* "confirm" parameter. */ if (confirmed_timeout) { - vty->confirmed_commit_rollback = nb_config_dup(running_config); + pthread_rwlock_rdlock(&running_config->lock); + { + vty->confirmed_commit_rollback = + nb_config_dup(running_config); + } + pthread_rwlock_unlock(&running_config->lock); vty->t_confirmed_commit_timeout = NULL; thread_add_timer(master, nb_cli_confirmed_commit_timeout, vty, @@ -316,8 +326,13 @@ static int nb_cli_commit(struct vty *vty, bool force, /* Map northbound return code to CLI return code. */ switch (ret) { case NB_OK: - nb_config_replace(vty->candidate_config_base, running_config, - true); + pthread_rwlock_rdlock(&running_config->lock); + { + nb_config_replace(vty->candidate_config_base, + running_config, true); + } + pthread_rwlock_unlock(&running_config->lock); + vty_out(vty, "%% Configuration committed successfully (Transaction ID #%u).\n\n", transaction_id); @@ -682,7 +697,12 @@ DEFPY (config_update, return CMD_WARNING; } - nb_config_replace(vty->candidate_config_base, running_config, true); + pthread_rwlock_rdlock(&running_config->lock); + { + nb_config_replace(vty->candidate_config_base, running_config, + true); + } + pthread_rwlock_unlock(&running_config->lock); vty_out(vty, "%% Candidate configuration updated successfully.\n\n"); @@ -782,8 +802,12 @@ DEFPY (show_config_running, } } - nb_cli_show_config(vty, running_config, format, translator, - !!with_defaults); + pthread_rwlock_rdlock(&running_config->lock); + { + nb_cli_show_config(vty, running_config, format, translator, + !!with_defaults); + } + pthread_rwlock_unlock(&running_config->lock); return CMD_SUCCESS; } @@ -897,57 +921,68 @@ DEFPY (show_config_compare, struct nb_config *config2, *config_transaction2 = NULL; int ret = CMD_WARNING; - if (c1_candidate) - config1 = vty->candidate_config; - else if (c1_running) - config1 = running_config; - else { - config_transaction1 = nb_db_transaction_load(c1_tid); - if (!config_transaction1) { - vty_out(vty, "%% Transaction %u does not exist\n\n", - (unsigned int)c1_tid); - goto exit; + /* + * For simplicity, lock the running configuration regardless if it's + * going to be used or not. + */ + pthread_rwlock_rdlock(&running_config->lock); + { + if (c1_candidate) + config1 = vty->candidate_config; + else if (c1_running) + config1 = running_config; + else { + config_transaction1 = nb_db_transaction_load(c1_tid); + if (!config_transaction1) { + vty_out(vty, + "%% Transaction %u does not exist\n\n", + (unsigned int)c1_tid); + goto exit; + } + config1 = config_transaction1; } - config1 = config_transaction1; - } - if (c2_candidate) - config2 = vty->candidate_config; - else if (c2_running) - config2 = running_config; - else { - config_transaction2 = nb_db_transaction_load(c2_tid); - if (!config_transaction2) { - vty_out(vty, "%% Transaction %u does not exist\n\n", - (unsigned int)c2_tid); - goto exit; + if (c2_candidate) + config2 = vty->candidate_config; + else if (c2_running) + config2 = running_config; + else { + config_transaction2 = nb_db_transaction_load(c2_tid); + if (!config_transaction2) { + vty_out(vty, + "%% Transaction %u does not exist\n\n", + (unsigned int)c2_tid); + goto exit; + } + config2 = config_transaction2; } - config2 = config_transaction2; - } - if (json) - format = NB_CFG_FMT_JSON; - else if (xml) - format = NB_CFG_FMT_XML; - else - format = NB_CFG_FMT_CMDS; + if (json) + format = NB_CFG_FMT_JSON; + else if (xml) + format = NB_CFG_FMT_XML; + else + format = NB_CFG_FMT_CMDS; - if (translator_family) { - translator = yang_translator_find(translator_family); - if (!translator) { - vty_out(vty, "%% Module translator \"%s\" not found\n", - translator_family); - goto exit; + if (translator_family) { + translator = yang_translator_find(translator_family); + if (!translator) { + vty_out(vty, + "%% Module translator \"%s\" not found\n", + translator_family); + goto exit; + } } - } - ret = nb_cli_show_config_compare(vty, config1, config2, format, - translator); -exit: - if (config_transaction1) - nb_config_free(config_transaction1); - if (config_transaction2) - nb_config_free(config_transaction2); + ret = nb_cli_show_config_compare(vty, config1, config2, format, + translator); + exit: + if (config_transaction1) + nb_config_free(config_transaction1); + if (config_transaction2) + nb_config_free(config_transaction2); + } + pthread_rwlock_unlock(&running_config->lock); return ret; } diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c index 2fc3c81cf2..e9669fc7e1 100644 --- a/lib/northbound_confd.c +++ b/lib/northbound_confd.c @@ -289,7 +289,11 @@ static int frr_confd_cdb_read_cb_prepare(int fd, int *subp, int reslen) struct cdb_iter_args iter_args; int ret; - candidate = nb_config_dup(running_config); + pthread_rwlock_rdlock(&running_config->lock); + { + candidate = nb_config_dup(running_config); + } + pthread_rwlock_unlock(&running_config->lock); /* Iterate over all configuration changes. */ iter_args.candidate = candidate; diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index e21f57d37e..44a55137f8 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -256,7 +256,11 @@ static int frr_sr_config_change_cb_verify(sr_session_ctx_t *session, return ret; } - candidate = nb_config_dup(running_config); + pthread_rwlock_rdlock(&running_config->lock); + { + candidate = nb_config_dup(running_config); + } + pthread_rwlock_unlock(&running_config->lock); while ((ret = sr_get_change_next(session, it, &sr_op, &sr_old_val, &sr_new_val)) @@ -2608,17 +2608,22 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive) vty->private_config = private_config; vty->xpath_index = 0; - if (private_config) { - vty->candidate_config = nb_config_dup(running_config); - vty->candidate_config_base = nb_config_dup(running_config); - vty_out(vty, - "Warning: uncommitted changes will be discarded on exit.\n\n"); - } else { - vty->candidate_config = vty_shared_candidate_config; - if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL) + pthread_rwlock_rdlock(&running_config->lock); + { + if (private_config) { + vty->candidate_config = nb_config_dup(running_config); vty->candidate_config_base = nb_config_dup(running_config); + vty_out(vty, + "Warning: uncommitted changes will be discarded on exit.\n\n"); + } else { + vty->candidate_config = vty_shared_candidate_config; + if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL) + vty->candidate_config_base = + nb_config_dup(running_config); + } } + pthread_rwlock_unlock(&running_config->lock); return CMD_SUCCESS; } |