From 9bde0b256919ff3987ea60101229fec195324102 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Mon, 19 Oct 2020 23:56:54 -0300 Subject: [PATCH 1/3] 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; From 8d869d378bf10ee02e4aed66c9c3c16f15fe0c26 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 22 Oct 2020 22:19:10 -0300 Subject: [PATCH 2/3] lib: combine two YANG schema iteration functions into one Combine yang_snodes_iterate_module() and yang_snodes_iterate_all() into an unified yang_snodes_iterate() function, where the first "module" parameter is optional. There's no point in having two separate YANG schema iteration functions anymore now that they are too similar. Signed-off-by: Renato Westphal --- lib/northbound.c | 6 +++--- lib/northbound_confd.c | 4 ++-- lib/northbound_sysrepo.c | 8 ++++---- lib/yang.c | 23 ++--------------------- lib/yang.h | 28 +++++----------------------- lib/yang_translator.c | 22 +++++++++++----------- tools/gen_northbound_callbacks.c | 7 +++---- tools/gen_yang_deviations.c | 4 ++-- 8 files changed, 32 insertions(+), 70 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index e79ccc7dd0..a978c7fe78 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -162,12 +162,12 @@ static int nb_node_del_cb(const struct lys_node *snode, void *arg) void nb_nodes_create(void) { - yang_snodes_iterate_all(nb_node_new_cb, 0, NULL); + yang_snodes_iterate(NULL, nb_node_new_cb, 0, NULL); } void nb_nodes_delete(void) { - yang_snodes_iterate_all(nb_node_del_cb, 0, NULL); + yang_snodes_iterate(NULL, nb_node_del_cb, 0, NULL); } struct nb_node *nb_node_find(const char *xpath) @@ -2254,7 +2254,7 @@ void nb_init(struct thread_master *tm, nb_load_callbacks(modules[i]); /* Validate northbound callbacks. */ - yang_snodes_iterate_all(nb_node_validate, 0, &errors); + yang_snodes_iterate(NULL, nb_node_validate, 0, &errors); if (errors > 0) { flog_err( EC_LIB_NB_CBS_VALIDATION, diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c index a53e90d558..8acba9fd2b 100644 --- a/lib/northbound_confd.c +++ b/lib/northbound_confd.c @@ -1279,7 +1279,7 @@ static int frr_confd_init_dp(const char *program_name) * Iterate over all loaded YANG modules and subscribe to the paths * referent to state data. */ - yang_snodes_iterate_all(frr_confd_subscribe_state, 0, &data_cbs); + yang_snodes_iterate(NULL, frr_confd_subscribe_state, 0, &data_cbs); /* Register notification stream. */ memset(&ncbs, 0, sizeof(ncbs)); @@ -1430,7 +1430,7 @@ static int frr_confd_init(const char *program_name) goto error; } - yang_snodes_iterate_all(frr_confd_calculate_snode_hash, 0, NULL); + yang_snodes_iterate(NULL, frr_confd_calculate_snode_hash, 0, NULL); hook_register(nb_notification_send, frr_confd_notification_send); diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index e9aac3ef14..c027f4de72 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -690,10 +690,10 @@ static int frr_sr_init(void) int event_pipe; frr_sr_subscribe_config(module); - yang_snodes_iterate_module(module->info, frr_sr_subscribe_state, - 0, module); - yang_snodes_iterate_module(module->info, frr_sr_subscribe_rpc, - 0, module); + yang_snodes_iterate(module->info, frr_sr_subscribe_state, 0, + module); + yang_snodes_iterate(module->info, frr_sr_subscribe_rpc, 0, + module); /* Watch subscriptions. */ ret = sr_get_event_pipe(module->sr_subscription, &event_pipe); diff --git a/lib/yang.c b/lib/yang.c index 46012acf51..c59cb642f0 100644 --- a/lib/yang.c +++ b/lib/yang.c @@ -227,8 +227,8 @@ next: return ret; } -int yang_snodes_iterate_module(const struct lys_module *module, - yang_iterate_cb cb, uint16_t flags, void *arg) +int yang_snodes_iterate(const struct lys_module *module, yang_iterate_cb cb, + uint16_t flags, void *arg) { const struct lys_module *module_iter; uint32_t idx = 0; @@ -252,25 +252,6 @@ int yang_snodes_iterate_module(const struct lys_module *module, return ret; } -int yang_snodes_iterate_all(yang_iterate_cb cb, uint16_t flags, void *arg) -{ - struct yang_module *module; - int ret = YANG_ITER_CONTINUE; - - RB_FOREACH (module, yang_modules, &yang_modules) { - struct lys_node *snode; - - LY_TREE_FOR (module->info->data, snode) { - ret = yang_snodes_iterate_subtree(snode, NULL, cb, - flags, arg); - if (ret == YANG_ITER_STOP) - return ret; - } - } - - return ret; -} - void yang_snode_get_path(const struct lys_node *snode, enum yang_path_type type, char *xpath, size_t xpath_len) { diff --git a/lib/yang.h b/lib/yang.h index 867ade9676..0cd6a4a6f2 100644 --- a/lib/yang.h +++ b/lib/yang.h @@ -186,10 +186,11 @@ extern int yang_snodes_iterate_subtree(const struct lys_node *snode, void *arg); /* - * Iterate over all libyang schema nodes from the given YANG module. + * Iterate over all libyang schema nodes from all loeaded modules of from the + * given YANG module. * * module - * YANG module to operate on. + * When set, iterate over all nodes of the specified module only. * * cb * Function to call with each schema node. @@ -203,27 +204,8 @@ extern int yang_snodes_iterate_subtree(const struct lys_node *snode, * Returns: * The return value of the last called callback. */ -extern int yang_snodes_iterate_module(const struct lys_module *module, - yang_iterate_cb cb, uint16_t flags, - void *arg); - -/* - * Iterate over all libyang schema nodes from all loaded YANG modules. - * - * cb - * Function to call with each schema node. - * - * flags - * YANG_ITER_* flags to control how the iteration is performed. - * - * arg - * Arbitrary argument passed as the second parameter in each call to 'cb'. - * - * Returns: - * The return value of the last called callback. - */ -extern int yang_snodes_iterate_all(yang_iterate_cb cb, uint16_t flags, - void *arg); +extern int yang_snodes_iterate(const struct lys_module *module, + yang_iterate_cb cb, uint16_t flags, void *arg); /* * Build schema path or data path of the schema node. diff --git a/lib/yang_translator.c b/lib/yang_translator.c index 7dbb1f3f1a..1f64675d6a 100644 --- a/lib/yang_translator.c +++ b/lib/yang_translator.c @@ -469,12 +469,12 @@ static unsigned int yang_translator_validate(struct yang_translator *translator) args.errors = 0; for (ALL_LIST_ELEMENTS_RO(translator->modules, ln, tmodule)) { - yang_snodes_iterate_module( - tmodule->module, yang_translator_validate_cb, - YANG_ITER_FILTER_NPCONTAINERS - | YANG_ITER_FILTER_LIST_KEYS - | YANG_ITER_FILTER_INPUT_OUTPUT, - &args); + yang_snodes_iterate(tmodule->module, + yang_translator_validate_cb, + YANG_ITER_FILTER_NPCONTAINERS + | YANG_ITER_FILTER_LIST_KEYS + | YANG_ITER_FILTER_INPUT_OUTPUT, + &args); } if (args.errors) @@ -500,11 +500,11 @@ static unsigned int yang_module_nodes_count(const struct lys_module *module) { unsigned int total = 0; - yang_snodes_iterate_module(module, yang_module_nodes_count_cb, - YANG_ITER_FILTER_NPCONTAINERS - | YANG_ITER_FILTER_LIST_KEYS - | YANG_ITER_FILTER_INPUT_OUTPUT, - &total); + yang_snodes_iterate(module, yang_module_nodes_count_cb, + YANG_ITER_FILTER_NPCONTAINERS + | YANG_ITER_FILTER_LIST_KEYS + | YANG_ITER_FILTER_INPUT_OUTPUT, + &total); return total; } diff --git a/tools/gen_northbound_callbacks.c b/tools/gen_northbound_callbacks.c index eaab932228..a785f43cdf 100644 --- a/tools/gen_northbound_callbacks.c +++ b/tools/gen_northbound_callbacks.c @@ -368,13 +368,12 @@ int main(int argc, char *argv[]) /* Generate callback prototypes. */ if (!static_cbs) { printf("/* prototypes */\n"); - yang_snodes_iterate_module(module->info, generate_prototypes, 0, - NULL); + yang_snodes_iterate(module->info, generate_prototypes, 0, NULL); printf("\n"); } /* Generate callback functions. */ - yang_snodes_iterate_module(module->info, generate_callbacks, 0, NULL); + yang_snodes_iterate(module->info, generate_callbacks, 0, NULL); strlcpy(module_name_underscores, module->name, sizeof(module_name_underscores)); @@ -386,7 +385,7 @@ int main(int argc, char *argv[]) "\t.name = \"%s\",\n" "\t.nodes = {\n", module_name_underscores, module->name); - yang_snodes_iterate_module(module->info, generate_nb_nodes, 0, NULL); + yang_snodes_iterate(module->info, generate_nb_nodes, 0, NULL); printf("\t\t{\n" "\t\t\t.xpath = NULL,\n" "\t\t},\n"); diff --git a/tools/gen_yang_deviations.c b/tools/gen_yang_deviations.c index f908e1fc69..53a53c943c 100644 --- a/tools/gen_yang_deviations.c +++ b/tools/gen_yang_deviations.c @@ -71,8 +71,8 @@ int main(int argc, char *argv[]) module = yang_module_load(argv[0]); /* Generate deviations. */ - yang_snodes_iterate_module(module->info, generate_yang_deviation, - YANG_ITER_FILTER_IMPLICIT, NULL); + yang_snodes_iterate(module->info, generate_yang_deviation, + YANG_ITER_FILTER_IMPLICIT, NULL); /* Cleanup and exit. */ yang_terminate(); From 59e85ca1baa61757b6d857d5559aa5a196e96ede Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Tue, 20 Oct 2020 00:20:48 -0300 Subject: [PATCH 3/3] lib: add API to load YANG modules on demand Make it possible to load YANG modules outside the main northbound initialization. The primary use case is to support YANG modules that are specific to an FRR plugin. Example: only load the PCEP YANG module when the corresponding FRR plugin is loaded. Other use cases might arise in the future. Signed-off-by: Renato Westphal --- lib/northbound.c | 40 +++++++++++++++++++++++++--------------- lib/northbound.h | 23 +++++++++++++++++++++++ 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index a978c7fe78..ecfa2c9d11 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -2236,24 +2236,10 @@ static void nb_load_callbacks(const struct frr_yang_module_info *module) } } -void nb_init(struct thread_master *tm, - const struct frr_yang_module_info *const modules[], - size_t nmodules, bool db_enabled) +void nb_validate_callbacks(void) { unsigned int errors = 0; - /* Load YANG modules. */ - for (size_t i = 0; i < nmodules; i++) - yang_module_load(modules[i]->name); - - /* Create a nb_node for all YANG schema nodes. */ - nb_nodes_create(); - - /* Load northbound callbacks. */ - for (size_t i = 0; i < nmodules; i++) - nb_load_callbacks(modules[i]); - - /* Validate northbound callbacks. */ yang_snodes_iterate(NULL, nb_node_validate, 0, &errors); if (errors > 0) { flog_err( @@ -2262,9 +2248,33 @@ void nb_init(struct thread_master *tm, __func__, errors); exit(1); } +} +void nb_load_module(const struct frr_yang_module_info *module_info) +{ + struct yang_module *module; + + DEBUGD(&nb_dbg_events, "northbound: loading %s.yang", + module_info->name); + + module = yang_module_load(module_info->name); + yang_snodes_iterate(module->info, nb_node_new_cb, 0, NULL); + nb_load_callbacks(module_info); +} + +void nb_init(struct thread_master *tm, + const struct frr_yang_module_info *const modules[], + size_t nmodules, bool db_enabled) +{ nb_db_enabled = db_enabled; + /* Load YANG modules and their corresponding northbound callbacks. */ + for (size_t i = 0; i < nmodules; i++) + nb_load_module(modules[i]); + + /* Validate northbound callbacks. */ + nb_validate_callbacks(); + /* Create an empty running configuration. */ running_config = nb_config_new(NULL); running_config_entries = hash_create(running_config_entry_key_make, diff --git a/lib/northbound.h b/lib/northbound.h index 16f19b3d5b..3f6e4dc46e 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -1237,6 +1237,29 @@ extern const char *nb_err_name(enum nb_error error); */ extern const char *nb_client_name(enum nb_client client); +/* + * Validate all northbound callbacks. + * + * Some errors, like missing callbacks or invalid priorities, are fatal and + * can't be recovered from. Other errors, like unneeded callbacks, are logged + * but otherwise ignored. + * + * Whenever a YANG module is loaded after startup, *all* northbound callbacks + * need to be validated and not only the callbacks from the newly loaded module. + * This is because augmentations can change the properties of the augmented + * module, making mandatory the implementation of additional callbacks. + */ +void nb_validate_callbacks(void); + +/* + * Load a YANG module with its corresponding northbound callbacks. + * + * module_info + * Pointer to structure containing the module name and its northbound + * callbacks. + */ +void nb_load_module(const struct frr_yang_module_info *module_info); + /* * Initialize the northbound layer. Should be called only once during the * daemon initialization process.