ripd, ripngd: change how we keep track of redistribution configuration

ripd and ripngd were leveraging the zclient code to keep track of
the redistribute configuration, which is what most daemons do. The
problem, however, is that the zclient code uses VRF IDs to identify
VRFs, and VRF IDs are unknown until a VRF is enabled (information
received from zebra). This means we can't configure a redistribute
command on a RIP instance when the corresponding VRF is disabled
(doing so leads to a null-dereference crash right now in both ripd
and ripngd).

To fix this, change the rip/ripng data structures so that they
keep track of the full redistribute configuration and not only the
route-map and metric associated to each command. This is similar
to what bgpd and ospfd are doing to solve the same problem. In the
future the zclient code and all daemons need to be refactored to
consolidate the handling of redistribute configuration in a single
place to reduce code duplication. One of the most important changes
to do is to use VRF names and not VRF IDs to identify VRFs.

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

8 files changed, 154 insertions, 84 deletions

diff --git a/ripd/rip_northbound.c b/ripd/rip_northbound.c
index d2360c470..13520d11d 100644
--- a/ripd/rip_northbound.c
+++ b/ripd/rip_northbound.c
@@ -637,6 +637,17 @@ static int ripd_instance_redistribute_create(enum nb_event event,
 					     const struct lyd_node *dnode,
 					     union nb_resource *resource)
 {
+	struct rip *rip;
+	int type;
+
+	if (event != NB_EV_APPLY)
+		return NB_OK;
+
+	rip = yang_dnode_get_entry(dnode, true);
+	type = yang_dnode_get_enum(dnode, "./protocol");
+
+	rip->redist[type].enabled = true;
+
 	return NB_OK;
 }
 
@@ -652,7 +663,17 @@ static int ripd_instance_redistribute_delete(enum nb_event event,
 	rip = yang_dnode_get_entry(dnode, true);
 	type = yang_dnode_get_enum(dnode, "./protocol");
 
-	rip_redistribute_conf_delete(rip, type);
+	rip->redist[type].enabled = false;
+	if (rip->redist[type].route_map.name) {
+		free(rip->redist[type].route_map.name);
+		rip->redist[type].route_map.name = NULL;
+		rip->redist[type].route_map.map = NULL;
+	}
+	rip->redist[type].metric_config = false;
+	rip->redist[type].metric = 0;
+
+	if (rip->enabled)
+		rip_redistribute_conf_delete(rip, type);
 
 	return NB_OK;
 }
@@ -666,7 +687,8 @@ ripd_instance_redistribute_apply_finish(const struct lyd_node *dnode)
 	rip = yang_dnode_get_entry(dnode, true);
 	type = yang_dnode_get_enum(dnode, "./protocol");
 
