From 9bde0b256919ff3987ea60101229fec195324102 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Mon, 19 Oct 2020 23:56:54 -0300 Subject: [PATCH] lib: fix iteration over schema nodes of a single YANG module The only safe way to iterate over all schema nodes of a given YANG module is by iterating over all schema nodes of all YANG modules and filter out the nodes that belong to other modules. The original yang_snodes_iterate_module() code did the following: 1 - Iterate over all top-level schema nodes of the given module; 2 - Iterate over all augmentations of the given module. While that iteration strategy is more efficient, it does't handle well more complex YANG hierarchies containing nested augmentations or self-augmenting modules. Any iteration that isn't done on the resolved YANG data hierarchy is fragile and prone to errors. Fixes regression introduced by commit 8a923b48513316b where the gen_northbound_callbacks tool was generating duplicate callbacks for certain modules. Signed-off-by: Renato Westphal --- lib/northbound.c | 12 ++++++++---- lib/northbound_confd.c | 8 ++++++-- lib/northbound_sysrepo.c | 4 ++++ lib/yang.c | 27 ++++++++++++++------------- 4 files changed, 32 insertions(+), 19 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index c99f993ea5..e79ccc7dd0 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -152,8 +152,10 @@ static int nb_node_del_cb(const struct lys_node *snode, void *arg) struct nb_node *nb_node; nb_node = snode->priv; - lys_set_private(snode, NULL); - XFREE(MTYPE_NB_NODE, nb_node); + if (nb_node) { + lys_set_private(snode, NULL); + XFREE(MTYPE_NB_NODE, nb_node); + } return YANG_ITER_CONTINUE; } @@ -273,8 +275,10 @@ static int nb_node_validate(const struct lys_node *snode, void *arg) unsigned int *errors = arg; /* Validate callbacks and priority. */ - *errors += nb_node_validate_cbs(nb_node); - *errors += nb_node_validate_priority(nb_node); + if (nb_node) { + *errors += nb_node_validate_cbs(nb_node); + *errors += nb_node_validate_priority(nb_node); + } return YANG_ITER_CONTINUE; } diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c index c1cb0fc11d..a53e90d558 100644 --- a/lib/northbound_confd.c +++ b/lib/northbound_confd.c @@ -550,6 +550,9 @@ static int frr_confd_init_cdb(void) continue; nb_node = snode->priv; + if (!nb_node) + continue; + DEBUGD(&nb_dbg_client_confd, "%s: subscribing to '%s'", __func__, nb_node->xpath); @@ -1189,7 +1192,7 @@ static int frr_confd_subscribe_state(const struct lys_node *snode, void *arg) struct nb_node *nb_node = snode->priv; struct confd_data_cbs *data_cbs = arg; - if (!CHECK_FLAG(snode->flags, LYS_CONFIG_R)) + if (!nb_node || !CHECK_FLAG(snode->flags, LYS_CONFIG_R)) return YANG_ITER_CONTINUE; /* We only need to subscribe to the root of the state subtrees. */ if (snode->parent && CHECK_FLAG(snode->parent->flags, LYS_CONFIG_R)) @@ -1393,7 +1396,8 @@ static int frr_confd_calculate_snode_hash(const struct lys_node *snode, { struct nb_node *nb_node = snode->priv; - nb_node->confd_hash = confd_str2hash(snode->name); + if (nb_node) + nb_node->confd_hash = confd_str2hash(snode->name); return YANG_ITER_CONTINUE; } diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index 3cd310c5a7..e9aac3ef14 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -575,6 +575,8 @@ static int frr_sr_subscribe_state(const struct lys_node *snode, void *arg) return YANG_ITER_CONTINUE; nb_node = snode->priv; + if (!nb_node) + return YANG_ITER_CONTINUE; DEBUGD(&nb_dbg_client_sysrepo, "sysrepo: providing data to '%s'", nb_node->xpath); @@ -599,6 +601,8 @@ static int frr_sr_subscribe_rpc(const struct lys_node *snode, void *arg) return YANG_ITER_CONTINUE; nb_node = snode->priv; + if (!nb_node) + return YANG_ITER_CONTINUE; DEBUGD(&nb_dbg_client_sysrepo, "sysrepo: providing RPC to '%s'", nb_node->xpath); diff --git a/lib/yang.c b/lib/yang.c index 5bf7758e18..46012acf51 100644 --- a/lib/yang.c +++ b/lib/yang.c @@ -230,22 +230,23 @@ next: int yang_snodes_iterate_module(const struct lys_module *module, yang_iterate_cb cb, uint16_t flags, void *arg) { - struct lys_node *snode; + const struct lys_module *module_iter; + uint32_t idx = 0; int ret = YANG_ITER_CONTINUE; - LY_TREE_FOR (module->data, snode) { - ret = yang_snodes_iterate_subtree(snode, module, cb, flags, - arg); - if (ret == YANG_ITER_STOP) - return ret; - } + idx = ly_ctx_internal_modules_count(ly_native_ctx); + while ((module_iter = ly_ctx_get_module_iter(ly_native_ctx, &idx))) { + struct lys_node *snode; - for (uint8_t i = 0; i < module->augment_size; i++) { - ret = yang_snodes_iterate_subtree( - (const struct lys_node *)&module->augment[i], module, - cb, flags, arg); - if (ret == YANG_ITER_STOP) - return ret; + if (!module_iter->implemented) + continue; + + LY_TREE_FOR (module_iter->data, snode) { + ret = yang_snodes_iterate_subtree(snode, module, cb, + flags, arg); + if (ret == YANG_ITER_STOP) + return ret; + } } return ret; -- 2.39.5