lib: remove leaf-list xpath hack from northbound

Currently, when editing a leaf-list, `nb_candidate_edit` expects to receive it's xpath without a predicate and the value in a separate argument, and then creates the full xpath. This hack is complicated, because it depends on the operation and on the caller being a backend or not. Instead, let's require to always include the predicate in a leaf-list xpath. Update all the usages in the code accordingly. Signed-off-by: Igor Ryzhov <[email protected]>
FRRouting · Jan 23, 2024 · 4e8b43b · 4e8b43b
1 parent 22d1ad7
commit 4e8b43b
Show file tree

Hide file tree

Showing 10 changed files with 169 additions and 121 deletions.
diff --git a/eigrpd/eigrp_cli.c b/eigrpd/eigrp_cli.c
@@ -131,12 +131,14 @@ DEFPY_YANG(
 	"Suppress routing updates on an interface\n"
 	"Interface to suppress on\n")
 {
+	char xpath[XPATH_MAXLEN];
+
+	snprintf(xpath, sizeof(xpath), "./passive-interface[.='%s']", ifname);
+
 	if (no)
-		nb_cli_enqueue_change(vty, "./passive-interface",
-				      NB_OP_DESTROY, ifname);
+		nb_cli_enqueue_change(vty, xpath, NB_OP_DESTROY, NULL);
 	else
-		nb_cli_enqueue_change(vty, "./passive-interface",
-				      NB_OP_CREATE, ifname);
+		nb_cli_enqueue_change(vty, xpath, NB_OP_CREATE, NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }
@@ -354,12 +356,14 @@ DEFPY_YANG(
 	"Enable routing on an IP network\n"
 	"EIGRP network prefix\n")
 {
+	char xpath[XPATH_MAXLEN];
+
+	snprintf(xpath, sizeof(xpath), "./network[.='%s']", prefix_str);
+
 	if (no)
-		nb_cli_enqueue_change(vty, "./network", NB_OP_DESTROY,
-				      prefix_str);
+		nb_cli_enqueue_change(vty, xpath, NB_OP_DESTROY, NULL);
 	else
-		nb_cli_enqueue_change(vty, "./network", NB_OP_CREATE,
-				      prefix_str);
+		nb_cli_enqueue_change(vty, xpath, NB_OP_CREATE, NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }
@@ -383,12 +387,14 @@ DEFPY_YANG(
 	"Specify a neighbor router\n"
 	"Neighbor address\n")
 {
+	char xpath[XPATH_MAXLEN];
+
+	snprintf(xpath, sizeof(xpath), "./neighbor[.='%s']", addr_str);
+
 	if (no)
-		nb_cli_enqueue_change(vty, "./neighbor", NB_OP_DESTROY,
-				      addr_str);
+		nb_cli_enqueue_change(vty, xpath, NB_OP_DESTROY, NULL);
 	else
-		nb_cli_enqueue_change(vty, "./neighbor", NB_OP_CREATE,
-				      addr_str);
+		nb_cli_enqueue_change(vty, xpath, NB_OP_CREATE, NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }
@@ -658,8 +664,9 @@ DEFPY_YANG(
 		 as_str);
 	nb_cli_enqueue_change(vty, xpath, NB_OP_CREATE, NULL);
 
-	snprintf(xpath_auth, sizeof(xpath_auth), "%s/summarize-addresses", xpath);
-	nb_cli_enqueue_change(vty, xpath_auth, NB_OP_CREATE, prefix_str);
+	snprintf(xpath_auth, sizeof(xpath_auth),
+		 "%s/summarize-addresses[.='%s']", xpath, prefix_str);
+	nb_cli_enqueue_change(vty, xpath_auth, NB_OP_CREATE, NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }
@@ -681,8 +688,9 @@ DEFPY_YANG(
 		 as_str);
 	nb_cli_enqueue_change(vty, xpath, NB_OP_CREATE, NULL);
 
-	snprintf(xpath_auth, sizeof(xpath_auth), "%s/summarize-addresses", xpath);
-	nb_cli_enqueue_change(vty, xpath_auth, NB_OP_DESTROY, prefix_str);
+	snprintf(xpath_auth, sizeof(xpath_auth),
+		 "%s/summarize-addresses[.='%s']", xpath, prefix_str);
+	nb_cli_enqueue_change(vty, xpath_auth, NB_OP_DESTROY, NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }

diff --git a/isisd/isis_cli.c b/isisd/isis_cli.c
@@ -300,8 +300,12 @@ DEFPY_YANG(net, net_cmd, "[no] net WORD",
       "A Network Entity Title for this process (OSI only)\n"
       "XX.XXXX. ... .XXX.XX  Network entity title (NET)\n")
 {
-	nb_cli_enqueue_change(vty, "./area-address",
-			      no ? NB_OP_DESTROY : NB_OP_CREATE, net);
+	char xpath[XPATH_MAXLEN];
+
+	snprintf(xpath, XPATH_MAXLEN, "./area-address[.='%s']", net);
+
+	nb_cli_enqueue_change(vty, xpath, no ? NB_OP_DESTROY : NB_OP_CREATE,
+			      NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }
@@ -3054,12 +3058,16 @@ void cli_show_ip_isis_circ_type(struct vty *vty, const struct lyd_node *dnode,
 }
 
 static int ag_change(struct vty *vty, int argc, struct cmd_token **argv,
-		     const char *xpath, bool no, int start_idx)
+		     const char *xpath_base, bool no, int start_idx)
 {
-	for (int i = start_idx; i < argc; i++)
+	char xpath[XPATH_MAXLEN];
+
+	for (int i = start_idx; i < argc; i++) {
+		snprintf(xpath, XPATH_MAXLEN, "%s[.='%s']", xpath_base,
+			 argv[i]->arg);
 		nb_cli_enqueue_change(vty, xpath,
-				      no ? NB_OP_DESTROY : NB_OP_CREATE,
-				      argv[i]->arg);
+				      no ? NB_OP_DESTROY : NB_OP_CREATE, NULL);
+	}
 	return nb_cli_apply_changes(vty, NULL);
 }
 
@@ -3302,31 +3310,27 @@ DEFPY(isis_lfa_exclude_interface, isis_lfa_exclude_interface_cmd,
       "Exclude an interface from computation\n"
       "Interface name\n")
 {
+	char xpath[XPATH_MAXLEN];
+
 	if (!level || strmatch(level, "level-1")) {
-		if (no) {
-			nb_cli_enqueue_change(
-				vty,
-				"./frr-isisd:isis/fast-reroute/level-1/lfa/exclude-interface",
-				NB_OP_DESTROY, ifname);
-		} else {
-			nb_cli_enqueue_change(
-				vty,
-				"./frr-isisd:isis/fast-reroute/level-1/lfa/exclude-interface",
-				NB_OP_CREATE, ifname);
-		}
+		snprintf(xpath, sizeof(xpath),
+			 "./frr-isisd:isis/fast-reroute/level-1/lfa/exclude-interface[.='%s']",
+			 ifname);
+
+		if (no)
+			nb_cli_enqueue_change(vty, xpath, NB_OP_DESTROY, NULL);
+		else
+			nb_cli_enqueue_change(vty, xpath, NB_OP_CREATE, NULL);
 	}
 	if (!level || strmatch(level, "level-2")) {
-		if (no) {
-			nb_cli_enqueue_change(
-				vty,
-				"./frr-isisd:isis/fast-reroute/level-2/lfa/exclude-interface",
-				NB_OP_DESTROY, ifname);
-		} else {
-			nb_cli_enqueue_change(
-				vty,
-				"./frr-isisd:isis/fast-reroute/level-2/lfa/exclude-interface",
-				NB_OP_CREATE, ifname);
-		}
+		snprintf(xpath, sizeof(xpath),
+			 "./frr-isisd:isis/fast-reroute/level-2/lfa/exclude-interface[.='%s']",
+			 ifname);
+
+		if (no)
+			nb_cli_enqueue_change(vty, xpath, NB_OP_DESTROY, NULL);
+		else
+			nb_cli_enqueue_change(vty, xpath, NB_OP_CREATE, NULL);
 	}
 
 	return nb_cli_apply_changes(vty, NULL);

diff --git a/lib/northbound.c b/lib/northbound.c
@@ -695,35 +695,22 @@ static int dnode_create(struct nb_config *candidate, const char *xpath,
 
 int nb_candidate_edit(struct nb_config *candidate, const struct nb_node *nb_node,
 		      enum nb_operation operation, const char *xpath,
-		      bool in_backend, const struct yang_data *previous,
+		      const struct yang_data *previous,
 		      const struct yang_data *data)
 {
 	struct lyd_node *dnode, *dep_dnode, *old_dnode;
-	char xpath_edit[XPATH_MAXLEN];
 	char dep_xpath[XPATH_MAXLEN];
 	struct lyd_node *parent = NULL;
 	uint32_t options = 0;
 	LY_ERR err;
 
-	/*
-	 * Use special notation for leaf-lists (RFC 6020, section 9.13.5).
-	 * if we are in a backend client this notation was already applied
-	 * by mgmtd before sending to us.
-	 */
-	if (!in_backend && nb_node->snode->nodetype == LYS_LEAFLIST &&
-	    (operation == NB_OP_DESTROY || operation == NB_OP_DELETE))
-		snprintf(xpath_edit, sizeof(xpath_edit), "%s[.='%s']", xpath,
-			 data->value);
-	else
-		strlcpy(xpath_edit, xpath, sizeof(xpath_edit));
-
 	switch (operation) {
 	case NB_OP_CREATE:
 	case NB_OP_MODIFY:
 		options = LYD_NEW_PATH_UPDATE;
 		fallthrough;
 	case NB_OP_CREATE_EXCL:
-		err = dnode_create(candidate, xpath_edit, data->value, options,
+		err = dnode_create(candidate, xpath, data->value, options,
 				   &dnode);
 		if (err) {
 			return err;
@@ -750,7 +737,7 @@ int nb_candidate_edit(struct nb_config *candidate, const struct nb_node *nb_node
 		break;
 	case NB_OP_DESTROY:
 	case NB_OP_DELETE:
-		dnode = yang_dnode_get(candidate->dnode, xpath_edit);
+		dnode = yang_dnode_get(candidate->dnode, xpath);
 		if (!dnode) {
 			if (operation == NB_OP_DELETE)
 				return NB_ERR;
@@ -768,12 +755,12 @@ int nb_candidate_edit(struct nb_config *candidate, const struct nb_node *nb_node
 		lyd_free_tree(dnode);
 		break;
 	case NB_OP_REPLACE:
-		old_dnode = yang_dnode_get(candidate->dnode, xpath_edit);
+		old_dnode = yang_dnode_get(candidate->dnode, xpath);
 		if (old_dnode) {
 			parent = lyd_parent(old_dnode);
 			lyd_unlink_tree(old_dnode);
 		}
-		err = dnode_create(candidate, xpath_edit, data->value, options,
+		err = dnode_create(candidate, xpath, data->value, options,
 				   &dnode);
 		if (!err && dnode && !old_dnode) {
 			/* create dependency if the node didn't exist */
@@ -908,8 +895,7 @@ void nb_candidate_edit_config_changes(struct nb_config *candidate_config,
 		 * configuration.
 		 */
 		ret = nb_candidate_edit(candidate_config, nb_node,
-					change->operation, xpath, in_backend,
-					NULL, data);
+					change->operation, xpath, NULL, data);
 		yang_data_free(data);
 		if (ret != NB_OK) {
 			flog_warn(

diff --git a/lib/northbound.h b/lib/northbound.h
@@ -950,9 +950,6 @@ extern bool nb_is_operation_allowed(struct nb_node *nb_node,
  * xpath
  *    XPath of the configuration node being edited.
  *
- * in_backend
- *    Specify whether the changes are being applied in the backend or not.
- *
  * previous
  *    Previous value of the configuration node. Should be used only when the
  *    operation is NB_OP_MOVE, otherwise this parameter is ignored.
@@ -967,7 +964,7 @@ extern bool nb_is_operation_allowed(struct nb_node *nb_node,
 extern int nb_candidate_edit(struct nb_config *candidate,
 			     const struct nb_node *nb_node,
 			     enum nb_operation operation, const char *xpath,
-			     bool in_backend, const struct yang_data *previous,
+			     const struct yang_data *previous,
 			     const struct yang_data *data);
 
 /*

diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c
@@ -256,7 +256,7 @@ frr_confd_cdb_diff_iter(confd_hkeypath_t *kp, enum cdb_iter_op cdb_op,
 	/* Edit the candidate configuration. */
 	data = yang_data_new(xpath, value_str);
 	ret = nb_candidate_edit(iter_args->candidate, nb_node, nb_op, xpath,
-				false, NULL, data);
+				NULL, data);
 	yang_data_free(data);
 	if (ret != NB_OK) {
 		flog_warn(

diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c
@@ -219,7 +219,7 @@ static int frr_sr_process_change(struct nb_config *candidate,
 	sr_val_to_buff(sr_data, value_str, sizeof(value_str));
 	data = yang_data_new(xpath, value_str);
 
-	ret = nb_candidate_edit(candidate, nb_node, nb_op, xpath, false, NULL, data);
+	ret = nb_candidate_edit(candidate, nb_node, nb_op, xpath, NULL, data);
 	yang_data_free(data);
 	if (ret != NB_OK) {
 		flog_warn(

diff --git a/pimd/pim_cmd_common.c b/pimd/pim_cmd_common.c
@@ -525,7 +525,9 @@ int pim_process_rp_cmd(struct vty *vty, const char *rp_str,
 		       const char *group_str)
 {
 	const char *vrfname;
-	char rp_group_xpath[XPATH_MAXLEN];
+	char group_xpath[XPATH_MAXLEN];
+	char rp_xpath[XPATH_MAXLEN];
+	int printed;
 	int result = 0;
 	struct prefix group;
 	pim_addr rp_addr;
@@ -570,20 +572,25 @@ int pim_process_rp_cmd(struct vty *vty, const char *rp_str,
 	if (vrfname == NULL)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	snprintf(rp_group_xpath, sizeof(rp_group_xpath),
-		 FRR_PIM_STATIC_RP_XPATH, "frr-pim:pimd", "pim", vrfname,
-		 FRR_PIM_AF_XPATH_VAL, rp_str);
-	strlcat(rp_group_xpath, "/group-list", sizeof(rp_group_xpath));
+	snprintf(rp_xpath, sizeof(rp_xpath), FRR_PIM_STATIC_RP_XPATH,
+		 "frr-pim:pimd", "pim", vrfname, FRR_PIM_AF_XPATH_VAL, rp_str);
+	printed = snprintf(group_xpath, sizeof(group_xpath),
+			   "%s/group-list[.='%s']", rp_xpath, group_str);
+
+	if (printed >= (int)(sizeof(group_xpath))) {
+		vty_out(vty, "Xpath too long (%d > %u)", printed + 1,
+			XPATH_MAXLEN);
+		return CMD_WARNING_CONFIG_FAILED;
+	}
 
-	nb_cli_enqueue_change(vty, rp_group_xpath, NB_OP_CREATE, group_str);
+	nb_cli_enqueue_change(vty, group_xpath, NB_OP_CREATE, NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }
 
 int pim_process_no_rp_cmd(struct vty *vty, const char *rp_str,
 			  const char *group_str)
 {
-	char group_list_xpath[XPATH_MAXLEN];
 	char group_xpath[XPATH_MAXLEN];
 	char rp_xpath[XPATH_MAXLEN];
 	int printed;
@@ -596,18 +603,8 @@ int pim_process_no_rp_cmd(struct vty *vty, const char *rp_str,
 
 	snprintf(rp_xpath, sizeof(rp_xpath), FRR_PIM_STATIC_RP_XPATH,
 		 "frr-pim:pimd", "pim", vrfname, FRR_PIM_AF_XPATH_VAL, rp_str);
-
-	printed = snprintf(group_list_xpath, sizeof(group_list_xpath),
-			   "%s/group-list", rp_xpath);
-
-	if (printed >= (int)(sizeof(group_list_xpath))) {
-		vty_out(vty, "Xpath too long (%d > %u)", printed + 1,
-			XPATH_MAXLEN);
-		return CMD_WARNING_CONFIG_FAILED;
-	}
-
-	printed = snprintf(group_xpath, sizeof(group_xpath), "%s[.='%s']",
-			   group_list_xpath, group_str);
+	printed = snprintf(group_xpath, sizeof(group_xpath),
+			   "%s/group-list[.='%s']", rp_xpath, group_str);
 
 	if (printed >= (int)(sizeof(group_xpath))) {
 		vty_out(vty, "Xpath too long (%d > %u)", printed + 1,
@@ -624,8 +621,7 @@ int pim_process_no_rp_cmd(struct vty *vty, const char *rp_str,
 	if (yang_is_last_list_dnode(group_dnode))
 		nb_cli_enqueue_change(vty, rp_xpath, NB_OP_DESTROY, NULL);
 	else
-		nb_cli_enqueue_change(vty, group_list_xpath, NB_OP_DESTROY,
-				      group_str);
+		nb_cli_enqueue_change(vty, group_xpath, NB_OP_DESTROY, NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }
@@ -3407,6 +3403,8 @@ int pim_process_ssmpingd_cmd(struct vty *vty, enum nb_operation operation,
 {
 	const char *vrfname;
 	char ssmpingd_ip_xpath[XPATH_MAXLEN];
+	char ssmpingd_src_ip_xpath[XPATH_MAXLEN];
+	int printed;
 
 	vrfname = pim_cli_get_vrf_name(vty);
 	if (vrfname == NULL)
@@ -3415,10 +3413,16 @@ int pim_process_ssmpingd_cmd(struct vty *vty, enum nb_operation operation,
 	snprintf(ssmpingd_ip_xpath, sizeof(ssmpingd_ip_xpath),
 		 FRR_PIM_VRF_XPATH, "frr-pim:pimd", "pim", vrfname,
 		 FRR_PIM_AF_XPATH_VAL);
-	strlcat(ssmpingd_ip_xpath, "/ssm-pingd-source-ip",
-		sizeof(ssmpingd_ip_xpath));
+	printed = snprintf(ssmpingd_src_ip_xpath, sizeof(ssmpingd_src_ip_xpath),
+			   "%s/ssm-pingd-source-ip[.='%s']", ssmpingd_ip_xpath,
+			   src_str);
+	if (printed >= (int)sizeof(ssmpingd_src_ip_xpath)) {
+		vty_out(vty, "Xpath too long (%d > %u)", printed + 1,
+			XPATH_MAXLEN);
+		return CMD_WARNING_CONFIG_FAILED;
+	}
 
-	nb_cli_enqueue_change(vty, ssmpingd_ip_xpath, operation, src_str);
+	nb_cli_enqueue_change(vty, ssmpingd_src_ip_xpath, operation, NULL);
 
 	return nb_cli_apply_changes(vty, NULL);
 }