pimd: rework MSDP northbound integration

Simplify the MSDP handling functions and allow source changes.

Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>

7 files changed, 99 insertions, 221 deletions

diff --git a/pimd/pim_cmd.c b/pimd/pim_cmd.c
index b3d444465..41d70bd27 100644
--- a/pimd/pim_cmd.c
+++ b/pimd/pim_cmd.c
@@ -9669,15 +9669,14 @@ ALIAS(no_ip_pim_bfd, no_ip_pim_bfd_param_cmd,
       "Desired min transmit interval\n")
 #endif /* !HAVE_BFDD */
 
-	DEFUN (ip_msdp_peer,
-	       ip_msdp_peer_cmd,
-	       "ip msdp peer A.B.C.D source A.B.C.D",
-	       IP_STR
-	       CFG_MSDP_STR
-	       "Configure MSDP peer\n"
-	       "peer ip address\n"
-	       "Source address for TCP connection\n"
-	       "local ip address\n")
+DEFPY(ip_msdp_peer, ip_msdp_peer_cmd,
+      "ip msdp peer A.B.C.D$peer source A.B.C.D$source",
+      IP_STR
+      CFG_MSDP_STR
+      "Configure MSDP peer\n"
+      "Peer IP address\n"
+      "Source address for TCP connection\n"
+      "Local IP address\n")
 {
 	const char *vrfname;
 	char temp_xpath[XPATH_MAXLEN];
@@ -9688,16 +9687,15 @@ ALIAS(no_ip_pim_bfd, no_ip_pim_bfd_param_cmd,
 		return CMD_WARNING_CONFIG_FAILED;
 
 	snprintf(msdp_peer_source_xpath, sizeof(msdp_peer_source_xpath),
-		 FRR_PIM_AF_XPATH,
-		 "frr-pim:pimd", "pim", vrfname, "frr-routing:ipv4");
+		 FRR_PIM_AF_XPATH, "frr-pim:pimd", "pim", vrfname,
+		 "frr-routing:ipv4");
 	snprintf(temp_xpath, sizeof(temp_xpath),
-		 "/msdp-peer[peer-ip='%s']/source-ip",
-		 argv[3]->arg);
+		 "/msdp-peer[peer-ip='%s']/source-ip", peer_str);
 	strlcat(msdp_peer_source_xpath, temp_xpath,
 		sizeof(msdp_peer_source_xpath));
 
 	nb_cli_enqueue_change(vty, msdp_peer_source_xpath, NB_OP_MODIFY,
-			      argv[5]->arg);
+			      source_str);
 
 	return nb_cli_apply_changes(vty, NULL);
 }
diff --git a/pimd/pim_msdp.c b/pimd/pim_msdp.c
index 095c6de54..4ceee2959 100644
--- a/pimd/pim_msdp.c
+++ b/pimd/pim_msdp.c
@@ -1063,11 +1063,10 @@ static void pim_msdp_addr2su(union sockunion *su, struct in_addr addr)
 }
 
 /* 11.2.A1: create a new peer and transition state to listen or connecting */