-	rip_redistribute_conf_update(rip, type);
+	if (rip->enabled)
+		rip_redistribute_conf_update(rip, type);
 }
 
 /*
@@ -688,10 +710,10 @@ ripd_instance_redistribute_route_map_modify(enum nb_event event,
 	type = yang_dnode_get_enum(dnode, "../protocol");
 	rmap_name = yang_dnode_get_string(dnode, NULL);
 
-	if (rip->route_map[type].name)
-		free(rip->route_map[type].name);
-	rip->route_map[type].name = strdup(rmap_name);
-	rip->route_map[type].map = route_map_lookup_by_name(rmap_name);
+	if (rip->redist[type].route_map.name)
+		free(rip->redist[type].route_map.name);
+	rip->redist[type].route_map.name = strdup(rmap_name);
+	rip->redist[type].route_map.map = route_map_lookup_by_name(rmap_name);
 
 	return NB_OK;
 }
@@ -709,9 +731,9 @@ ripd_instance_redistribute_route_map_delete(enum nb_event event,
 	rip = yang_dnode_get_entry(dnode, true);
 	type = yang_dnode_get_enum(dnode, "../protocol");
 
-	free(rip->route_map[type].name);
-	rip->route_map[type].name = NULL;
-	rip->route_map[type].map = NULL;
+	free(rip->redist[type].route_map.name);
+	rip->redist[type].route_map.name = NULL;
+	rip->redist[type].route_map.map = NULL;
 
 	return NB_OK;
 }
@@ -735,8 +757,8 @@ ripd_instance_redistribute_metric_modify(enum nb_event event,
 	type = yang_dnode_get_enum(dnode, "../protocol");
 	metric = yang_dnode_get_uint8(dnode, NULL);
 
-	rip->route_map[type].metric_config = true;
-	rip->route_map[type].metric = metric;
+	rip->redist[type].metric_config = true;
+	rip->redist[type].metric = metric;
 
 	return NB_OK;
 }
@@ -754,8 +776,8 @@ ripd_instance_redistribute_metric_delete(enum nb_event event,
 	rip = yang_dnode_get_entry(dnode, true);
 	type = yang_dnode_get_enum(dnode, "../protocol");
 
-	rip->route_map[type].metric_config = false;
-	rip->route_map[type].metric = 0;
+	rip->redist[type].metric_config = false;
+	rip->redist[type].metric = 0;
 
 	return NB_OK;
 }
diff --git a/ripd/rip_zebra.c b/ripd/rip_zebra.c
index d8b35cf97..4f0df1223 100644
--- a/ripd/rip_zebra.c
+++ b/ripd/rip_zebra.c
@@ -168,23 +168,28 @@ void rip_redistribute_conf_delete(struct rip *rip, int type)
 
 int rip_redistribute_check(struct rip *rip, int type)
 {
-	return vrf_bitmap_check(zclient->redist[AFI_IP][type],
-				rip->vrf->vrf_id);
+	return rip->redist[type].enabled;
 }
 
-void rip_redistribute_clean(struct rip *rip)
+void rip_redistribute_enable(struct rip *rip)
 {
 	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++) {
-		if (!vrf_bitmap_check(zclient->redist[AFI_IP][i],
-				      rip->vrf->vrf_id))
+		if (!rip_redistribute_check(rip, i))
 			continue;
 
-		if (zclient->sock > 0)
-			zebra_redistribute_send(ZEBRA_REDISTRIBUTE_DELETE,
-						zclient, AFI_IP, i, 0,
-						rip->vrf->vrf_id);
+		zebra_redistribute_send(ZEBRA_REDISTRIBUTE_ADD, zclient, AFI_IP,
+					i, 0, rip->vrf->vrf_id);
+	}
+}
 
-		vrf_bitmap_unset(zclient->redist[AFI_IP][i], rip->vrf->vrf_id);
+void rip_redistribute_disable(struct rip *rip)
+{
+	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++) {
+		if (!rip_redistribute_check(rip, i))
+			continue;
+
+		zebra_redistribute_send(ZEBRA_REDISTRIBUTE_DELETE, zclient,
+					AFI_IP, i, 0, rip->vrf->vrf_id);
 	}
 }
 
@@ -192,8 +197,7 @@ void rip_show_redistribute_config(struct vty *vty, struct rip *rip)
 {
 	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++) {
 		if (i == zclient->redist_default
-		    || !vrf_bitmap_check(zclient->redist[AFI_IP][i],
-					 rip->vrf->vrf_id))
+		    || !rip_redistribute_check(rip, i))
 			continue;
 
 		vty_out(vty, " %s", zebra_route_string(i));
diff --git a/ripd/ripd.c b/ripd/ripd.c
index a6a2a29de..7fe8fc8c8 100644
--- a/ripd/ripd.c
+++ b/ripd/ripd.c
@@ -2239,10 +2239,10 @@ void rip_output_process(struct connected *ifc, struct sockaddr_in *to,
 			}
 
 			/* Apply redistribute route map - continue, if deny */
