lib, mgmtd: respect base xpath in mgmtd

`nb_cli_apply_changes` can be called with base xpath which should be
prepended to xpaths of every change in a transaction. This base xpath is
respected by regular northbound CLI but not by mgmtd. This commit fixes
the problem.

Signed-off-by: Igor Ryzhov <[email protected]>

lib/mgmt_be_client.c

@@ -426,8 +426,7 @@ static int mgmt_be_txn_cfg_prepare(struct mgmt_be_txn_ctx *txn)
 				client_ctx->candidate_config,
 				txn_req->req.set_cfg.cfg_changes,
 				(size_t)txn_req->req.set_cfg.num_cfg_changes,
-				NULL, NULL, 0, err_buf, sizeof(err_buf),
-				&error);
+				NULL, err_buf, sizeof(err_buf), &error);
 			if (error) {
 				err_buf[sizeof(err_buf) - 1] = 0;
 				MGMTD_BE_CLIENT_ERR(

lib/northbound.c

@@ -763,8 +763,8 @@ static bool nb_is_operation_allowed(struct nb_node *nb_node,
 
 void nb_candidate_edit_config_changes(
 	struct nb_config *candidate_config, struct nb_cfg_change cfg_changes[],
-	size_t num_cfg_changes, const char *xpath_base, const char *curr_xpath,
-	int xpath_index, char *err_buf, int err_bufsize, bool *error)
+	size_t num_cfg_changes, const char *xpath_base, char *err_buf,
+	int err_bufsize, bool *error)
 {
 	if (error)
 		*error = false;
@@ -782,19 +782,12 @@ void nb_candidate_edit_config_changes(
 
 		/* Handle relative XPaths. */
 		memset(xpath, 0, sizeof(xpath));
-		if (xpath_index > 0 &&
-		    (xpath_base[0] == '.' || change->xpath[0] == '.'))
-			strlcpy(xpath, curr_xpath, sizeof(xpath));
-		if (xpath_base[0]) {
-			if (xpath_base[0] == '.')
-				strlcat(xpath, xpath_base + 1, sizeof(xpath));
-			else
-				strlcat(xpath, xpath_base, sizeof(xpath));
+		if (change->xpath[0] != '/') {
+			strlcpy(xpath, xpath_base, sizeof(xpath));
+			if (change->xpath[0] != 0)
+				strlcat(xpath, "/", sizeof(xpath));
 		}
-		if (change->xpath[0] == '.')
-			strlcat(xpath, change->xpath + 1, sizeof(xpath));
-		else
-			strlcpy(xpath, change->xpath, sizeof(xpath));
+		strlcat(xpath, change->xpath, sizeof(xpath));
 
 		/* Find the northbound node associated to the data path. */
 		nb_node = nb_node_find(xpath);

lib/northbound.h

@@ -916,12 +916,6 @@ extern bool nb_candidate_needs_update(const struct nb_config *candidate);
  * xpath_base
  *    Base xpath for config.
  *
- * curr_xpath
- *    Current xpath for config.
- *
- * xpath_index
- *    Index of xpath being processed.
- *
  * err_buf
  *    Buffer to store human-readable error message in case of error.
  *
@@ -933,8 +927,8 @@ extern bool nb_candidate_needs_update(const struct nb_config *candidate);
  */
 extern void nb_candidate_edit_config_changes(
 	struct nb_config *candidate_config, struct nb_cfg_change cfg_changes[],
-	size_t num_cfg_changes, const char *xpath_base, const char *curr_xpath,
-	int xpath_index, char *err_buf, int err_bufsize, bool *error);
+	size_t num_cfg_changes, const char *xpath_base, char *err_buf,
+	int err_bufsize, bool *error);
 
 /*
  * Delete candidate configuration changes.

lib/northbound_cli.c

@@ -130,6 +130,22 @@ void nb_cli_enqueue_change(struct vty *vty, const char *xpath,
 		return;
 	}
 
+	/*
+	 * A lot of places in the code use "." to indicate that operation is
+	 * done for the base xpath itself. I doesn't make much sense, because if
+	 * we append "." to the base xpath, the resulting xpath becomes invalid.
+	 * Just ignore such xpath values.
+	 */
+	if (xpath[0] == '.' && xpath[1] == 0)
+		xpath = "";
+
+	/*
+	 * A lot of places in the code use "./" prefix for relative xpaths. It
+	 * is not needed and adds parsing complexity, so should be just skipped.
+	 */
+	if (xpath[0] == '.' && xpath[1] == '/')
+		xpath += 2;
+
 	change = &vty->cfg_changes[vty->num_cfg_changes++];
 	strlcpy(change->xpath, xpath, sizeof(change->xpath));
 	change->operation = operation;
@@ -147,8 +163,7 @@ static int nb_cli_apply_changes_internal(struct vty *vty,
 
 	nb_candidate_edit_config_changes(
 		vty->candidate_config, vty->cfg_changes, vty->num_cfg_changes,
-		xpath_base, VTY_CURR_XPATH, vty->xpath_index, buf, sizeof(buf),
-		&error);
+		xpath_base, buf, sizeof(buf), &error);
 	if (error) {
 		/*
 		 * Failure to edit the candidate configuration should never
@@ -182,6 +197,7 @@ static int nb_cli_apply_changes_internal(struct vty *vty,
 
 int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 {
+	char xpath_base_abs[XPATH_MAXLEN] = {};
 	char xpath_base[XPATH_MAXLEN] = {};
 	bool implicit_commit;
 	int ret;
@@ -195,25 +211,30 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 		va_end(ap);
 	}
 
+	vty_combine_xpath(vty, xpath_base_abs, sizeof(xpath_base_abs),
+			  xpath_base);
+
 	if (vty_mgmt_should_process_cli_apply_changes(vty)) {
 		VTY_CHECK_XPATH;
 
 		if (vty->type == VTY_FILE)
 			return CMD_SUCCESS;
 
 		implicit_commit = vty_needs_implicit_commit(vty);
-		ret = vty_mgmt_send_config_data(vty, implicit_commit);
+		ret = vty_mgmt_send_config_data(vty, xpath_base_abs,
+						implicit_commit);
 		if (ret >= 0 && !implicit_commit)
 			vty->mgmt_num_pending_setcfg++;
 		return ret;
 	}
 
-	return nb_cli_apply_changes_internal(vty, xpath_base, false);
+	return nb_cli_apply_changes_internal(vty, xpath_base_abs, false);
 }
 
 int nb_cli_apply_changes_clear_pending(struct vty *vty,
 				       const char *xpath_base_fmt, ...)
 {
+	char xpath_base_abs[XPATH_MAXLEN] = {};
 	char xpath_base[XPATH_MAXLEN] = {};
 	bool implicit_commit;
 	int ret;
@@ -227,6 +248,9 @@ int nb_cli_apply_changes_clear_pending(struct vty *vty,
 		va_end(ap);
 	}
 
+	vty_combine_xpath(vty, xpath_base_abs, sizeof(xpath_base_abs),
+			  xpath_base);
+
 	if (vty_mgmt_should_process_cli_apply_changes(vty)) {
 		VTY_CHECK_XPATH;
 		/*
@@ -238,13 +262,14 @@ int nb_cli_apply_changes_clear_pending(struct vty *vty,
 		 * conversions to mgmtd require full proper implementations.
 		 */
 		implicit_commit = vty_needs_implicit_commit(vty);
-		ret = vty_mgmt_send_config_data(vty, implicit_commit);
+		ret = vty_mgmt_send_config_data(vty, xpath_base_abs,
+						implicit_commit);
 		if (ret >= 0 && !implicit_commit)
 			vty->mgmt_num_pending_setcfg++;
 		return ret;
 	}
 
-	return nb_cli_apply_changes_internal(vty, xpath_base, true);
+	return nb_cli_apply_changes_internal(vty, xpath_base_abs, true);
 }
 
 int nb_cli_rpc(struct vty *vty, const char *xpath, struct list *input,

lib/vty.c

@@ -3674,6 +3674,20 @@ static struct mgmt_fe_client_cbs mgmt_cbs = {
 	.get_data_notify = vty_mgmt_get_data_result_notified,
 };
 
+void vty_combine_xpath(struct vty *vty, char *dst, size_t dst_size,
+		       const char *xpath)
+{
+	memset(dst, 0, dst_size);
+
+	/* If xpath is not absolute, prepend current vty xpath. */
+	if (vty->xpath_index > 0 && xpath[0] != '/') {
+		strlcpy(dst, VTY_CURR_XPATH, dst_size);
+		if (xpath[0] != 0)
+			strlcat(dst, "/", dst_size);
+	}
+	strlcat(dst, xpath, dst_size);
+}
+
 void vty_init_mgmt_fe(void)
 {
 	char name[40];
@@ -3718,12 +3732,14 @@ int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
 	return 0;
 }
 
-int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
+int vty_mgmt_send_config_data(struct vty *vty, const char *xpath_base,
+			      bool implicit_commit)
 {
 	Mgmtd__YangDataValue value[VTY_MAXCFGCHANGES];
 	Mgmtd__YangData cfg_data[VTY_MAXCFGCHANGES];
 	Mgmtd__YangCfgDataReq cfg_req[VTY_MAXCFGCHANGES];
 	Mgmtd__YangCfgDataReq *cfgreq[VTY_MAXCFGCHANGES] = {0};
+	char xpath[VTY_MAXCFGCHANGES][XPATH_MAXLEN];
 	size_t indx;
 
 	if (vty->type == VTY_FILE) {
@@ -3759,6 +3775,9 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 		}
 	}
 
+	if (xpath_base == NULL)
+		xpath_base = "";
+
 	for (indx = 0; indx < vty->num_cfg_changes; indx++) {
 		mgmt_yang_data_init(&cfg_data[indx]);
 
@@ -3771,7 +3790,15 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 			cfg_data[indx].value = &value[indx];
 		}
 
-		cfg_data[indx].xpath = vty->cfg_changes[indx].xpath;
+		memset(xpath[indx], 0, sizeof(xpath[indx]));
+		if (vty->cfg_changes[indx].xpath[0] != '/') {
+			strlcpy(xpath[indx], xpath_base, sizeof(xpath[indx]));
+			if (vty->cfg_changes[indx].xpath[0] != 0)
+				strlcat(xpath[indx], "/", sizeof(xpath[indx]));
+		}
+		strlcat(xpath[indx], vty->cfg_changes[indx].xpath,
+			sizeof(xpath[indx]));
+		cfg_data[indx].xpath = xpath[indx];
 
 		mgmt_yang_cfg_data_req_init(&cfg_req[indx]);
 		cfg_req[indx].data = &cfg_data[indx];

lib/vty.h

@@ -407,12 +407,16 @@ extern void vty_stdio_suspend(void);
 extern void vty_stdio_resume(void);
 extern void vty_stdio_close(void);
 
+extern void vty_combine_xpath(struct vty *vty, char *dst, size_t dst_size,
+			      const char *xpath);
+
 extern void vty_init_mgmt_fe(void);
 extern bool vty_mgmt_fe_enabled(void);
 extern bool vty_mgmt_should_process_cli_apply_changes(struct vty *vty);
 
 extern bool mgmt_vty_read_configs(void);
-extern int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit);
+extern int vty_mgmt_send_config_data(struct vty *vty, const char *xpath_base,
+				     bool implicit_commit);
 extern int vty_mgmt_send_commit_config(struct vty *vty, bool validate_only,
 				       bool abort);
 extern int vty_mgmt_send_get_req(struct vty *vty, bool is_config,

mgmtd/mgmt_txn.c

@@ -664,8 +664,8 @@ static void mgmt_txn_process_set_cfg(struct event *thread)
 						 txn_req->req.set_cfg->cfg_changes,
 						 (size_t)txn_req->req.set_cfg
 							 ->num_cfg_changes,
-						 NULL, NULL, 0, err_buf,
-						 sizeof(err_buf), &error);
+						 NULL, err_buf, sizeof(err_buf),
+						 &error);
 		if (error) {
 			mgmt_fe_send_set_cfg_reply(txn->session_id, txn->txn_id,
 						   txn_req->req.set_cfg->ds_id,

mgmtd/mgmt_vty.c

@@ -157,7 +157,7 @@ DEFPY(mgmt_set_config_data, mgmt_set_config_data_cmd,
 	vty->cfg_changes[0].operation = NB_OP_CREATE;
 	vty->num_cfg_changes = 1;
 
-	vty_mgmt_send_config_data(vty, false);
+	vty_mgmt_send_config_data(vty, NULL, false);
 	return CMD_SUCCESS;
 }
 
@@ -174,7 +174,7 @@ DEFPY(mgmt_delete_config_data, mgmt_delete_config_data_cmd,
 	vty->cfg_changes[0].operation = NB_OP_DESTROY;
 	vty->num_cfg_changes = 1;
 
-	vty_mgmt_send_config_data(vty, false);
+	vty_mgmt_send_config_data(vty, NULL, false);
 	return CMD_SUCCESS;
 }