From f182d8d8a28fcb29c44f3775b9223ab9c9013c91 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Tue, 16 Feb 2021 12:49:25 +0300 Subject: [PATCH 1/4] lib: add ability to register dependencies between northbound nodes Signed-off-by: Igor Ryzhov --- lib/northbound.c | 60 ++++++++++++++++++++++++++++++++++++++++++++---- lib/northbound.h | 13 +++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index ecfa2c9d11..224951b22b 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -185,6 +185,25 @@ struct nb_node *nb_node_find(const char *xpath) return snode->priv; } +void nb_node_set_dependency_cbs(const char *dependency_xpath, + const char *dependant_xpath, + struct nb_dependency_callbacks *cbs) +{ + struct nb_node *dependency = nb_node_find(dependency_xpath); + struct nb_node *dependant = nb_node_find(dependant_xpath); + + if (!dependency || !dependant) + return; + + dependency->dep_cbs.get_dependant_xpath = cbs->get_dependant_xpath; + dependant->dep_cbs.get_dependency_xpath = cbs->get_dependency_xpath; +} + +bool nb_node_has_dependency(struct nb_node *node) +{ + return node->dep_cbs.get_dependency_xpath != NULL; +} + static int nb_node_validate_cb(const struct nb_node *nb_node, enum nb_operation operation, int callback_implemented, bool optional) @@ -532,8 +551,9 @@ int nb_candidate_edit(struct nb_config *candidate, const struct yang_data *previous, const struct yang_data *data) { - struct lyd_node *dnode; + struct lyd_node *dnode, *dep_dnode; char xpath_edit[XPATH_MAXLEN]; + char dep_xpath[XPATH_MAXLEN]; /* Use special notation for leaf-lists (RFC 6020, section 9.13.5). */ if (nb_node->snode->nodetype == LYS_LEAFLIST) @@ -549,9 +569,33 @@ int nb_candidate_edit(struct nb_config *candidate, dnode = lyd_new_path(candidate->dnode, ly_native_ctx, xpath_edit, (void *)data->value, 0, LYD_PATH_OPT_UPDATE); - if (!dnode && ly_errno) { - flog_warn(EC_LIB_LIBYANG, "%s: lyd_new_path() failed", - __func__); + if (dnode) { + /* + * create dependency + * + * dnode returned by the lyd_new_path may be from a + * different schema, so we need to update the nb_node + */ + nb_node = dnode->schema->priv; + if (nb_node->dep_cbs.get_dependency_xpath) { + nb_node->dep_cbs.get_dependency_xpath( + dnode, dep_xpath); + + ly_errno = 0; + dep_dnode = lyd_new_path(candidate->dnode, + ly_native_ctx, + dep_xpath, NULL, 0, + LYD_PATH_OPT_UPDATE); + if (!dep_dnode && ly_errno) { + flog_warn(EC_LIB_LIBYANG, + "%s: lyd_new_path(%s) failed", + __func__, dep_xpath); + return NB_ERR; + } + } + } else if (ly_errno) { + flog_warn(EC_LIB_LIBYANG, "%s: lyd_new_path(%s) failed", + __func__, xpath_edit); return NB_ERR; } break; @@ -563,6 +607,14 @@ int nb_candidate_edit(struct nb_config *candidate, * whether to ignore it or not. */ return NB_ERR_NOT_FOUND; + /* destroy dependant */ + if (nb_node->dep_cbs.get_dependant_xpath) { + nb_node->dep_cbs.get_dependant_xpath(dnode, dep_xpath); + + dep_dnode = yang_dnode_get(candidate->dnode, dep_xpath); + if (dep_dnode) + lyd_free(dep_dnode); + } lyd_free(dnode); break; case NB_OP_MOVE: diff --git a/lib/northbound.h b/lib/northbound.h index 8dd6b4c337..3e1342f985 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -509,6 +509,11 @@ struct nb_callbacks { void (*cli_show_end)(struct vty *vty, struct lyd_node *dnode); }; +struct nb_dependency_callbacks { + void (*get_dependant_xpath)(const struct lyd_node *dnode, char *xpath); + void (*get_dependency_xpath)(const struct lyd_node *dnode, char *xpath); +}; + /* * Northbound-specific data that is allocated for each schema node of the native * YANG modules. @@ -523,6 +528,8 @@ struct nb_node { /* Priority - lower priorities are processed first. */ uint32_t priority; + struct nb_dependency_callbacks dep_cbs; + /* Callbacks implemented for this node. */ struct nb_callbacks cbs; @@ -722,6 +729,12 @@ void nb_nodes_delete(void); */ extern struct nb_node *nb_node_find(const char *xpath); +extern void nb_node_set_dependency_cbs(const char *dependency_xpath, + const char *dependant_xpath, + struct nb_dependency_callbacks *cbs); + +bool nb_node_has_dependency(struct nb_node *node); + /* * Create a new northbound configuration. * From 3fa607bebacd4881053414d641e8376b4e4f6e83 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Mon, 15 Feb 2021 21:17:08 +0300 Subject: [PATCH 2/4] bgpd: don't rely on northbound control plane protocol vrf pointer Signed-off-by: Igor Ryzhov --- bgpd/bgp_nb_config.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bgpd/bgp_nb_config.c b/bgpd/bgp_nb_config.c index f2443bd164..697b94dcae 100644 --- a/bgpd/bgp_nb_config.c +++ b/bgpd/bgp_nb_config.c @@ -69,7 +69,7 @@ int bgp_router_create(struct nb_cb_create_args *args) { const struct lyd_node *vrf_dnode; struct bgp *bgp; - struct vrf *vrf; + const char *vrf_name; const char *name = NULL; as_t as; enum bgp_instance_type inst_type; @@ -87,12 +87,12 @@ int bgp_router_create(struct nb_cb_create_args *args) case NB_EV_APPLY: vrf_dnode = yang_dnode_get_parent(args->dnode, "control-plane-protocol"); - vrf = nb_running_get_entry(vrf_dnode, NULL, true); + vrf_name = yang_dnode_get_string(vrf_dnode, "./vrf"); - if (strmatch(vrf->name, VRF_DEFAULT_NAME)) { + if (strmatch(vrf_name, VRF_DEFAULT_NAME)) { name = NULL; } else { - name = vrf->name; + name = vrf_name; inst_type = BGP_INSTANCE_TYPE_VRF; } From 09b150ef2a20948947981fedcc86c82fcfa51a75 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Tue, 16 Feb 2021 12:57:30 +0300 Subject: [PATCH 3/4] lib: add definitions for vrf xpaths Signed-off-by: Igor Ryzhov --- lib/vrf.c | 7 +++---- lib/vrf.h | 3 +++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/vrf.c b/lib/vrf.c index 136938783f..0a91f4bc86 100644 --- a/lib/vrf.c +++ b/lib/vrf.c @@ -686,8 +686,8 @@ int vrf_handler_create(struct vty *vty, const char *vrfname, } if (vty) { - snprintf(xpath_list, sizeof(xpath_list), - "/frr-vrf:lib/vrf[name='%s']", vrfname); + snprintf(xpath_list, sizeof(xpath_list), FRR_VRF_KEY_XPATH, + vrfname); nb_cli_enqueue_change(vty, xpath_list, NB_OP_CREATE, NULL); ret = nb_cli_apply_changes(vty, xpath_list); @@ -821,8 +821,7 @@ DEFUN_YANG (no_vrf, return CMD_WARNING_CONFIG_FAILED; } - snprintf(xpath_list, sizeof(xpath_list), "/frr-vrf:lib/vrf[name='%s']", - vrfname); + snprintf(xpath_list, sizeof(xpath_list), FRR_VRF_KEY_XPATH, vrfname); nb_cli_enqueue_change(vty, xpath_list, NB_OP_DESTROY, NULL); return nb_cli_apply_changes(vty, xpath_list); diff --git a/lib/vrf.h b/lib/vrf.h index 32e6fb4289..333d68ce96 100644 --- a/lib/vrf.h +++ b/lib/vrf.h @@ -52,6 +52,9 @@ enum { IFLA_VRF_UNSPEC, IFLA_VRF_TABLE, __IFLA_VRF_MAX }; #define VRF_ALL_CMD_HELP_STR "Specify the VRF\nAll VRFs\n" #define VRF_FULL_CMD_HELP_STR "Specify the VRF\nThe VRF name\nAll VRFs\n" +#define FRR_VRF_XPATH "/frr-vrf:lib/vrf" +#define FRR_VRF_KEY_XPATH "/frr-vrf:lib/vrf[name='%s']" + /* * Pass some OS specific data up through * to the daemons From 2ada626940e6396a8313eb5688835e8fc38b571d Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Tue, 16 Feb 2021 12:57:01 +0300 Subject: [PATCH 4/4] lib: register dependency between control plane protocol and vrf nb nodes When the control plane protocol is created, the vrf structure is allocated, and its address is stored in the northbound node. The vrf structure may later be deleted by the user, which will lead to a stale pointer stored in this node. Instead of this, allow daemons that use the vrf pointer to register the dependency between the control plane protocol and vrf nodes. This will guarantee that the nodes will always be created and deleted together, and there won't be any stale pointers. Add such registration to staticd and pimd. Signed-off-by: Igor Ryzhov --- lib/routing_nb.h | 7 +++++ lib/routing_nb_config.c | 61 +++++++++++++++++++++++++++++++++-------- pimd/pim_main.c | 2 ++ staticd/static_main.c | 2 ++ 4 files changed, 61 insertions(+), 11 deletions(-) diff --git a/lib/routing_nb.h b/lib/routing_nb.h index d1b59ea29e..ffba631a10 100644 --- a/lib/routing_nb.h +++ b/lib/routing_nb.h @@ -15,10 +15,17 @@ int routing_control_plane_protocols_control_plane_protocol_destroy( #define FRR_ROUTING_KEY_XPATH \ "/frr-routing:routing/control-plane-protocols/" \ "control-plane-protocol[type='%s'][name='%s'][vrf='%s']" + +#define FRR_ROUTING_KEY_XPATH_VRF \ + "/frr-routing:routing/control-plane-protocols/" \ + "control-plane-protocol[vrf='%s']" + /* * callbacks for routing to handle configuration events * based on the control plane protocol */ DECLARE_HOOK(routing_conf_event, (struct nb_cb_create_args *args), (args)) +void routing_control_plane_protocols_register_vrf_dependency(void); + #endif /* _FRR_ROUTING_NB_H_ */ diff --git a/lib/routing_nb_config.c b/lib/routing_nb_config.c index b789e8494e..17698d2b87 100644 --- a/lib/routing_nb_config.c +++ b/lib/routing_nb_config.c @@ -45,15 +45,21 @@ int routing_control_plane_protocols_control_plane_protocol_create( case NB_EV_ABORT: break; case NB_EV_APPLY: - vrfname = yang_dnode_get_string(args->dnode, "./vrf"); - vrf = vrf_lookup_by_name(vrfname); - vrf = vrf ? vrf : vrf_get(VRF_UNKNOWN, vrfname); - if (!vrf) { - flog_warn(EC_LIB_NB_CB_CONFIG_APPLY, - "vrf creation %s failed", vrfname); - return NB_ERR; + /* + * If the daemon relies on the VRF pointer stored in this + * dnode, then it should register the dependency between this + * module and the VRF module using + * routing_control_plane_protocols_register_vrf_dependency. + * If such dependency is not registered, then nothing is + * stored in the dnode. If the dependency is registered, + * find the vrf and store the pointer. + */ + if (nb_node_has_dependency(args->dnode->schema->priv)) { + vrfname = yang_dnode_get_string(args->dnode, "./vrf"); + vrf = vrf_lookup_by_name(vrfname); + assert(vrf); + nb_running_set_entry(args->dnode, vrf); } - nb_running_set_entry(args->dnode, vrf); break; }; @@ -63,12 +69,45 @@ int routing_control_plane_protocols_control_plane_protocol_create( int routing_control_plane_protocols_control_plane_protocol_destroy( struct nb_cb_destroy_args *args) { - struct vrf *vrf __attribute__((unused)); - if (args->event != NB_EV_APPLY) return NB_OK; - vrf = nb_running_unset_entry(args->dnode); + /* + * If dependency on VRF module is registered, then VRF + * pointer was stored and must be cleared. + */ + if (nb_node_has_dependency(args->dnode->schema->priv)) + nb_running_unset_entry(args->dnode); return NB_OK; } + +static void vrf_to_control_plane_protocol(const struct lyd_node *dnode, + char *xpath) +{ + const char *vrf; + + vrf = yang_dnode_get_string(dnode, "./name"); + + snprintf(xpath, XPATH_MAXLEN, FRR_ROUTING_KEY_XPATH_VRF, vrf); +} + +static void control_plane_protocol_to_vrf(const struct lyd_node *dnode, + char *xpath) +{ + const char *vrf; + + vrf = yang_dnode_get_string(dnode, "./vrf"); + + snprintf(xpath, XPATH_MAXLEN, FRR_VRF_KEY_XPATH, vrf); +} + +void routing_control_plane_protocols_register_vrf_dependency(void) +{ + struct nb_dependency_callbacks cbs; + + cbs.get_dependant_xpath = vrf_to_control_plane_protocol; + cbs.get_dependency_xpath = control_plane_protocol_to_vrf; + + nb_node_set_dependency_cbs(FRR_VRF_XPATH, FRR_ROUTING_XPATH, &cbs); +} diff --git a/pimd/pim_main.c b/pimd/pim_main.c index 9c11cc47d5..5a09e7a8ee 100644 --- a/pimd/pim_main.c +++ b/pimd/pim_main.c @@ -145,6 +145,8 @@ int main(int argc, char **argv, char **envp) hook_register(routing_conf_event, routing_control_plane_protocols_name_validate); + routing_control_plane_protocols_register_vrf_dependency(); + frr_config_fork(); #ifdef PIM_DEBUG_BYDEFAULT diff --git a/staticd/static_main.c b/staticd/static_main.c index ac8f8ff029..560814771d 100644 --- a/staticd/static_main.c +++ b/staticd/static_main.c @@ -164,6 +164,8 @@ int main(int argc, char **argv, char **envp) hook_register(routing_conf_event, routing_control_plane_protocols_name_validate); + routing_control_plane_protocols_register_vrf_dependency(); + snprintf(backup_config_file, sizeof(backup_config_file), "%s/zebra.conf", frr_sysconfdir); staticd_di.backup_config_file = backup_config_file;