From f05a4e3b577b50f20b85ae95e53222c133bc7b1f Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Fri, 19 Jan 2024 15:25:57 +0000 Subject: [PATCH 1/3] lib: libyang logging temp off to avoid unwanted log message We don't want libyang logging when an schema path doesn't exist since this is an acceptable outcome. Signed-off-by: Christian Hopps --- lib/northbound.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/northbound.c b/lib/northbound.c index 42e4ebbcc9..ea34c9cc03 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -157,12 +157,21 @@ void nb_nodes_delete(void) struct nb_node *nb_node_find(const char *path) { const struct lysc_node *snode; + uint32_t llopts; /* * Use libyang to find the schema node associated to the path and get - * the northbound node from there (snode private pointer). + * the northbound node from there (snode private pointer). We need to + * disable logging temporarily to avoid libyang from logging an error + * message when the node is not found. */ + llopts = ly_log_options(LY_LOSTORE); + llopts &= ~LY_LOLOG; + ly_temp_log_options(&llopts); + snode = yang_find_snode(ly_native_ctx, path, 0); + + ly_temp_log_options(NULL); if (!snode) return NULL; From 9e34d817fcccd29115c2fef0c9199cca24b631fd Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Thu, 18 Jan 2024 04:17:35 +0000 Subject: [PATCH 2/3] lib: better conditionalize leaf-list predicate xpath addition If we're in the backend we already have the predicate added by mgmtd -- don't add it again. Signed-off-by: Christian Hopps --- lib/mgmt_be_client.c | 2 +- lib/northbound.c | 27 +++++++++++++++++---------- lib/northbound.h | 18 +++++++++++++----- lib/northbound_cli.c | 6 +++--- lib/northbound_confd.c | 2 +- lib/northbound_sysrepo.c | 2 +- mgmtd/mgmt_txn.c | 4 ++-- 7 files changed, 38 insertions(+), 23 deletions(-) diff --git a/lib/mgmt_be_client.c b/lib/mgmt_be_client.c index 16aea249a4..d50dd03fdc 100644 --- a/lib/mgmt_be_client.c +++ b/lib/mgmt_be_client.c @@ -449,7 +449,7 @@ static int mgmt_be_txn_cfg_prepare(struct mgmt_be_txn_ctx *txn) client_ctx->candidate_config, txn_req->req.set_cfg.cfg_changes, (size_t)txn_req->req.set_cfg.num_cfg_changes, - NULL, err_buf, sizeof(err_buf), &error); + NULL, true, err_buf, sizeof(err_buf), &error); if (error) { err_buf[sizeof(err_buf) - 1] = 0; MGMTD_BE_CLIENT_ERR( diff --git a/lib/northbound.c b/lib/northbound.c index ea34c9cc03..1deeb36fd1 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -694,10 +694,9 @@ static int dnode_create(struct nb_config *candidate, const char *xpath, return NB_OK; } -int nb_candidate_edit(struct nb_config *candidate, - const struct nb_node *nb_node, +int nb_candidate_edit(struct nb_config *candidate, const struct nb_node *nb_node, enum nb_operation operation, const char *xpath, - const struct yang_data *previous, + bool in_backend, const struct yang_data *previous, const struct yang_data *data) { struct lyd_node *dnode, *dep_dnode, *old_dnode, *parent; @@ -706,8 +705,13 @@ int nb_candidate_edit(struct nb_config *candidate, uint32_t options = 0; LY_ERR err; - /* Use special notation for leaf-lists (RFC 6020, section 9.13.5). */ - if (nb_node->snode->nodetype == LYS_LEAFLIST) + /* + * Use special notation for leaf-lists (RFC 6020, section 9.13.5). + * if we are in a backend client this notation was already applied + * by mgmtd before sending to us. + */ + if (!in_backend && nb_node->snode->nodetype == LYS_LEAFLIST && + (operation == NB_OP_DESTROY || operation == NB_OP_DELETE)) snprintf(xpath_edit, sizeof(xpath_edit), "%s[.='%s']", xpath, data->value); else @@ -838,10 +842,12 @@ bool nb_is_operation_allowed(struct nb_node *nb_node, enum nb_operation oper) return true; } -void nb_candidate_edit_config_changes( - struct nb_config *candidate_config, struct nb_cfg_change cfg_changes[], - size_t num_cfg_changes, const char *xpath_base, char *err_buf, - int err_bufsize, bool *error) +void nb_candidate_edit_config_changes(struct nb_config *candidate_config, + struct nb_cfg_change cfg_changes[], + size_t num_cfg_changes, + const char *xpath_base, bool in_backend, + char *err_buf, int err_bufsize, + bool *error) { if (error) *error = false; @@ -895,7 +901,8 @@ void nb_candidate_edit_config_changes( * configuration. */ ret = nb_candidate_edit(candidate_config, nb_node, - change->operation, xpath, NULL, data); + change->operation, xpath, in_backend, + NULL, data); yang_data_free(data); if (ret != NB_OK) { flog_warn( diff --git a/lib/northbound.h b/lib/northbound.h index 4f9ab565d9..482a055965 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -950,6 +950,9 @@ extern bool nb_is_operation_allowed(struct nb_node *nb_node, * xpath * XPath of the configuration node being edited. * + * in_backend + * Specify whether the changes are being applied in the backend or not. + * * previous * Previous value of the configuration node. Should be used only when the * operation is NB_OP_MOVE, otherwise this parameter is ignored. @@ -964,7 +967,7 @@ extern bool nb_is_operation_allowed(struct nb_node *nb_node, extern int nb_candidate_edit(struct nb_config *candidate, const struct nb_node *nb_node, enum nb_operation operation, const char *xpath, - const struct yang_data *previous, + bool in_backend, const struct yang_data *previous, const struct yang_data *data); /* @@ -1009,6 +1012,9 @@ extern bool nb_candidate_needs_update(const struct nb_config *candidate); * xpath_base * Base xpath for config. * + * in_backend + * Specify whether the changes are being applied in the backend or not. + * * err_buf * Buffer to store human-readable error message in case of error. * @@ -1018,10 +1024,12 @@ extern bool nb_candidate_needs_update(const struct nb_config *candidate); * error * TRUE on error, FALSE on success */ -extern void nb_candidate_edit_config_changes( - struct nb_config *candidate_config, struct nb_cfg_change cfg_changes[], - size_t num_cfg_changes, const char *xpath_base, char *err_buf, - int err_bufsize, bool *error); +extern void nb_candidate_edit_config_changes(struct nb_config *candidate_config, + struct nb_cfg_change cfg_changes[], + size_t num_cfg_changes, + const char *xpath_base, + bool in_backend, char *err_buf, + int err_bufsize, bool *error); /* * Delete candidate configuration changes. diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index 92d4ffb2ba..0358a0f377 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -146,9 +146,9 @@ static int nb_cli_apply_changes_internal(struct vty *vty, VTY_CHECK_XPATH; - nb_candidate_edit_config_changes( - vty->candidate_config, vty->cfg_changes, vty->num_cfg_changes, - xpath_base, buf, sizeof(buf), &error); + nb_candidate_edit_config_changes(vty->candidate_config, vty->cfg_changes, + vty->num_cfg_changes, xpath_base, + false, buf, sizeof(buf), &error); if (error) { /* * Failure to edit the candidate configuration should never diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c index 8503d18002..c866b0afb4 100644 --- a/lib/northbound_confd.c +++ b/lib/northbound_confd.c @@ -256,7 +256,7 @@ frr_confd_cdb_diff_iter(confd_hkeypath_t *kp, enum cdb_iter_op cdb_op, /* Edit the candidate configuration. */ data = yang_data_new(xpath, value_str); ret = nb_candidate_edit(iter_args->candidate, nb_node, nb_op, xpath, - NULL, data); + false, NULL, data); yang_data_free(data); if (ret != NB_OK) { flog_warn( diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index 198d96e381..050477af91 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -219,7 +219,7 @@ static int frr_sr_process_change(struct nb_config *candidate, sr_val_to_buff(sr_data, value_str, sizeof(value_str)); data = yang_data_new(xpath, value_str); - ret = nb_candidate_edit(candidate, nb_node, nb_op, xpath, NULL, data); + ret = nb_candidate_edit(candidate, nb_node, nb_op, xpath, false, NULL, data); yang_data_free(data); if (ret != NB_OK) { flog_warn( diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 842e13cf11..b4b67b4551 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -587,8 +587,8 @@ static void mgmt_txn_process_set_cfg(struct event *thread) txn_req->req.set_cfg->cfg_changes, (size_t)txn_req->req.set_cfg ->num_cfg_changes, - NULL, err_buf, sizeof(err_buf), - &error); + NULL, false, err_buf, + sizeof(err_buf), &error); if (error) { mgmt_fe_send_set_cfg_reply(txn->session_id, txn->txn_id, txn_req->req.set_cfg->ds_id, From fa7ff16e2bf0e1cc4e8b7c61a087cab289749ab5 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Thu, 18 Jan 2024 17:06:45 +0000 Subject: [PATCH 3/3] mgmtd: remove heavy duplication in mgmtd config read Previously each container created all it's decendents before descending into the children and repeating the process. Signed-off-by: Christian Hopps --- lib/northbound.c | 25 +++++++++++------- lib/northbound.h | 5 ++++ mgmtd/mgmt_be_adapter.c | 56 ++++++++++++++++------------------------- mgmtd/mgmt_be_adapter.h | 4 +-- 4 files changed, 44 insertions(+), 46 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index 1deeb36fd1..a831fc58b2 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -417,10 +417,9 @@ static inline int nb_config_cb_compare(const struct nb_config_cb *a, } RB_GENERATE(nb_config_cbs, nb_config_cb, entry, nb_config_cb_compare); -static void nb_config_diff_add_change(struct nb_config_cbs *changes, - enum nb_cb_operation operation, - uint32_t *seq, - const struct lyd_node *dnode) +void nb_config_diff_add_change(struct nb_config_cbs *changes, + enum nb_cb_operation operation, uint32_t *seq, + const struct lyd_node *dnode) { struct nb_config_change *change; @@ -699,9 +698,10 @@ int nb_candidate_edit(struct nb_config *candidate, const struct nb_node *nb_node bool in_backend, const struct yang_data *previous, const struct yang_data *data) { - struct lyd_node *dnode, *dep_dnode, *old_dnode, *parent; + struct lyd_node *dnode, *dep_dnode, *old_dnode; char xpath_edit[XPATH_MAXLEN]; char dep_xpath[XPATH_MAXLEN]; + struct lyd_node *parent = NULL; uint32_t options = 0; LY_ERR err; @@ -876,10 +876,17 @@ void nb_candidate_edit_config_changes(struct nb_config *candidate_config, /* Find the northbound node associated to the data path. */ nb_node = nb_node_find(xpath); if (!nb_node) { - flog_warn(EC_LIB_YANG_UNKNOWN_DATA_PATH, - "%s: unknown data path: %s", __func__, xpath); - if (error) - *error = true; + if (in_backend) + DEBUGD(&nb_dbg_cbs_config, + "%s: ignoring non-handled path: %s", + __func__, xpath); + else { + flog_warn(EC_LIB_YANG_UNKNOWN_DATA_PATH, + "%s: unknown data path: %s", __func__, + xpath); + if (error) + *error = true; + } continue; } /* Find if the node to be edited is not a key node */ diff --git a/lib/northbound.h b/lib/northbound.h index 482a055965..0a6bc88921 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -1031,6 +1031,11 @@ extern void nb_candidate_edit_config_changes(struct nb_config *candidate_config, bool in_backend, char *err_buf, int err_bufsize, bool *error); + +extern void nb_config_diff_add_change(struct nb_config_cbs *changes, + enum nb_cb_operation operation, + uint32_t *seq, + const struct lyd_node *dnode); /* * Delete candidate configuration changes. * diff --git a/mgmtd/mgmt_be_adapter.c b/mgmtd/mgmt_be_adapter.c index 2be70901a8..c8f52a101c 100644 --- a/mgmtd/mgmt_be_adapter.c +++ b/mgmtd/mgmt_be_adapter.c @@ -597,20 +597,6 @@ struct mgmt_be_get_adapter_config_params { uint32_t seq; }; -/* - * Callback to store the change a node in the datastore if it should be sync'd - * to the adapter (i.e., if the adapter is subscribed to it). - */ -static void mgmt_be_iter_and_get_cfg(const char *xpath, struct lyd_node *node, - struct nb_node *nb_node, void *ctx) -{ - struct mgmt_be_get_adapter_config_params *parms = ctx; - struct mgmt_be_client_adapter *adapter = parms->adapter; - - if (be_is_client_interested(xpath, adapter->id, true)) - nb_config_diff_created(node, &parms->seq, parms->cfg_chgs); -} - /* * Initialize a BE client over a new connection */ @@ -769,32 +755,32 @@ void mgmt_be_adapter_toggle_client_debug(bool set) * Get a full set of changes for all the config that an adapter is subscribed to * receive. */ -int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, - struct nb_config_cbs **cfg_chgs) +void mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, + struct nb_config_cbs **changes) { - struct mgmt_be_get_adapter_config_params parms; - struct nb_config *cfg_root = mgmt_ds_get_nb_config(mm->running_ds); + const struct lyd_node *root, *dnode; + uint32_t seq = 0; + char *xpath; - assert(cfg_chgs); + /* We can't be in the middle of sending other chgs when here. */ + assert(RB_EMPTY(nb_config_cbs, &adapter->cfg_chgs)); - /* - * TODO: we should consider making this an assertable condition and - * guaranteeing it be true when this function is called. B/c what is - * going to happen if there are some changes being sent, and we don't - * gather a new snapshot, what new changes that came after the previous - * snapshot will then be lost? - */ - if (RB_EMPTY(nb_config_cbs, &adapter->cfg_chgs)) { - parms.adapter = adapter; - parms.cfg_chgs = &adapter->cfg_chgs; - parms.seq = 0; + *changes = &adapter->cfg_chgs; + LY_LIST_FOR (running_config->dnode, root) { + LYD_TREE_DFS_BEGIN (root, dnode) { + if (lysc_is_key(dnode->schema)) + goto walk_cont; - mgmt_ds_iter_data(MGMTD_DS_RUNNING, cfg_root, "", - mgmt_be_iter_and_get_cfg, (void *)&parms); + xpath = lyd_path(dnode, LYD_PATH_STD, NULL, 0); + if (be_is_client_interested(xpath, adapter->id, true)) + nb_config_diff_add_change(*changes, NB_CB_CREATE, &seq, dnode); + else + LYD_TREE_DFS_continue = 1; /* skip any subtree */ + free(xpath); + walk_cont: + LYD_TREE_DFS_END(root, dnode); + } } - - *cfg_chgs = &adapter->cfg_chgs; - return 0; } uint64_t mgmt_be_interested_clients(const char *xpath, bool config) diff --git a/mgmtd/mgmt_be_adapter.h b/mgmtd/mgmt_be_adapter.h index 96e807f6c4..3407d4c6a7 100644 --- a/mgmtd/mgmt_be_adapter.h +++ b/mgmtd/mgmt_be_adapter.h @@ -157,8 +157,8 @@ extern const char *mgmt_be_client_id2name(enum mgmt_be_client_id id); extern void mgmt_be_adapter_toggle_client_debug(bool set); /* Fetch backend adapter config. */ -extern int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, - struct nb_config_cbs **cfg_chgs); +extern void mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, + struct nb_config_cbs **changes); /* Create/destroy a transaction. */ extern int mgmt_be_send_txn_req(struct mgmt_be_client_adapter *adapter,