-static enum pim_msdp_err pim_msdp_peer_new(struct pim_instance *pim,
-					   struct in_addr peer_addr,
-					   struct in_addr local_addr,
-					   const char *mesh_group_name,
-					   struct pim_msdp_peer **mp_p)
+struct pim_msdp_peer *pim_msdp_peer_new(struct pim_instance *pim,
+					const struct in_addr *peer,
+					const struct in_addr *local,
+					const char *mesh_group_name)
 {
 	struct pim_msdp_peer *mp;
 
@@ -1076,12 +1075,12 @@ static enum pim_msdp_err pim_msdp_peer_new(struct pim_instance *pim,
 	mp = XCALLOC(MTYPE_PIM_MSDP_PEER, sizeof(*mp));
 
 	mp->pim = pim;
-	mp->peer = peer_addr;
+	mp->peer = *peer;
 	pim_inet4_dump("<peer?>", mp->peer, mp->key_str, sizeof(mp->key_str));
 	pim_msdp_addr2su(&mp->su_peer, mp->peer);
-	mp->local = local_addr;
+	mp->local = *local;
 	/* XXX: originator_id setting needs to move to the mesh group */
-	pim->msdp.originator_id = local_addr;
+	pim->msdp.originator_id = *local;
 	pim_msdp_addr2su(&mp->su_local, mp->local);
 	mp->mesh_group_name = XSTRDUP(MTYPE_PIM_MSDP_MG_NAME, mesh_group_name);
 	mp->state = PIM_MSDP_INACTIVE;
@@ -1112,10 +1111,7 @@ static enum pim_msdp_err pim_msdp_peer_new(struct pim_instance *pim,
 	} else {
 		pim_msdp_peer_connect(mp);
 	}
-	if (mp_p) {
-		*mp_p = mp;
-	}
-	return PIM_MSDP_ERR_NONE;
+	return mp;
 }
 
 struct pim_msdp_peer *pim_msdp_peer_find(struct pim_instance *pim,
@@ -1127,43 +1123,6 @@ struct pim_msdp_peer *pim_msdp_peer_find(struct pim_instance *pim,
 	return hash_lookup(pim->msdp.peer_hash, &lookup);
 }
 
-/* add peer configuration if it doesn't already exist */
-enum pim_msdp_err pim_msdp_peer_add(struct pim_instance *pim,
-				    struct in_addr peer_addr,
-				    struct in_addr local_addr,
-				    const char *mesh_group_name,
-				    struct pim_msdp_peer **mp_p)
-{
-	struct pim_msdp_peer *mp;
-
-	if (mp_p) {
-		*mp_p = NULL;
-	}
-
-	if (peer_addr.s_addr == local_addr.s_addr) {
-		/* skip session setup if config is invalid */
-		if (PIM_DEBUG_MSDP_EVENTS) {
-			char peer_str[INET_ADDRSTRLEN];
-
-			pim_inet4_dump("<peer?>", peer_addr, peer_str,
-				       sizeof(peer_str));
-			zlog_debug("%s add skipped as DIP=SIP", peer_str);
-		}
-		return PIM_MSDP_ERR_SIP_EQ_DIP;
-	}
-
-	mp = pim_msdp_peer_find(pim, peer_addr);
-	if (mp) {
-		if (mp_p) {
-			*mp_p = mp;
-		}
-		return PIM_MSDP_ERR_PEER_EXISTS;
-	}
-
-	return pim_msdp_peer_new(pim, peer_addr, local_addr, mesh_group_name,
-				 mp_p);
-}
-
 /* release all mem associated with a peer */
 static void pim_msdp_peer_free(struct pim_msdp_peer *mp)
 {
@@ -1188,36 +1147,38 @@ static void pim_msdp_peer_free(struct pim_msdp_peer *mp)
 }
 
 /* delete the peer config */
-static enum pim_msdp_err pim_msdp_peer_do_del(struct pim_msdp_peer *mp)
+void pim_msdp_peer_do_del(struct pim_msdp_peer **mp)
 {
+	if (*mp == NULL)
+		return;
+
 	/* stop the tcp connection and shutdown all timers */
-	pim_msdp_peer_stop_tcp_conn(mp, true /* chg_state */);
+	pim_msdp_peer_stop_tcp_conn(*mp, true /* chg_state */);
 
 	/* remove the session from various tables */
-	listnode_delete(mp->pim->msdp.peer_list, mp);
-	hash_release(mp->pim->msdp.peer_hash, mp);
+	listnode_delete((*mp)->pim->msdp.peer_list, *mp);
+	hash_release((*mp)->pim->msdp.peer_hash, *mp);
 
 	if (PIM_DEBUG_MSDP_EVENTS) {
-		zlog_debug("MSDP peer %s deleted", mp->key_str);
+		zlog_debug("MSDP peer %s deleted", (*mp)->key_str);
 	}
 
 	/* free up any associated memory */
-	pim_msdp_peer_free(mp);
-
-	return PIM_MSDP_ERR_NONE;
+	pim_msdp_peer_free(*mp);
+	*mp = NULL;
 }
 
-enum pim_msdp_err pim_msdp_peer_del(struct pim_instance *pim,
-				    struct in_addr peer_addr)
+void pim_msdp_peer_change_source(struct pim_msdp_peer *mp,
+				 const struct in_addr *addr)
 {
-	struct pim_msdp_peer *mp;
+	pim_msdp_peer_stop_tcp_conn(mp, true);
 
-	mp = pim_msdp_peer_find(pim, peer_addr);
-	if (!mp) {
-		return PIM_MSDP_ERR_NO_PEER;
-	}
+	mp->local = *addr;
 
-	return pim_msdp_peer_do_del(mp);
+	if (PIM_MSDP_PEER_IS_LISTENER(mp))
+		pim_msdp_peer_listen(mp);
+	else
+		pim_msdp_peer_connect(mp);
 }
 
 /* peer hash and peer list helpers */
@@ -1319,7 +1280,7 @@ void pim_msdp_mg_mbr_del(struct pim_msdp_mg *mg, struct pim_msdp_mg_mbr *mbr)
 {
 	/* Delete active peer session if any */
 	if (mbr->mp) {
-		pim_msdp_peer_do_del(mbr->mp);
+		pim_msdp_peer_do_del(&mbr->mp);
 	}
 
 	listnode_delete(mg->mbr_list, mbr);
@@ -1342,10 +1303,8 @@ static void pim_msdp_src_del(struct pim_msdp_mg *mg)
 
 	/* SIP is being removed - tear down all active peer sessions */
 	for (ALL_LIST_ELEMENTS_RO(mg->mbr_list, mbr_node, mbr)) {
-		if (mbr->mp) {
-			pim_msdp_peer_do_del(mbr->mp);
-			mbr->mp = NULL;
-		}
+		if (mbr->mp)
+			pim_msdp_peer_do_del(&mbr->mp);
 	}
 	if (PIM_DEBUG_MSDP_EVENTS) {
 		zlog_debug("MSDP mesh-group %s src cleared",
@@ -1397,7 +1356,7 @@ bool pim_msdp_peer_config_write(struct vty *vty, struct pim_instance *pim,
 
 	for (ALL_LIST_ELEMENTS_RO(pim->msdp.peer_list, node, mp)) {
 		/* Non meshed peers have the group name set to 'default'. */
-		if (strcmp(mp->mesh_group_name, "default"))
+		if (strcmp(mp->mesh_group_name, MSDP_SOLO_PEER_GROUP_NAME))
 			continue;
 
 		vty_out(vty, "%sip msdp peer %pI4 source %pI4\n", spaces,
@@ -1504,8 +1463,8 @@ void pim_msdp_mg_src_add(struct pim_instance *pim, struct pim_msdp_mg *mg,
 
 	/* Create data structures and start TCP connection. */
 	for (ALL_LIST_ELEMENTS_RO(mg->mbr_list, mbr_node, mbr))
-		pim_msdp_peer_add(pim, mbr->mbr_ip, mg->src_ip,
-				  mg->mesh_group_name, &mbr->mp);
+		mbr->mp = pim_msdp_peer_new(pim, &mbr->mbr_ip, &mg->src_ip,
+					    mg->mesh_group_name);
 
 	if (PIM_DEBUG_MSDP_EVENTS)
 		zlog_debug("MSDP mesh-group %s src %pI4 set",
@@ -1524,8 +1483,8 @@ struct pim_msdp_mg_mbr *pim_msdp_mg_mbr_add(struct pim_instance *pim,
 
 	/* if valid SIP has been configured add peer session */
 	if (mg->src_ip.s_addr != INADDR_ANY)
-		pim_msdp_peer_add(pim, mbr->mbr_ip, mg->src_ip,
-				  mg->mesh_group_name, &mbr->mp);
+		mbr->mp = pim_msdp_peer_new(pim, &mbr->mbr_ip, &mg->src_ip,
+					    mg->mesh_group_name);
 
 	if (PIM_DEBUG_MSDP_EVENTS)
 		zlog_debug("MSDP mesh-group %s mbr %pI4 created",
diff --git a/pimd/pim_msdp.h b/pimd/pim_msdp.h
index bb7ee01ad..b9b6e0cc3 100644
--- a/pimd/pim_msdp.h
+++ b/pimd/pim_msdp.h
@@ -222,12 +222,6 @@ struct pim_msdp {
 struct pim_instance;
 void pim_msdp_init(struct pim_instance *pim, struct thread_master *master);
 void pim_msdp_exit(struct pim_instance *pim);
-enum pim_msdp_err pim_msdp_peer_add(struct pim_instance *pim,
-				    struct in_addr peer, struct in_addr local,
-				    const char *mesh_group_name,
-				    struct pim_msdp_peer **mp_p);
-enum pim_msdp_err pim_msdp_peer_del(struct pim_instance *pim,
-				    struct in_addr peer_addr);
 char *pim_msdp_state_dump(enum pim_msdp_peer_state state, char *buf,
 			  int buf_size);
 struct pim_msdp_peer *pim_msdp_peer_find(struct pim_instance *pim,
@@ -288,4 +282,34 @@ struct pim_msdp_mg_mbr *pim_msdp_mg_mbr_add(struct pim_instance *pim,
  */
 void pim_msdp_mg_mbr_del(struct pim_msdp_mg *mg, struct pim_msdp_mg_mbr *mbr);
 
+#define MSDP_SOLO_PEER_GROUP_NAME "default"
+
+/**
+ * Allocates MSDP peer data structure and starts state machine.
+ *
+ * \param pim PIM instance
+ * \param peer_addr peer address
+ * \param local_addr local listening address
+ * \param mesh_group_name mesh group name (or `MSDP_SOLO_PEER_GROUP_NAME` for
+ *                        peers without group).
+ */
+struct pim_msdp_peer *pim_msdp_peer_new(struct pim_instance *pim,
+					const struct in_addr *peer_addr,
+					const struct in_addr *local_addr,
+					const char *mesh_group_name);
+
+/**
+ * Stops peer state machine and free memory.
+ */
+void pim_msdp_peer_do_del(struct pim_msdp_peer **mp);
+
+/**
+ * Changes peer source address.
+ *
+ * NOTE:
+ * This will cause the connection to drop and start again.
+ */
+void pim_msdp_peer_change_source(struct pim_msdp_peer *mp,
+				 const struct in_addr *addr);
+
 #endif
diff --git a/pimd/pim_msdp_packet.c b/pimd/pim_msdp_packet.c
index 4aaf0f53d..48f1c864a 100644
--- a/pimd/pim_msdp_packet.c
+++ b/pimd/pim_msdp_packet.c
@@ -524,7 +524,8 @@ static void pim_msdp_pkt_sa_rx_one(struct pim_msdp_peer *mp, struct in_addr rp)
 	for (ALL_LIST_ELEMENTS_RO(mp->pim->msdp.peer_list, peer_node, peer)) {
 		if (!pim_msdp_peer_rpf_check(peer, rp)
 		    && (strcmp(mp->mesh_group_name, peer->mesh_group_name)
-			|| !strcmp(mp->mesh_group_name, "default"))) {
+			|| !strcmp(mp->mesh_group_name,
+				   MSDP_SOLO_PEER_GROUP_NAME))) {
 			pim_msdp_pkt_sa_tx_one_to_one_peer(peer, rp, sg);
 		}
 	}
diff --git a/pimd/pim_nb.c b/pimd/pim_nb.c
index ea53f1ef1..848a28d31 100644
--- a/pimd/pim_nb.c
+++ b/pimd/pim_nb.c
@@ -149,7 +149,6 @@ const struct frr_yang_module_info frr_pim_info = {
 			.xpath = "/frr-routing:routing/control-plane-protocols/control-plane-protocol/frr-pim:pim/address-family/msdp-peer/source-ip",
 			.cbs = {
 				.modify = routing_control_plane_protocols_control_plane_protocol_pim_address_family_msdp_peer_source_ip_modify,
-				.destroy = routing_control_plane_protocols_control_plane_protocol_pim_address_family_msdp_peer_source_ip_destroy,
 			}
 		},
 		{
diff --git a/pimd/pim_nb.h b/pimd/pim_nb.h
index 1959b403f..07387be76 100644
--- a/pimd/pim_nb.h
+++ b/pimd/pim_nb.h
@@ -72,8 +72,6 @@ int routing_control_plane_protocols_control_plane_protocol_pim_address_family_ms
 	struct nb_cb_destroy_args *args);
 int routing_control_plane_protocols_control_plane_protocol_pim_address_family_msdp_peer_source_ip_modify(
 	struct nb_cb_modify_args *args);
-int routing_control_plane_protocols_control_plane_protocol_pim_address_family_msdp_peer_source_ip_destroy(
-	struct nb_cb_destroy_args *args);
 int routing_control_plane_protocols_control_plane_protocol_pim_address_family_mlag_create(
 	struct nb_cb_create_args *args);
 int routing_control_plane_protocols_control_plane_protocol_pim_address_family_mlag_destroy(
diff --git a/pimd/pim_nb_config.c b/pimd/pim_nb_config.c
index b70656ea7..d91aa61e7 100644
--- a/pimd/pim_nb_config.c
+++ b/pimd/pim_nb_config.c
@@ -243,65 +243,6 @@ static int pim_ssm_cmd_worker(struct pim_instance *pim, const char *plist,
 	return ret;
 }
 
-static int ip_msdp_peer_cmd_worker(struct pim_instance *pim,
-		struct in_addr peer_addr,
-		struct in_addr local_addr,
-		char *errmsg, size_t errmsg_len)
-{
-	enum pim_msdp_err result;
-	int ret = NB_OK;
-
-	result = pim_msdp_peer_add(pim, peer_addr, local_addr, "default",
-			NULL /* mp_p */);
-	switch (result) {
-	case PIM_MSDP_ERR_NONE:
-		break;
-	case PIM_MSDP_ERR_OOM:
-		ret = NB_ERR;
-		snprintf(errmsg, errmsg_len,
-			 "%% Out of memory");
-		break;
-	case PIM_MSDP_ERR_PEER_EXISTS:
-		ret = NB_ERR;
-		snprintf(errmsg, errmsg_len,
-			 "%% Peer exists");
-		break;
-	case PIM_MSDP_ERR_MAX_MESH_GROUPS:
-		ret = NB_ERR;
-		snprintf(errmsg, errmsg_len,
-			 "%% Only one mesh-group allowed currently");
-		break;
-	default:
-		ret = NB_ERR;
-		snprintf(errmsg, errmsg_len,
-			 "%% peer add failed");
-	}
-
-	return ret;
-}
-
-static int ip_no_msdp_peer_cmd_worker(struct pim_instance *pim,
-		struct in_addr peer_addr,
-		char *errmsg, size_t errmsg_len)
-{
-	enum pim_msdp_err result;
-
-	result = pim_msdp_peer_del(pim, peer_addr);
-	switch (result) {
-	case PIM_MSDP_ERR_NONE:
-		break;
-	case PIM_MSDP_ERR_NO_PEER:
-		snprintf(errmsg, errmsg_len,
-			 "%% Peer does not exist");
-		break;
-	default:
-		snprintf(errmsg, errmsg_len,
-			 "%% peer del failed");
-	}
-
-	return result ? NB_ERR : NB_OK;
-}
-
 static int pim_rp_cmd_worker(struct pim_instance *pim,
 		struct in_addr rp_addr,
 		struct prefix group, const char *plist,
@@ -1163,11 +1104,26 @@ int pim_msdp_mesh_group_members_destroy(struct nb_cb_destroy_args *args)
 int routing_control_plane_protocols_control_plane_protocol_pim_address_family_msdp_peer_create(
 	struct nb_cb_create_args *args)
 {
+	struct pim_msdp_peer *mp;
+	struct pim_instance *pim;
+	struct vrf *vrf;
+	struct ipaddr peer_ip;
+	struct ipaddr source_ip;
+
 	switch (args->event) {
 	case NB_EV_VALIDATE:
 	case NB_EV_PREPARE:
 	case NB_EV_ABORT:
+		break;
 	case NB_EV_APPLY:
+		vrf = nb_running_get_entry(args->dnode, NULL, true);
+		pim = vrf->info;
+		yang_dnode_get_ip(&peer_ip, args->dnode, "./peer-ip");
+		yang_dnode_get_ip(&source_ip, args->dnode, "./source-ip");
+		mp = pim_msdp_peer_new(pim, &peer_ip.ipaddr_v4,
+				       &source_ip.ipaddr_v4,
+				       MSDP_SOLO_PEER_GROUP_NAME);
+		nb_running_set_entry(args->dnode, mp);
 		break;
 	}
 
@@ -1177,10 +1133,7 @@ int routing_control_plane_protocols_control_plane_protocol_pim_address_family_ms
 int routing_control_plane_protocols_control_plane_protocol_pim_address_family_msdp_peer_destroy(
 	struct nb_cb_destroy_args *args)
 {
-	int result;
-	struct pim_instance *pim;
-	struct ipaddr peer_ip;
-	struct vrf *vrf;
+	struct pim_msdp_peer *mp;
 
 	switch (args->event) {
 	case NB_EV_VALIDATE:
@@ -1188,16 +1141,8 @@ int routing_control_plane_protocols_control_plane_protocol_pim_address_family_ms
 	case NB_EV_ABORT:
 		break;
 	case NB_EV_APPLY:
-		vrf = nb_running_get_entry(args->dnode, NULL, true);
-		pim = vrf->info;
-		yang_dnode_get_ip(&peer_ip, args->dnode, "./peer-ip");
-		result = ip_no_msdp_peer_cmd_worker(pim, peer_ip.ip._v4_addr,
-				args->errmsg,
-				args->errmsg_len);
-
-		if (result)
-			return NB_ERR_INCONSISTENCY;
-
+		mp = nb_running_unset_entry(args->dnode);
+		pim_msdp_peer_do_del(&mp);
 		break;
 	}
 
@@ -1210,64 +1155,18 @@ int routing_control_plane_protocols_control_plane_protocol_pim_address_family_ms
 int routing_control_plane_protocols_control_plane_protocol_pim_address_family_msdp_peer_source_ip_modify(
 	struct nb_cb_modify_args *args)
 {
-	int result;
-	struct vrf *vrf;
-	struct pim_instance *pim;
-	struct ipaddr peer_ip;
+	struct pim_msdp_peer *mp;
 	struct ipaddr source_ip;
-	const struct lyd_node *mesh_group_name_dnode;
-	const char *mesh_group_name;
 
 	switch (args->event) {
 	case NB_EV_VALIDATE:
-		mesh_group_name_dnode =
-			yang_dnode_get(args->dnode,
-					"../../msdp-mesh-group/mesh-group-name");
-		if (mesh_group_name_dnode) {
-			mesh_group_name =
-				yang_dnode_get_string(mesh_group_name_dnode,
-						".");
-			if (strcmp(mesh_group_name, "default")) {
-				/* currently only one mesh-group can exist at a
-				 * time
-				 */
-				snprintf(args->errmsg, args->errmsg_len,
-					 "%% Only one mesh-group allowed currently");
-				return NB_ERR_VALIDATION;
-			}
-		}
-		break;
 	case NB_EV_PREPARE:
 	case NB_EV_ABORT:
 		break;
 	case NB_EV_APPLY:
-		vrf = nb_running_get_entry(args->dnode, NULL, true);
-		pim = vrf->info;
-		yang_dnode_get_ip(&peer_ip, args->dnode, "../peer-ip");
+		mp = nb_running_get_entry(args->dnode, NULL, true);
 		yang_dnode_get_ip(&source_ip, args->dnode, NULL);
-
-		result = ip_msdp_peer_cmd_worker(pim, peer_ip.ip._v4_addr,
-				source_ip.ip._v4_addr,
-				args->errmsg,
-				args->errmsg_len);
-
-		if (result)
-			return NB_ERR_INCONSISTENCY;
-
-		break;
-	}
-
-	return NB_OK;
-}
-
-int routing_control_plane_protocols_control_plane_protocol_pim_address_family_msdp_peer_source_ip_destroy(
-	struct nb_cb_destroy_args *args)
-{
-	switch (args->event) {
-	case NB_EV_VALIDATE:
-	case NB_EV_PREPARE:
-	case NB_EV_ABORT:
-	case NB_EV_APPLY:
+		pim_msdp_peer_change_source(mp, &source_ip.ipaddr_v4);
 		break;
 	}