From c7c9103b01899d316372cc0272c37df9a7e59426 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sat, 5 Oct 2019 21:20:28 -0300 Subject: [PATCH] lib: remove expensive error handling in the northbound CLI client The nb_cli_apply_changes() function was creating a full copy of the candidate configuration before editing it. This excerpt from the northbond documentation explains why this was being done: "NOTE: the nb_cli_cfg_change() function clones the candidate configuration before actually editing it. This way, if any error happens during the editing, the original candidate is restored to avoid inconsistencies. Either all changes from the configuration command are performed successfully or none are. It's like a mini-transaction but happening on the candidate configuration (thus the northbound callbacks are not involved)". The problem is that this kind of error handling is just too expensive. A command should never fail to edit the candidate configuration unless there's a bug in the code (e.g. when the CLI wrapper command passes an integer value that YANG rejects due to a "range" statement). In such cases, a command might fail to be applied or applied only partially if it edits multiple YANG nodes. When that happens, just log an error to make the operator aware of the problem, but otherwise ignore it instead of rejecting the command and restoring the candidate to its previous state. We shouldn't add an extreme overhead to the northbound CLI client only to handle errors that should never happen in practice. Signed-off-by: Renato Westphal --- lib/northbound_cli.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index a15fe3d1c9..a215d8b759 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -84,20 +84,12 @@ void nb_cli_enqueue_change(struct vty *vty, const char *xpath, int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) { - struct nb_config *candidate_transitory; char xpath_base[XPATH_MAXLEN] = {}; bool error = false; int ret; VTY_CHECK_XPATH; - /* - * Create a copy of the candidate configuration. For consistency, we - * need to ensure that either all changes made by the command are - * accepted or none are. - */ - candidate_transitory = nb_config_dup(vty->candidate_config); - /* Parse the base XPath format string. */ if (xpath_base_fmt) { va_list ap; @@ -137,7 +129,7 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) flog_warn(EC_LIB_YANG_UNKNOWN_DATA_PATH, "%s: unknown data path: %s", __func__, xpath); error = true; - break; + continue; } /* If the value is not set, get the default if it exists. */ @@ -149,7 +141,7 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) * Ignore "not found" errors when editing the candidate * configuration. */ - ret = nb_candidate_edit(candidate_transitory, nb_node, + ret = nb_candidate_edit(vty->candidate_config, nb_node, change->operation, xpath, NULL, data); yang_data_free(data); if (ret != NB_OK && ret != NB_ERR_NOT_FOUND) { @@ -159,29 +151,20 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) __func__, nb_operation_name(change->operation), xpath); error = true; - break; + continue; } } if (error) { - nb_config_free(candidate_transitory); - - switch (frr_get_cli_mode()) { - case FRR_CLI_CLASSIC: - vty_out(vty, "%% Configuration failed.\n\n"); - break; - case FRR_CLI_TRANSACTIONAL: - vty_out(vty, - "%% Failed to edit candidate configuration.\n\n"); - break; - } + /* + * Failure to edit the candidate configuration should never + * happen in practice, unless there's a bug in the code. When + * that happens, log the error but otherwise ignore it. + */ + vty_out(vty, "%% Failed to edit configuration.\n\n"); vty_show_libyang_errors(vty, ly_native_ctx); - - return CMD_WARNING_CONFIG_FAILED; } - nb_config_replace(vty->candidate_config, candidate_transitory, false); - /* Do an implicit "commit" when using the classic CLI mode. */ if (frr_get_cli_mode() == FRR_CLI_CLASSIC) { ret = nb_candidate_commit(vty->candidate_config, NB_CLIENT_CLI, -- 2.39.5