Merge pull request #8095 from idryzhov/fix-nb-stale-pointers

fix stale pointers in northbound nodes

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;
 		}
 

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:

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.
  *

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_ */

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);
+}

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);

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

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

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;