-			if (rip->route_map[rinfo->type].name
+			if (rip->redist[rinfo->type].route_map.name
 			    && rinfo->sub_type != RIP_ROUTE_INTERFACE) {
 				ret = route_map_apply(
-					rip->route_map[rinfo->type].map,
+					rip->redist[rinfo->type].route_map.map,
 					(struct prefix *)p, RMAP_RIP, rinfo);
 
 				if (ret == RMAP_DENYMATCH) {
@@ -2258,11 +2258,10 @@ void rip_output_process(struct connected *ifc, struct sockaddr_in *to,
 			/* When route-map does not set metric. */
 			if (!rinfo->metric_set) {
 				/* If redistribute metric is set. */
-				if (rip->route_map[rinfo->type].metric_config
+				if (rip->redist[rinfo->type].metric_config
 				    && rinfo->metric != RIP_METRIC_INFINITY) {
 					rinfo->metric_out =
-						rip->route_map[rinfo->type]
-							.metric;
+						rip->redist[rinfo->type].metric;
 				} else {
 					/* If the route is not connected or
 					   localy generated
@@ -3382,8 +3381,8 @@ void rip_clean(struct rip *rip)
 	stream_free(rip->obuf);
 
 	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++)
-		if (rip->route_map[i].name)
-			free(rip->route_map[i].name);
+		if (rip->redist[i].route_map.name)
+			free(rip->redist[i].route_map.name);
 
 	route_table_finish(rip->table);
 	route_table_finish(rip->neighbor);
@@ -3398,7 +3397,6 @@ void rip_clean(struct rip *rip)
 	list_delete(&rip->offset_list_master);
 	rip_interfaces_clean(rip);
 	route_table_finish(rip->distance_table);
-	rip_redistribute_clean(rip);
 
 	RB_REMOVE(rip_instance_head, &rip_instances, rip);
 	XFREE(MTYPE_RIP_VRF_NAME, rip->vrf_name);
@@ -3447,9 +3445,9 @@ void rip_if_rmap_update_interface(struct interface *ifp)
 static void rip_routemap_update_redistribute(struct rip *rip)
 {
 	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++) {
-		if (rip->route_map[i].name)
-			rip->route_map[i].map = route_map_lookup_by_name(
-				rip->route_map[i].name);
+		if (rip->redist[i].route_map.name)
+			rip->redist[i].route_map.map = route_map_lookup_by_name(
+				rip->redist[i].route_map.name);
 	}
 }
 
@@ -3501,6 +3499,9 @@ static void rip_instance_enable(struct rip *rip, struct vrf *vrf, int sock)
 	rip_vrf_link(rip, vrf);
 	rip->enabled = true;
 
+	/* Resend all redistribute requests. */
+	rip_redistribute_enable(rip);
+
 	/* Create read and timer thread. */
 	rip_event(rip, RIP_READ, rip->sock);
 	rip_event(rip, RIP_UPDATE_EVENT, 1);
@@ -3536,6 +3537,9 @@ static void rip_instance_disable(struct rip *rip)
 		route_unlock_node(rp);
 	}
 
+	/* Flush all redistribute requests. */
+	rip_redistribute_disable(rip);
+
 	/* Cancel RIP related timers. */
 	RIP_TIMER_OFF(rip->t_update);
 	RIP_TIMER_OFF(rip->t_triggered_update);
diff --git a/ripd/ripd.h b/ripd/ripd.h
index d88c5c1e6..f78dae7a8 100644
--- a/ripd/ripd.h
+++ b/ripd/ripd.h
@@ -170,13 +170,16 @@ struct rip {
 	/* RIP offset-lists. */
 	struct list *offset_list_master;
 
-	/* For redistribute route map. */
+	/* RIP redistribute configuration. */
 	struct {
-		char *name;
-		struct route_map *map;
+		bool enabled;
+		struct {
+			char *name;
+			struct route_map *map;
+		} route_map;
 		bool metric_config;
 		uint8_t metric;
-	} route_map[ZEBRA_ROUTE_MAX];
+	} redist[ZEBRA_ROUTE_MAX];
 
 	/* For distribute-list container */
 	struct distribute_ctx *distribute_ctx;
@@ -487,7 +490,8 @@ extern struct rip *rip_info_get_instance(const struct rip_info *rinfo);
 extern struct rip_distance *rip_distance_new(void);
 extern void rip_distance_free(struct rip_distance *rdistance);
 extern uint8_t rip_distance_apply(struct rip *rip, struct rip_info *rinfo);
-extern void rip_redistribute_clean(struct rip *rip);
+extern void rip_redistribute_enable(struct rip *rip);
+extern void rip_redistribute_disable(struct rip *rip);
 
 extern int rip_route_rte(struct rip_info *rinfo);
 extern struct rip_info *rip_ecmp_add(struct rip *rip,
diff --git a/ripngd/ripng_northbound.c b/ripngd/ripng_northbound.c
index 4b15a3aef..dda57a31f 100644
--- a/ripngd/ripng_northbound.c
+++ b/ripngd/ripng_northbound.c
@@ -411,6 +411,17 @@ static int ripngd_instance_redistribute_create(enum nb_event event,
 					       const struct lyd_node *dnode,
 					       union nb_resource *resource)
 {
+	struct ripng *ripng;
+	int type;
+
+	if (event != NB_EV_APPLY)
+		return NB_OK;
+
+	ripng = yang_dnode_get_entry(dnode, true);
+	type = yang_dnode_get_enum(dnode, "./protocol");
+
+	ripng->redist[type].enabled = true;
+
 	return NB_OK;
 }
 
@@ -426,7 +437,17 @@ static int ripngd_instance_redistribute_delete(enum nb_event event,
 	ripng = yang_dnode_get_entry(dnode, true);
 	type = yang_dnode_get_enum(dnode, "./protocol");
 
-	ripng_redistribute_conf_delete(ripng, type);
+	ripng->redist[type].enabled = false;
+	if (ripng->redist[type].route_map.name) {
+		free(ripng->redist[type].route_map.name);
+		ripng->redist[type].route_map.name = NULL;
+		ripng->redist[type].route_map.map = NULL;
+	}
+	ripng->redist[type].metric_config = false;
+	ripng->redist[type].metric = 0;
+
+	if (ripng->enabled)
+		ripng_redistribute_conf_delete(ripng, type);
 
 	return NB_OK;
 }
@@ -440,7 +461,8 @@ ripngd_instance_redistribute_apply_finish(const struct lyd_node *dnode)
 	ripng = yang_dnode_get_entry(dnode, true);
 	type = yang_dnode_get_enum(dnode, "./protocol");
 
-	ripng_redistribute_conf_update(ripng, type);
+	if (ripng->enabled)
+		ripng_redistribute_conf_update(ripng, type);
 }
 
 /*
@@ -462,10 +484,10 @@ ripngd_instance_redistribute_route_map_modify(enum nb_event event,
 	type = yang_dnode_get_enum(dnode, "../protocol");
 	rmap_name = yang_dnode_get_string(dnode, NULL);
 
-	if (ripng->route_map[type].name)
-		free(ripng->route_map[type].name);
-	ripng->route_map[type].name = strdup(rmap_name);
-	ripng->route_map[type].map = route_map_lookup_by_name(rmap_name);
+	if (ripng->redist[type].route_map.name)
+		free(ripng->redist[type].route_map.name);
+	ripng->redist[type].route_map.name = strdup(rmap_name);
+	ripng->redist[type].route_map.map = route_map_lookup_by_name(rmap_name);
 
 	return NB_OK;
 }
@@ -483,9 +505,9 @@ ripngd_instance_redistribute_route_map_delete(enum nb_event event,
 	ripng = yang_dnode_get_entry(dnode, true);
 	type = yang_dnode_get_enum(dnode, "../protocol");
 
-	free(ripng->route_map[type].name);
-	ripng->route_map[type].name = NULL;
-	ripng->route_map[type].map = NULL;
+	free(ripng->redist[type].route_map.name);
+	ripng->redist[type].route_map.name = NULL;
+	ripng->redist[type].route_map.map = NULL;
 
 	return NB_OK;
 }
@@ -509,8 +531,8 @@ ripngd_instance_redistribute_metric_modify(enum nb_event event,
 	type = yang_dnode_get_enum(dnode, "../protocol");
 	metric = yang_dnode_get_uint8(dnode, NULL);
 
-	ripng->route_map[type].metric_config = true;
-	ripng->route_map[type].metric = metric;
+	ripng->redist[type].metric_config = true;
+	ripng->redist[type].metric = metric;
 
 	return NB_OK;
 }
@@ -528,8 +550,8 @@ ripngd_instance_redistribute_metric_delete(enum nb_event event,
 	ripng = yang_dnode_get_entry(dnode, true);
 	type = yang_dnode_get_enum(dnode, "../protocol");
 
-	ripng->route_map[type].metric_config = false;
-	ripng->route_map[type].metric = 0;
+	ripng->redist[type].metric_config = false;
+	ripng->redist[type].metric = 0;
 
 	return NB_OK;
 }
diff --git a/ripngd/ripng_zebra.c b/ripngd/ripng_zebra.c
index 913ac5191..e2dcc0238 100644
--- a/ripngd/ripng_zebra.c
+++ b/ripngd/ripng_zebra.c
@@ -165,24 +165,28 @@ void ripng_redistribute_conf_delete(struct ripng *ripng, int type)
 
 int ripng_redistribute_check(struct ripng *ripng, int type)
 {
-	return vrf_bitmap_check(zclient->redist[AFI_IP6][type],
-				ripng->vrf->vrf_id);
+	return ripng->redist[type].enabled;
 }
 
-void ripng_redistribute_clean(struct ripng *ripng)
+void ripng_redistribute_enable(struct ripng *ripng)
 {
 	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++) {
-		if (!vrf_bitmap_check(zclient->redist[AFI_IP6][i],
-				      ripng->vrf->vrf_id))
+		if (!ripng_redistribute_check(ripng, i))
 			continue;
 
-		if (zclient->sock > 0)
-			zebra_redistribute_send(ZEBRA_REDISTRIBUTE_DELETE,
-						zclient, AFI_IP6, i, 0,
-						ripng->vrf->vrf_id);
+		zebra_redistribute_send(ZEBRA_REDISTRIBUTE_ADD, zclient,
+					AFI_IP6, i, 0, ripng->vrf->vrf_id);
+	}
+}
 
-		vrf_bitmap_unset(zclient->redist[AFI_IP6][i],
-				 ripng->vrf->vrf_id);
+void ripng_redistribute_disable(struct ripng *ripng)
+{
+	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++) {
+		if (!ripng_redistribute_check(ripng, i))
+			continue;
+
+		zebra_redistribute_send(ZEBRA_REDISTRIBUTE_DELETE, zclient,
+					AFI_IP6, i, 0, ripng->vrf->vrf_id);
 	}
 }
 
@@ -192,8 +196,7 @@ void ripng_redistribute_write(struct vty *vty, struct ripng *ripng)
 
 	for (i = 0; i < ZEBRA_ROUTE_MAX; i++) {
 		if (i == zclient->redist_default
-		    || !vrf_bitmap_check(zclient->redist[AFI_IP6][i],
-					 ripng->vrf->vrf_id))
+		    || !ripng_redistribute_check(ripng, i))
 			continue;
 
 		vty_out(vty, "    %s", zebra_route_string(i));
diff --git a/ripngd/ripngd.c b/ripngd/ripngd.c
index 951c73bc4..c3aa9d8db 100644
--- a/ripngd/ripngd.c
+++ b/ripngd/ripngd.c
@@ -1682,10 +1682,11 @@ void ripng_output_process(struct interface *ifp, struct sockaddr_in6 *to,
 			}
 
 			/* Redistribute route-map. */
-			if (ripng->route_map[rinfo->type].name) {
-				ret = route_map_apply(
-					ripng->route_map[rinfo->type].map,
-					(struct prefix *)p, RMAP_RIPNG, rinfo);
+			if (ripng->redist[rinfo->type].route_map.name) {
+				ret = route_map_apply(ripng->redist[rinfo->type]
+							      .route_map.map,
+						      (struct prefix *)p,
+						      RMAP_RIPNG, rinfo);
 
 				if (ret == RMAP_DENYMATCH) {
 					if (IS_RIPNG_DEBUG_PACKET)
@@ -1700,10 +1701,10 @@ void ripng_output_process(struct interface *ifp, struct sockaddr_in6 *to,
 			/* When the route-map does not set metric. */
 			if (!rinfo->metric_set) {
 				/* If the redistribute metric is set. */
-				if (ripng->route_map[rinfo->type].metric_config
+				if (ripng->redist[rinfo->type].metric_config
 				    && rinfo->metric != RIPNG_METRIC_INFINITY) {
 					rinfo->metric_out =
-						ripng->route_map[rinfo->type]
+						ripng->redist[rinfo->type]
 							.metric;
 				} else {
 					/* If the route is not connected or
@@ -2531,8 +2532,8 @@ void ripng_clean(struct ripng *ripng)
 		ripng_instance_disable(ripng);
 
 	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++)
-		if (ripng->route_map[i].name)
-			free(ripng->route_map[i].name);
+		if (ripng->redist[i].route_map.name)
+			free(ripng->redist[i].route_map.name);
 
 	agg_table_finish(ripng->table);
 	list_delete(&ripng->peer_list);
@@ -2548,7 +2549,6 @@ void ripng_clean(struct ripng *ripng)
 	vector_free(ripng->passive_interface);
 	list_delete(&ripng->offset_list_master);
 	ripng_interface_clean(ripng);
-	ripng_redistribute_clean(ripng);
 
 	RB_REMOVE(ripng_instance_head, &ripng_instances, ripng);
 	XFREE(MTYPE_RIPNG_VRF_NAME, ripng->vrf_name);
@@ -2598,9 +2598,10 @@ void ripng_if_rmap_update_interface(struct interface *ifp)
 static void ripng_routemap_update_redistribute(struct ripng *ripng)
 {
 	for (int i = 0; i < ZEBRA_ROUTE_MAX; i++) {
-		if (ripng->route_map[i].name)
-			ripng->route_map[i].map = route_map_lookup_by_name(
-				ripng->route_map[i].name);
+		if (ripng->redist[i].route_map.name)
+			ripng->redist[i].route_map.map =
+				route_map_lookup_by_name(
+					ripng->redist[i].route_map.name);
 	}
 }
 
@@ -2652,6 +2653,9 @@ static void ripng_instance_enable(struct ripng *ripng, struct vrf *vrf,
 	ripng_vrf_link(ripng, vrf);
 	ripng->enabled = true;
 
+	/* Resend all redistribute requests. */
+	ripng_redistribute_enable(ripng);
+
 	/* Create read and timer thread. */
 	ripng_event(ripng, RIPNG_READ, ripng->sock);
 	ripng_event(ripng, RIPNG_UPDATE_EVENT, 1);
@@ -2694,6 +2698,9 @@ static void ripng_instance_disable(struct ripng *ripng)
 		}
 	}
 
+	/* Flush all redistribute requests. */
+	ripng_redistribute_disable(ripng);
+
 	/* Cancel the RIPng timers */
 	RIPNG_TIMER_OFF(ripng->t_update);
 	RIPNG_TIMER_OFF(ripng->t_triggered_update);
diff --git a/ripngd/ripngd.h b/ripngd/ripngd.h
index d6c4394c6..b38e6b3a9 100644
--- a/ripngd/ripngd.h
+++ b/ripngd/ripngd.h
@@ -149,13 +149,16 @@ struct ripng {
 	/* RIPng ECMP flag */
 	bool ecmp;
 
-	/* For redistribute route map. */
+	/* RIPng redistribute configuration. */
 	struct {
-		char *name;
-		struct route_map *map;
+		bool enabled;
+		struct {
+			char *name;
+			struct route_map *map;
+		} route_map;
 		bool metric_config;
 		uint8_t metric;
-	} route_map[ZEBRA_ROUTE_MAX];
+	} redist[ZEBRA_ROUTE_MAX];
 
 	/* For distribute-list container */
 	struct distribute_ctx *distribute_ctx;
@@ -447,7 +450,8 @@ extern void ripng_if_rmap_update_interface(struct interface *);
 extern void ripng_zebra_ipv6_add(struct ripng *ripng, struct agg_node *node);
 extern void ripng_zebra_ipv6_delete(struct ripng *ripng, struct agg_node *node);
 
-extern void ripng_redistribute_clean(struct ripng *ripng);
+extern void ripng_redistribute_enable(struct ripng *ripng);
+extern void ripng_redistribute_disable(struct ripng *ripng);
 extern int ripng_redistribute_check(struct ripng *ripng, int type);
 extern void ripng_redistribute_write(struct vty *vty, struct ripng *ripng);