Merge pull request #13846 from LabNConsulting/chopps/backport-13795

backport of #13795

lib/command.c

@@ -1333,11 +1333,12 @@ int config_from_file(struct vty *vty, FILE *fp, unsigned int *line_num)
 /* Configuration from terminal */
 DEFUN (config_terminal,
        config_terminal_cmd,
-       "configure [terminal]",
+       "configure [terminal [file-lock]]",
        "Configuration from vty interface\n"
+       "Configuration with locked datastores\n"
        "Configuration terminal\n")
 {
-	return vty_config_enter(vty, false, false);
+	return vty_config_enter(vty, false, false, argc == 3);
 }
 
 /* Enable command */

lib/mgmt.proto

@@ -243,7 +243,8 @@ message FeSetConfigReply {
   required DatastoreId ds_id = 2;
   required uint64 req_id = 3;
   required bool success = 4;
-  optional string error_if_any = 5;
+  required bool implicit_commit = 5;
+  optional string error_if_any = 6;
 }
 
 message FeCommitConfigReq {

lib/mgmt_fe_client.c

@@ -164,7 +164,7 @@ static int mgmt_fe_send_session_req(struct mgmt_fe_client *client,
 
 int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client, uint64_t session_id,
 			    uint64_t req_id, Mgmtd__DatastoreId ds_id,
-			    bool lock)
+			    bool lock, bool scok)
 {
 	(void)req_id;
 	Mgmtd__FeMessage fe_msg;
@@ -185,7 +185,7 @@ int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client, uint64_t session_id,
 		lock ? "" : "UN", dsid2name(ds_id), session_id);
 
 
-	return mgmt_fe_client_send_msg(client, &fe_msg, false);
+	return mgmt_fe_client_send_msg(client, &fe_msg, scok);
 }
 
 int mgmt_fe_send_setcfg_req(struct mgmt_fe_client *client, uint64_t session_id,
@@ -411,6 +411,7 @@ static int mgmt_fe_client_handle_msg(struct mgmt_fe_client *client,
 				session->user_ctx, fe_msg->setcfg_reply->req_id,
 				fe_msg->setcfg_reply->success,
 				fe_msg->setcfg_reply->ds_id,
+				fe_msg->setcfg_reply->implicit_commit,
 				fe_msg->setcfg_reply->error_if_any);
 		break;
 	case MGMTD__FE_MESSAGE__MESSAGE_COMMCFG_REPLY:

lib/mgmt_fe_client.h

@@ -91,7 +91,7 @@ struct mgmt_fe_client_cbs {
 				  uintptr_t session_id,
 				  uintptr_t user_session_client,
 				  uint64_t req_id, bool success,
-				  Mgmtd__DatastoreId ds_id,
+				  Mgmtd__DatastoreId ds_id, bool implcit_commit,
 				  char *errmsg_if_any);
 
 	void (*commit_config_notify)(struct mgmt_fe_client *client,
@@ -219,7 +219,8 @@ mgmt_fe_destroy_client_session(struct mgmt_fe_client *client,
  */
 extern int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client,
 				   uint64_t session_id, uint64_t req_id,
-				   Mgmtd__DatastoreId ds_id, bool lock_ds);
+				   Mgmtd__DatastoreId ds_id, bool lock_ds,
+				   bool scok);
 
 /*
  * Send SET_CONFIG_REQ to MGMTD for one or more config data(s).

lib/mgmt_msg.c

@@ -548,20 +548,26 @@ int msg_conn_send_msg(struct msg_conn *conn, uint8_t version, void *msg,
 	if (conn->remote_conn && short_circuit_ok) {
 		uint8_t *buf = msg;
 		size_t n = mlen;
+		bool old;
 
 		if (packf) {
 			buf = XMALLOC(MTYPE_TMP, mlen);
 			n = packf(msg, buf);
 		}
 
+		++conn->short_circuit_depth;
 		MGMT_MSG_DBG(dbgtag, "SC send: depth %u msg: %p",
-			     ++conn->short_circuit_depth, msg);
+			     conn->short_circuit_depth, msg);
 
+		old = conn->remote_conn->is_short_circuit;
+		conn->remote_conn->is_short_circuit = true;
 		conn->remote_conn->handle_msg(version, buf, n,
 					      conn->remote_conn);
+		conn->remote_conn->is_short_circuit = old;
 
+		--conn->short_circuit_depth;
 		MGMT_MSG_DBG(dbgtag, "SC return from depth: %u msg: %p",
-			     conn->short_circuit_depth--, msg);
+			     conn->short_circuit_depth, msg);
 
 		if (packf)
 			XFREE(MTYPE_TMP, buf);
@@ -661,12 +667,10 @@ static bool msg_client_connect_short_circuit(struct msg_client *client)
 	set_nonblocking(sockets[0]);
 	setsockopt_so_sendbuf(sockets[0], client->conn.mstate.max_write_buf);
 	setsockopt_so_recvbuf(sockets[0], client->conn.mstate.max_read_buf);
-	client->conn.is_short_circuit = true;
 
 	/* server side */
 	memset(&su, 0, sizeof(union sockunion));
 	server_conn = server->create(sockets[1], &su);
-	server_conn->is_short_circuit = true;
 
 	client->conn.remote_conn = server_conn;
 	server_conn->remote_conn = &client->conn;

lib/mgmt_msg.h

@@ -98,8 +98,8 @@ struct msg_conn {
 			   struct msg_conn *conn);
 	void *user;
 	uint short_circuit_depth;
+	bool is_short_circuit;	/* true when the message being handled is SC */
 	bool is_client;
-	bool is_short_circuit;
 	bool debug;
 };
 

lib/northbound_cli.c

@@ -763,7 +763,7 @@ DEFUN (config_exclusive,
        "Configuration from vty interface\n"
        "Configure exclusively from this terminal\n")
 {
-	return vty_config_enter(vty, true, true);
+	return vty_config_enter(vty, true, true, false);
 }
 
 /* Configure using a private candidate configuration. */
@@ -773,7 +773,7 @@ DEFUN (config_private,
        "Configuration from vty interface\n"
        "Configure using a private candidate configuration\n")
 {
-	return vty_config_enter(vty, true, false);
+	return vty_config_enter(vty, true, false, false);
 }
 
 DEFPY (config_commit,

lib/vty.c

@@ -70,7 +70,6 @@ struct nb_config *vty_mgmt_candidate_config;
 
 static struct mgmt_fe_client *mgmt_fe_client;
 static bool mgmt_fe_connected;
-static bool mgmt_candidate_ds_wr_locked;
 static uint64_t mgmt_client_id_next;
 static uint64_t mgmt_last_req_id = UINT64_MAX;
 
@@ -129,6 +128,35 @@ char const *const mgmt_daemons[] = {
 };
 uint mgmt_daemons_count = array_size(mgmt_daemons);
 
+
+static int vty_mgmt_lock_candidate_inline(struct vty *vty)
+{
+	assert(!vty->mgmt_locked_candidate_ds);
+	(void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, true, true);
+	return vty->mgmt_locked_candidate_ds ? 0 : -1;
+}
+
+static int vty_mgmt_unlock_candidate_inline(struct vty *vty)
+{
+	assert(vty->mgmt_locked_candidate_ds);
+	(void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false, true);
+	return vty->mgmt_locked_candidate_ds ? -1 : 0;
+}
+
+static int vty_mgmt_lock_running_inline(struct vty *vty)
+{
+	assert(!vty->mgmt_locked_running_ds);
+	(void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_RUNNING, true, true);
+	return vty->mgmt_locked_running_ds ? 0 : -1;
+}
+
+static int vty_mgmt_unlock_running_inline(struct vty *vty)
+{
+	assert(vty->mgmt_locked_running_ds);
+	(void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_RUNNING, false, true);
+	return vty->mgmt_locked_running_ds ? -1 : 0;
+}
+
 void vty_mgmt_resume_response(struct vty *vty, bool success)
 {
 	uint8_t header[4] = {0, 0, 0, 0};
@@ -1648,12 +1676,12 @@ struct vty *vty_new(void)
 		if (!mgmt_client_id_next)
 			mgmt_client_id_next++;
 		new->mgmt_client_id = mgmt_client_id_next++;
-		if (mgmt_fe_create_client_session(
+		new->mgmt_session_id = 0;
-			    mgmt_fe_client, new->mgmt_client_id,
+		mgmt_fe_create_client_session(
-			    (uintptr_t) new) != MGMTD_SUCCESS)
+			mgmt_fe_client, new->mgmt_client_id, (uintptr_t) new);
-			zlog_err(
+		/* we short-circuit create the session so it must be set now */
-				"Failed to open a MGMTD Frontend session for VTY session %p!!",
+		assertf(new->mgmt_session_id != 0,
-				new);
+			"Failed to create client session for VTY");
 	}
 
 	return new;
@@ -2202,10 +2230,11 @@ bool mgmt_vty_read_configs(void)
 	vty->node = CONFIG_NODE;
 	vty->config = true;
 	vty->pending_allowed = true;
-	vty->candidate_config = vty_shared_candidate_config;
-	vty->mgmt_locked_candidate_ds = true;
-	mgmt_candidate_ds_wr_locked = true;
 
+	vty->candidate_config = vty_shared_candidate_config;
+
+	vty_mgmt_lock_candidate_inline(vty);
+	vty_mgmt_lock_running_inline(vty);
 
 	for (index = 0; index < array_size(mgmt_daemons); index++) {
 		snprintf(path, sizeof(path), "%s/%s.conf", frr_sysconfdir,
@@ -2250,10 +2279,15 @@ bool mgmt_vty_read_configs(void)
 		fclose(confp);
 	}
 
-	vty->pending_allowed = false;
+	/* Conditionally unlock as the config file may have "exit"d early which
+	 * would then have unlocked things.
+	 */
+	if (vty->mgmt_locked_running_ds)
+		vty_mgmt_unlock_running_inline(vty);
+	if (vty->mgmt_locked_candidate_ds)
+		vty_mgmt_unlock_candidate_inline(vty);
 
-	vty->mgmt_locked_candidate_ds = false;
+	vty->pending_allowed = false;
-	mgmt_candidate_ds_wr_locked = false;
 
 	if (!count)
 		vty_close(vty);
@@ -2367,7 +2401,7 @@ static void vtysh_read(struct event *thread)
 				 */
 				if (vty->mgmt_req_pending_cmd) {
 					MGMTD_FE_CLIENT_DBG(
-						"postpone CLI cmd response pending mgmtd %s on vty session-id %" PRIu64,
+						"postpone CLI response pending mgmtd %s on vty session-id %" PRIu64,
 						vty->mgmt_req_pending_cmd,
 						vty->mgmt_session_id);
 					return;
@@ -2453,6 +2487,9 @@ void vty_close(struct vty *vty)
 		MGMTD_FE_CLIENT_ERR(
 			"vty closed, uncommitted config will be lost.");
 
+	/* Drop out of configure / transaction if needed. */
+	vty_config_exit(vty);
+
 	if (mgmt_fe_client && vty->mgmt_session_id) {
 		MGMTD_FE_CLIENT_DBG("closing vty session");
 		mgmt_fe_destroy_client_session(mgmt_fe_client,
@@ -2460,9 +2497,6 @@ void vty_close(struct vty *vty)
 		vty->mgmt_session_id = 0;
 	}
 
-	/* Drop out of configure / transaction if needed. */
-	vty_config_exit(vty);
-
 	/* Cancel threads.*/
 	EVENT_OFF(vty->t_read);
 	EVENT_OFF(vty->t_write);
@@ -2821,28 +2855,35 @@ bool vty_read_config(struct nb_config *config, const char *config_file,
 	return true;
 }
 
-int vty_config_enter(struct vty *vty, bool private_config, bool exclusive)
+int vty_config_enter(struct vty *vty, bool private_config, bool exclusive,
+		     bool file_lock)
 {
-	if (exclusive && nb_running_lock(NB_CLIENT_CLI, vty)) {
+	if (exclusive && !vty_mgmt_fe_enabled() &&
+	    nb_running_lock(NB_CLIENT_CLI, vty)) {
 		vty_out(vty, "%% Configuration is locked by other client\n");
 		return CMD_WARNING;
 	}
 
-	if (vty_mgmt_fe_enabled()) {
+	/*
-		if (!mgmt_candidate_ds_wr_locked) {
+	 * We only need to do a lock when reading a config file as we will be
-			if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE,
+	 * sending a batch of setcfg changes followed by a single commit
-						     true) != 0) {
+	 * message. For user interactive mode we are doing implicit commits
-				vty_out(vty, "Not able to lock candidate DS\n");
+	 * those will obtain the lock (or not) when they try and commit.
-				return CMD_WARNING;
+	 */
-			}
+	if (file_lock && vty_mgmt_fe_enabled() && !private_config) {
-		} else {
+		if (vty_mgmt_lock_candidate_inline(vty)) {
 			vty_out(vty,
-				"Candidate DS already locked by different session\n");
+				"%% Can't enter config; candidate datastore locked by another session\n");
-			return CMD_WARNING;
+			return CMD_WARNING_CONFIG_FAILED;
 		}
-
+		if (vty_mgmt_lock_running_inline(vty)) {
-		vty->mgmt_locked_candidate_ds = true;
+			vty_out(vty,
-		mgmt_candidate_ds_wr_locked = true;
+				"%% Can't enter config; running datastore locked by another session\n");
+			vty_mgmt_unlock_candidate_inline(vty);
+			return CMD_WARNING_CONFIG_FAILED;
+		}
+		assert(vty->mgmt_locked_candidate_ds);
+		assert(vty->mgmt_locked_running_ds);
 	}
 
 	vty->node = CONFIG_NODE;
@@ -2855,7 +2896,9 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive)
 		vty->candidate_config_base = nb_config_dup(running_config);
 		vty_out(vty,
 			"Warning: uncommitted changes will be discarded on exit.\n\n");
-	} else {
+		return CMD_SUCCESS;
+	}
+
 	/*
 	 * NOTE: On the MGMTD daemon we point the VTY candidate DS to
 	 * the global MGMTD candidate DS. Else we point to the VTY
@@ -2865,13 +2908,12 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive)
 					? vty_mgmt_candidate_config
 					: vty_shared_candidate_config;
 	if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL)
-			vty->candidate_config_base =
+		vty->candidate_config_base = nb_config_dup(running_config);
-				nb_config_dup(running_config);
-	}
 
 	return CMD_SUCCESS;
 }
 
+
 void vty_config_exit(struct vty *vty)
 {
 	enum node_type node = vty->node;
@@ -2896,21 +2938,13 @@ int vty_config_node_exit(struct vty *vty)
 {
 	vty->xpath_index = 0;
 
-	/*
+	/* TODO: could we check for un-commited changes here? */
-	 * If we are not reading config file and we are mgmtd FE and we are
-	 * locked then unlock.
-	 */
-	if (vty->type != VTY_FILE && vty_mgmt_fe_enabled() &&
-	    mgmt_candidate_ds_wr_locked && vty->mgmt_locked_candidate_ds) {
-		if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false) !=
-		    0) {
-			vty_out(vty, "Not able to unlock candidate DS\n");
-			return CMD_WARNING;
-		}
 
-		vty->mgmt_locked_candidate_ds = false;
+	if (vty->mgmt_locked_running_ds)
-		mgmt_candidate_ds_wr_locked = false;
+		vty_mgmt_unlock_running_inline(vty);
-	}
+
+	if (vty->mgmt_locked_candidate_ds)
+		vty_mgmt_unlock_candidate_inline(vty);
 
 	/* Perform any pending commits. */
 	(void)nb_cli_pending_commit_check(vty);
@@ -3493,23 +3527,28 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client,
 
 	vty = (struct vty *)session_ctx;
 
-	if (!success) {
+	assert(ds_id == MGMTD_DS_CANDIDATE || ds_id == MGMTD_DS_RUNNING);
-		zlog_err("%socking for DS %u failed, Err: '%s'",
+	if (!success)
-			 lock_ds ? "L" : "Unl", ds_id, errmsg_if_any);
+		zlog_err("%socking for DS %u failed, Err: '%s' vty %p",
-		vty_out(vty, "ERROR: %socking for DS %u failed, Err: '%s'\n",
+			 lock_ds ? "L" : "Unl", ds_id, errmsg_if_any, vty);
-			lock_ds ? "L" : "Unl", ds_id, errmsg_if_any);
+	else {
-	} else {
 		MGMTD_FE_CLIENT_DBG("%socked DS %u successfully",
 				    lock_ds ? "L" : "Unl", ds_id);
+		if (ds_id == MGMTD_DS_CANDIDATE)
+			vty->mgmt_locked_candidate_ds = lock_ds;
+		else
+			vty->mgmt_locked_running_ds = lock_ds;
 	}
 
+	if (vty->mgmt_req_pending_cmd)
 		vty_mgmt_resume_response(vty, success);
 }
 
 static void vty_mgmt_set_config_result_notified(
 	struct mgmt_fe_client *client, uintptr_t usr_data, uint64_t client_id,
 	uintptr_t session_id, uintptr_t session_ctx, uint64_t req_id,
-	bool success, Mgmtd__DatastoreId ds_id, char *errmsg_if_any)
+	bool success, Mgmtd__DatastoreId ds_id, bool implicit_commit,
+	char *errmsg_if_any)
 {
 	struct vty *vty;
 
@@ -3527,6 +3566,12 @@ static void vty_mgmt_set_config_result_notified(
 				    client_id, req_id);
 	}
 
+	if (implicit_commit) {
+		/* In this case the changes have been applied, we are done */
+		vty_mgmt_unlock_candidate_inline(vty);
+		vty_mgmt_unlock_running_inline(vty);
+	}
+
 	vty_mgmt_resume_response(vty, success);
 }
 
@@ -3632,13 +3677,14 @@ bool vty_mgmt_should_process_cli_apply_changes(struct vty *vty)
 }
 
 int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
-			     bool lock)
+			     bool lock, bool scok)
 {
-	if (mgmt_fe_client && vty->mgmt_session_id) {
+	assert(mgmt_fe_client);
+	assert(vty->mgmt_session_id);
+
 	vty->mgmt_req_id++;
-		if (mgmt_fe_send_lockds_req(mgmt_fe_client,
+	if (mgmt_fe_send_lockds_req(mgmt_fe_client, vty->mgmt_session_id,
-					    vty->mgmt_session_id,
+				    vty->mgmt_req_id, ds_id, lock, scok)) {
-					    vty->mgmt_req_id, ds_id, lock)) {
 		zlog_err("Failed sending %sLOCK-DS-REQ req-id %" PRIu64,
 			 lock ? "" : "UN", vty->mgmt_req_id);
 		vty_out(vty, "Failed to send %sLOCK-DS-REQ to MGMTD!\n",
@@ -3646,8 +3692,8 @@ int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
 		return -1;
 	}
 
+	if (!scok)
 		vty->mgmt_req_pending_cmd = "MESSAGE_LOCKDS_REQ";
-	}
 
 	return 0;
 }
@@ -3659,7 +3705,6 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 	Mgmtd__YangCfgDataReq cfg_req[VTY_MAXCFGCHANGES];
 	Mgmtd__YangCfgDataReq *cfgreq[VTY_MAXCFGCHANGES] = {0};
 	size_t indx;
-	int cnt;
 
 	if (vty->type == VTY_FILE) {
 		/*
@@ -3667,45 +3712,51 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 		 * changes until we are done reading the file and have modified
 		 * the local candidate DS.
 		 */
-		assert(vty->mgmt_locked_candidate_ds);
 		/* no-one else should be sending data right now */
 		assert(!vty->mgmt_num_pending_setcfg);
 		return 0;
 	}
 
+	/* If we are FE client and we have a vty then we have a session */
+	assert(mgmt_fe_client && vty->mgmt_client_id && vty->mgmt_session_id);
 
-	if (mgmt_fe_client && vty->mgmt_client_id && !vty->mgmt_session_id) {
+	if (!vty->num_cfg_changes)
-		/*
-		 * We are connected to mgmtd but we do not yet have an
-		 * established session. this means we need to send any changes
-		 * made during this "down-time" to all backend clients when this
-		 * FE client finishes coming up.
-		 */
-		MGMTD_FE_CLIENT_DBG("skipping as no session exists");
 		return 0;
+
+	/* grab the candidate and running lock prior to sending implicit commit
+	 * command
+	 */
+	if (implicit_commit) {
+		if (vty_mgmt_lock_candidate_inline(vty)) {
+			vty_out(vty,
+				"%% command failed, could not lock candidate DS\n");
+			return -1;
+		} else if (vty_mgmt_lock_running_inline(vty)) {
+			vty_out(vty,
+				"%% command failed, could not lock running DS\n");
+			return -1;
+		}
 	}
 
-	if (mgmt_fe_client && vty->mgmt_session_id) {
-		cnt = 0;
 	for (indx = 0; indx < vty->num_cfg_changes; indx++) {
-			mgmt_yang_data_init(&cfg_data[cnt]);
+		mgmt_yang_data_init(&cfg_data[indx]);
 
 		if (vty->cfg_changes[indx].value) {
-				mgmt_yang_data_value_init(&value[cnt]);
+			mgmt_yang_data_value_init(&value[indx]);
-				value[cnt].encoded_str_val =
+			value[indx].encoded_str_val =
 				(char *)vty->cfg_changes[indx].value;
-				value[cnt].value_case =
+			value[indx].value_case =
 				MGMTD__YANG_DATA_VALUE__VALUE_ENCODED_STR_VAL;
-				cfg_data[cnt].value = &value[cnt];
+			cfg_data[indx].value = &value[indx];
 		}
 
-			cfg_data[cnt].xpath = vty->cfg_changes[indx].xpath;
+		cfg_data[indx].xpath = vty->cfg_changes[indx].xpath;
 
-			mgmt_yang_cfg_data_req_init(&cfg_req[cnt]);
+		mgmt_yang_cfg_data_req_init(&cfg_req[indx]);
-			cfg_req[cnt].data = &cfg_data[cnt];
+		cfg_req[indx].data = &cfg_data[indx];
 		switch (vty->cfg_changes[indx].operation) {
 		case NB_OP_DESTROY:
-				cfg_req[cnt].req_type =
+			cfg_req[indx].req_type =
 				MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA;
 			break;
 
@@ -3714,7 +3765,7 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 		case NB_OP_MOVE:
 		case NB_OP_PRE_VALIDATE:
 		case NB_OP_APPLY_FINISH:
-				cfg_req[cnt].req_type =
+			cfg_req[indx].req_type =
 				MGMTD__CFG_DATA_REQ_TYPE__SET_DATA;
 			break;
 		case NB_OP_GET_ELEM:
@@ -3722,30 +3773,30 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 		case NB_OP_GET_KEYS:
 		case NB_OP_LOOKUP_ENTRY:
 		case NB_OP_RPC:
-				assert(!"Invalid type of operation");
-				break;
 		default:
-				assert(!"non-enum value, invalid");
+			assertf(false,
+				"Invalid operation type for send config: %d",
+				vty->cfg_changes[indx].operation);
+			/*NOTREACHED*/
+			abort();
 		}
 
-			cfgreq[cnt] = &cfg_req[cnt];
+		cfgreq[indx] = &cfg_req[indx];
-			cnt++;
 	}
+	if (!indx)
+		return 0;
 
 	vty->mgmt_req_id++;
-		if (cnt && mgmt_fe_send_setcfg_req(
+	if (mgmt_fe_send_setcfg_req(mgmt_fe_client, vty->mgmt_session_id,
-				   mgmt_fe_client, vty->mgmt_session_id,
+				    vty->mgmt_req_id, MGMTD_DS_CANDIDATE,
-				   vty->mgmt_req_id, MGMTD_DS_CANDIDATE, cfgreq,
+				    cfgreq, indx, implicit_commit,
-				   cnt, implicit_commit,
+				    MGMTD_DS_RUNNING)) {
-				   MGMTD_DS_RUNNING) != MGMTD_SUCCESS) {
+		zlog_err("Failed to send %zu config xpaths to mgmtd", indx);
-			zlog_err("Failed to send %d Config Xpaths to MGMTD!!",
+		vty_out(vty, "%% Failed to send commands to mgmtd\n");
-				 (int)indx);
-			vty_out(vty, "Failed to send SETCFG-REQ to MGMTD!\n");
 		return -1;
 	}
 
 	vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ";
-	}
 
 	return 0;
 }

lib/vty.h

@@ -230,6 +230,7 @@ struct vty {
 	 * vty user. */
 	const char *mgmt_req_pending_cmd;
 	bool mgmt_locked_candidate_ds;
+	bool mgmt_locked_running_ds;
 };
 
 static inline void vty_push_context(struct vty *vty, int node, uint64_t id)
@@ -390,7 +391,7 @@ extern void vty_close(struct vty *);
 extern char *vty_get_cwd(void);
 extern void vty_update_xpath(const char *oldpath, const char *newpath);
 extern int vty_config_enter(struct vty *vty, bool private_config,
-			    bool exclusive);
+			    bool exclusive, bool file_lock);
 extern void vty_config_exit(struct vty *);
 extern int vty_config_node_exit(struct vty *);
 extern int vty_shell(struct vty *);
@@ -416,7 +417,7 @@ extern int vty_mgmt_send_get_config(struct vty *vty,
 extern int vty_mgmt_send_get_data(struct vty *vty, Mgmtd__DatastoreId datastore,
 				  const char **xpath_list, int num_req);
 extern int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
-				    bool lock);
+				    bool lock, bool scok);
 extern void vty_mgmt_resume_response(struct vty *vty, bool success);
 
 static inline bool vty_needs_implicit_commit(struct vty *vty)

mgmtd/mgmt_be_adapter.c

@@ -648,8 +648,7 @@ static void mgmt_be_adapter_process_msg(uint8_t version, uint8_t *data,
 	mgmtd__be_message__free_unpacked(be_msg, NULL);
 }
 
-static void mgmt_be_iter_and_get_cfg(struct mgmt_ds_ctx *ds_ctx,
+static void mgmt_be_iter_and_get_cfg(const char *xpath, struct lyd_node *node,
-				     const char *xpath, struct lyd_node *node,
 				     struct nb_node *nb_node, void *ctx)
 {
 	struct mgmt_be_get_adapter_config_params *parms = ctx;
@@ -806,10 +805,10 @@ mgmt_be_get_adapter_by_name(const char *name)
 }
 
 int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter,
-				  struct mgmt_ds_ctx *ds_ctx,
 			       struct nb_config_cbs **cfg_chgs)
 {
 	struct mgmt_be_get_adapter_config_params parms;
+	struct nb_config *cfg_root = mgmt_ds_get_nb_config(mm->running_ds);
 
 	assert(cfg_chgs);
 
@@ -825,8 +824,8 @@ int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter,
 		parms.cfg_chgs = &adapter->cfg_chgs;
 		parms.seq = 0;
 
-		mgmt_ds_iter_data(ds_ctx, "", mgmt_be_iter_and_get_cfg,
+		mgmt_ds_iter_data(MGMTD_DS_RUNNING, cfg_root, "",
-				  (void *)&parms);
+				  mgmt_be_iter_and_get_cfg, (void *)&parms);
 	}
 
 	*cfg_chgs = &adapter->cfg_chgs;

mgmtd/mgmt_be_adapter.h

@@ -110,9 +110,7 @@ extern struct mgmt_be_client_adapter *
 mgmt_be_get_adapter_by_id(enum mgmt_be_client_id id);
 
 /* Fetch backend adapter config. */
-extern int
+extern int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter,
-mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter,
-			      struct mgmt_ds_ctx *ds_ctx,
 				      struct nb_config_cbs **cfg_chgs);
 
 /* Create/destroy a transaction. */

mgmtd/mgmt_ds.c

@@ -22,7 +22,9 @@
 
 struct mgmt_ds_ctx {
 	Mgmtd__DatastoreId ds_id;
-	int lock; /* 0 unlocked, >0 read locked < write locked */
+
+	bool locked;
+	uint64_t vty_session_id; /* Owner of the lock or 0 */
 
 	bool config_ds;
 
@@ -76,29 +78,20 @@ static int mgmt_ds_dump_in_memory(struct mgmt_ds_ctx *ds_ctx,
 static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
 					   struct mgmt_ds_ctx *dst)
 {
-	struct lyd_node *dst_dnode, *src_dnode;
-
 	if (!src || !dst)
 		return -1;
 
 	MGMTD_DS_DBG("Replacing %s with %s", mgmt_ds_id2name(dst->ds_id),
 		     mgmt_ds_id2name(src->ds_id));
 
-	src_dnode = src->config_ds ? src->root.cfg_root->dnode
+	if (src->config_ds && dst->config_ds)
-				   : dst->root.dnode_root;
+		nb_config_replace(dst->root.cfg_root, src->root.cfg_root, true);
-	dst_dnode = dst->config_ds ? dst->root.cfg_root->dnode
+	else {
-				   : dst->root.dnode_root;
+		assert(!src->config_ds && !dst->config_ds);
-
+		if (dst->root.dnode_root)
-	if (dst_dnode)
+			yang_dnode_free(dst->root.dnode_root);
-		yang_dnode_free(dst_dnode);
+		dst->root.dnode_root = yang_dnode_dup(src->root.dnode_root);
-
+	}
-	/* Not using nb_config_replace as the oper ds does not contain nb_config
-	 */
-	dst_dnode = yang_dnode_dup(src_dnode);
-	if (dst->config_ds)
-		dst->root.cfg_root->dnode = dst_dnode;
-	else
-		dst->root.dnode_root = dst_dnode;
 
 	if (src->ds_id == MGMTD_DS_CANDIDATE) {
 		/*
@@ -108,8 +101,6 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
 		nb_config_diff_del_changes(&src->root.cfg_root->cfg_chgs);
 	}
 
-	/* TODO: Update the versions if nb_config present */
-
 	return 0;
 }
 
@@ -117,20 +108,21 @@ static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src,
 					 struct mgmt_ds_ctx *dst)
 {
 	int ret;
-	struct lyd_node **dst_dnode, *src_dnode;
 
 	if (!src || !dst)
 		return -1;
 
 	MGMTD_DS_DBG("Merging DS %d with %d", dst->ds_id, src->ds_id);
-
+	if (src->config_ds && dst->config_ds)
-	src_dnode = src->config_ds ? src->root.cfg_root->dnode
+		ret = nb_config_merge(dst->root.cfg_root, src->root.cfg_root,
-				   : dst->root.dnode_root;
+				      true);
-	dst_dnode = dst->config_ds ? &dst->root.cfg_root->dnode
+	else {
-				   : &dst->root.dnode_root;
+		assert(!src->config_ds && !dst->config_ds);
-	ret = lyd_merge_siblings(dst_dnode, src_dnode, 0);
+		ret = lyd_merge_siblings(&dst->root.dnode_root,
+					 src->root.dnode_root, 0);
+	}
 	if (ret != 0) {
-		MGMTD_DS_ERR("lyd_merge() failed with err %d", ret);
+		MGMTD_DS_ERR("merge failed with err: %d", ret);
 		return ret;
 	}
 
@@ -212,9 +204,11 @@ int mgmt_ds_init(struct mgmt_master *mm)
 
 void mgmt_ds_destroy(void)
 {
-	/*
+	nb_config_free(candidate.root.cfg_root);
-	 * TODO: Free the datastores.
+	candidate.root.cfg_root = NULL;
-	 */
+
+	yang_dnode_free(oper.root.dnode_root);
+	oper.root.dnode_root = NULL;
 }
 
 struct mgmt_ds_ctx *mgmt_ds_get_ctx_by_id(struct mgmt_master *mm,
@@ -244,40 +238,33 @@ bool mgmt_ds_is_config(struct mgmt_ds_ctx *ds_ctx)
 	return ds_ctx->config_ds;
 }
 
-int mgmt_ds_read_lock(struct mgmt_ds_ctx *ds_ctx)
+bool mgmt_ds_is_locked(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id)
 {
-	if (!ds_ctx)
+	assert(ds_ctx);
-		return EINVAL;
+	return (ds_ctx->locked && ds_ctx->vty_session_id == session_id);
-	if (ds_ctx->lock < 0)
+}
+
+int mgmt_ds_lock(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id)
+{
+	assert(ds_ctx);
+
+	if (ds_ctx->locked)
 		return EBUSY;
-	++ds_ctx->lock;
+
+	ds_ctx->locked = true;
+	ds_ctx->vty_session_id = session_id;
 	return 0;
 }
 
-int mgmt_ds_write_lock(struct mgmt_ds_ctx *ds_ctx)
+void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx)
 {
-	if (!ds_ctx)
+	assert(ds_ctx);
-		return EINVAL;
+	if (!ds_ctx->locked)
-	if (ds_ctx->lock != 0)
+		zlog_warn(
-		return EBUSY;
+			"%s: WARNING: unlock on unlocked in DS:%s last session-id %" PRIu64,
-	ds_ctx->lock = -1;
+			__func__, mgmt_ds_id2name(ds_ctx->ds_id),
-	return 0;
+			ds_ctx->vty_session_id);
-}
+	ds_ctx->locked = 0;
-
-int mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx)
-{
-	if (!ds_ctx)
-		return EINVAL;
-	if (ds_ctx->lock > 0)
-		--ds_ctx->lock;
-	else if (ds_ctx->lock < 0) {
-		assert(ds_ctx->lock == -1);
-		ds_ctx->lock = 0;
-	} else {
-		assert(ds_ctx->lock != 0);
-		return EINVAL;
-	}
-	return 0;
 }
 
 int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx,
@@ -314,10 +301,9 @@ struct nb_config *mgmt_ds_get_nb_config(struct mgmt_ds_ctx *ds_ctx)
 }
 
 static int mgmt_walk_ds_nodes(
-	struct mgmt_ds_ctx *ds_ctx, const char *base_xpath,
+	struct nb_config *root, const char *base_xpath,
 	struct lyd_node *base_dnode,
-	void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx,
+	void (*mgmt_ds_node_iter_fn)(const char *xpath, struct lyd_node *node,
-				     const char *xpath, struct lyd_node *node,
 				     struct nb_node *nb_node, void *ctx),
 	void *ctx)
 {
@@ -336,10 +322,7 @@ static int mgmt_walk_ds_nodes(
 		 * This function only returns the first node of a possible set
 		 * of matches issuing a warning if more than 1 matches
 		 */
-		base_dnode = yang_dnode_get(
+		base_dnode = yang_dnode_get(root->dnode, base_xpath);
-			ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode
-					   : ds_ctx->root.dnode_root,
-			base_xpath);
 	if (!base_dnode)
 		return -1;
 
@@ -348,7 +331,7 @@ static int mgmt_walk_ds_nodes(
 			       sizeof(xpath)));
 
 	nbnode = (struct nb_node *)base_dnode->schema->priv;
-	(*mgmt_ds_node_iter_fn)(ds_ctx, base_xpath, base_dnode, nbnode, ctx);
+	(*mgmt_ds_node_iter_fn)(base_xpath, base_dnode, nbnode, ctx);
 
 	/*
 	 * If the base_xpath points to a leaf node we can skip the tree walk.
@@ -370,7 +353,7 @@ static int mgmt_walk_ds_nodes(
 
 		MGMTD_DS_DBG(" -- Child xpath: %s", xpath);
 
-		ret = mgmt_walk_ds_nodes(ds_ctx, xpath, dnode,
+		ret = mgmt_walk_ds_nodes(root, xpath, dnode,
 					 mgmt_ds_node_iter_fn, ctx);
 		if (ret != 0)
 			break;
@@ -459,9 +442,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst,
 	return 0;
 }
 
-int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath,
+int mgmt_ds_iter_data(Mgmtd__DatastoreId ds_id, struct nb_config *root,
-		      void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx,
+		      const char *base_xpath,
-						   const char *xpath,
+		      void (*mgmt_ds_node_iter_fn)(const char *xpath,
 						   struct lyd_node *node,
 						   struct nb_node *nb_node,
 						   void *ctx),
@@ -472,7 +455,7 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath,
 	struct lyd_node *base_dnode = NULL;
 	struct lyd_node *node;
 
-	if (!ds_ctx)
+	if (!root)
 		return -1;
 
 	strlcpy(xpath, base_xpath, sizeof(xpath));
@@ -484,12 +467,11 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath,
 	 * Oper-state should be kept in mind though for the prefix walk
 	 */
 
-	MGMTD_DS_DBG(" -- START DS walk for DSid: %d", ds_ctx->ds_id);
+	MGMTD_DS_DBG(" -- START DS walk for DSid: %d", ds_id);
 
 	/* If the base_xpath is empty then crawl the sibblings */
 	if (xpath[0] == 0) {
-		base_dnode = ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode
+		base_dnode = root->dnode;
-						: ds_ctx->root.dnode_root;
 
 		/* get first top-level sibling */
 		while (base_dnode->parent)
@@ -499,11 +481,11 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath,
 			base_dnode = base_dnode->prev;
 
 		LY_LIST_FOR (base_dnode, node) {
-			ret = mgmt_walk_ds_nodes(ds_ctx, xpath, node,
+			ret = mgmt_walk_ds_nodes(root, xpath, node,
 						 mgmt_ds_node_iter_fn, ctx);
 		}
 	} else
-		ret = mgmt_walk_ds_nodes(ds_ctx, xpath, base_dnode,
+		ret = mgmt_walk_ds_nodes(root, xpath, base_dnode,
 					 mgmt_ds_node_iter_fn, ctx);
 
 	return ret;

mgmtd/mgmt_ds.h

@@ -179,19 +179,19 @@ extern struct mgmt_ds_ctx *mgmt_ds_get_ctx_by_id(struct mgmt_master *mm,
 extern bool mgmt_ds_is_config(struct mgmt_ds_ctx *ds_ctx);
 
 /*
- * Acquire read lock to a ds given a ds_handle
+ * Check if a given datastore is locked by a given session
  */
-extern int mgmt_ds_read_lock(struct mgmt_ds_ctx *ds_ctx);
+extern bool mgmt_ds_is_locked(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id);
 
 /*
  * Acquire write lock to a ds given a ds_handle
  */
-extern int mgmt_ds_write_lock(struct mgmt_ds_ctx *ds_ctx);
+extern int mgmt_ds_lock(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id);
 
 /*
  * Remove a lock from ds given a ds_handle
  */
-extern int mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx);
+extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx);
 
 /*
  * Copy from source to destination datastore.
@@ -233,8 +233,11 @@ extern int mgmt_ds_delete_data_nodes(struct mgmt_ds_ctx *ds_ctx,
 /*
  * Iterate over datastore data.
  *
- * ds_ctx
+ * ds_id
- *    Datastore context.
+ *    Datastore ID..
+ *
+ * root
+ *    The root of the tree to iterate over.
  *
  * base_xpath
  *    Base YANG xpath from where needs to be iterated.
@@ -252,9 +255,9 @@ extern int mgmt_ds_delete_data_nodes(struct mgmt_ds_ctx *ds_ctx,
  *    0 on success, -1 on failure.
  */
 extern int mgmt_ds_iter_data(
-	struct mgmt_ds_ctx *ds_ctx, const char *base_xpath,
+	Mgmtd__DatastoreId ds_id, struct nb_config *root,
-	void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx,
+	const char *base_xpath,
-				     const char *xpath, struct lyd_node *node,
+	void (*mgmt_ds_node_iter_fn)(const char *xpath, struct lyd_node *node,
 				     struct nb_node *nb_node, void *ctx),
 	void *ctx);
 

mgmtd/mgmt_fe_adapter.c

@@ -40,9 +40,7 @@ struct mgmt_fe_session_ctx {
 	uint64_t client_id;
 	uint64_t txn_id;
 	uint64_t cfg_txn_id;
-	uint8_t ds_write_locked[MGMTD_DS_MAX_ID];
+	uint8_t ds_locked[MGMTD_DS_MAX_ID];
-	uint8_t ds_read_locked[MGMTD_DS_MAX_ID];
-	uint8_t ds_locked_implict[MGMTD_DS_MAX_ID];
 	struct event *proc_cfg_txn_clnp;
 	struct event *proc_show_txn_clnp;
 
@@ -72,8 +70,12 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id,
 				  struct mgmt_ds_ctx *ds_ctx,
 				  struct mgmt_fe_session_ctx *session)
 {
-	if (!session->ds_write_locked[ds_id]) {
+	if (session->ds_locked[ds_id])
-		if (mgmt_ds_write_lock(ds_ctx) != 0) {
+		zlog_warn("multiple lock taken by session-id: %" PRIu64
+			  " on DS:%s",
+			  session->session_id, mgmt_ds_id2name(ds_id));
+	else {
+		if (mgmt_ds_lock(ds_ctx, session->session_id)) {
 			MGMTD_FE_ADAPTER_DBG(
 				"Failed to lock the DS:%s for session-id: %" PRIu64
 				" from %s!",
@@ -82,7 +84,7 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id,
 			return -1;
 		}
 
-		session->ds_write_locked[ds_id] = true;
+		session->ds_locked[ds_id] = true;
 		MGMTD_FE_ADAPTER_DBG(
 			"Write-Locked the DS:%s for session-id: %" PRIu64
 			" from %s",
@@ -93,97 +95,32 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id,
 	return 0;
 }
 
-static int
+static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id,
-mgmt_fe_session_read_lock_ds(Mgmtd__DatastoreId ds_id,
 				      struct mgmt_ds_ctx *ds_ctx,
 				      struct mgmt_fe_session_ctx *session)
 {
-	if (!session->ds_read_locked[ds_id]) {
+	if (!session->ds_locked[ds_id])
-		if (mgmt_ds_read_lock(ds_ctx) != 0) {
+		zlog_warn("unlock unlocked by session-id: %" PRIu64 " on DS:%s",
-			MGMTD_FE_ADAPTER_DBG(
+			  session->session_id, mgmt_ds_id2name(ds_id));
-				"Failed to lock the DS:%s for session-is: %" PRIu64
-				" from %s",
-				mgmt_ds_id2name(ds_id), session->session_id,
-				session->adapter->name);
-			return -1;
-		}
-
-		session->ds_read_locked[ds_id] = true;
-		MGMTD_FE_ADAPTER_DBG(
-			"Read-Locked the DS:%s for session-id: %" PRIu64
-			" from %s",
-			mgmt_ds_id2name(ds_id), session->session_id,
-			session->adapter->name);
-	}
-
-	return 0;
-}
-
-static int mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id,
-					 struct mgmt_ds_ctx *ds_ctx,
-					 struct mgmt_fe_session_ctx *session,
-					 bool unlock_write, bool unlock_read)
-{
-	if (unlock_write && session->ds_write_locked[ds_id]) {
-		session->ds_write_locked[ds_id] = false;
-		session->ds_locked_implict[ds_id] = false;
-		if (mgmt_ds_unlock(ds_ctx) != 0) {
-			MGMTD_FE_ADAPTER_DBG(
-				"Failed to unlock the DS:%s taken earlier by session-id: %" PRIu64
-				" from %s",
-				mgmt_ds_id2name(ds_id), session->session_id,
-				session->adapter->name);
-			return -1;
-		}
 
+	session->ds_locked[ds_id] = false;
+	mgmt_ds_unlock(ds_ctx);
 	MGMTD_FE_ADAPTER_DBG(
 		"Unlocked DS:%s write-locked earlier by session-id: %" PRIu64
 		" from %s",
 		mgmt_ds_id2name(ds_id), session->session_id,
 		session->adapter->name);
-	} else if (unlock_read && session->ds_read_locked[ds_id]) {
-		session->ds_read_locked[ds_id] = false;
-		session->ds_locked_implict[ds_id] = false;
-		if (mgmt_ds_unlock(ds_ctx) != 0) {
-			MGMTD_FE_ADAPTER_DBG(
-				"Failed to unlock the DS:%s taken earlier by session-id: %" PRIu64
-				" from %s",
-				mgmt_ds_id2name(ds_id), session->session_id,
-				session->adapter->name);
-			return -1;
-		}
-
-		MGMTD_FE_ADAPTER_DBG(
-			"Unlocked DS:%s read-locked earlier by session-id: %" PRIu64
-			" from %s",
-			mgmt_ds_id2name(ds_id), session->session_id,
-			session->adapter->name);
-	}
-
-	return 0;
 }
 
 static void
 mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session)
 {
-	Mgmtd__DatastoreId ds_id;
-	struct mgmt_ds_ctx *ds_ctx;
-
 	/*
 	 * Ensure any uncommitted changes in Candidate DS
 	 * is discarded.
 	 */
 	mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false);
 
-	for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) {
-		ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id);
-		if (ds_ctx) {
-			if (session->ds_locked_implict[ds_id])
-				mgmt_fe_session_unlock_ds(
-					ds_id, ds_ctx, session, true, false);
-		}
-	}
-
 	/*
 	 * Destroy the actual transaction created earlier.
 	 */
@@ -194,17 +131,6 @@ mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session)
 static void
 mgmt_fe_session_show_txn_cleanup(struct mgmt_fe_session_ctx *session)
 {
-	Mgmtd__DatastoreId ds_id;
-	struct mgmt_ds_ctx *ds_ctx;
-
-	for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) {
-		ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id);
-		if (ds_ctx) {
-			mgmt_fe_session_unlock_ds(ds_id, ds_ctx, session,
-						      false, true);
-		}
-	}
-
 	/*
 	 * Destroy the transaction created recently.
 	 */
@@ -245,25 +171,29 @@ mgmt_fe_session_compute_commit_timers(struct mgmt_commit_stats *cmt_stats)
 	}
 }
 
-static void mgmt_fe_cleanup_session(struct mgmt_fe_session_ctx **session)
+static void mgmt_fe_cleanup_session(struct mgmt_fe_session_ctx **sessionp)
 {
-	if ((*session)->adapter) {
+	Mgmtd__DatastoreId ds_id;
-		mgmt_fe_session_cfg_txn_cleanup((*session));
+	struct mgmt_ds_ctx *ds_ctx;
-		mgmt_fe_session_show_txn_cleanup((*session));
+	struct mgmt_fe_session_ctx *session = *sessionp;
-		mgmt_fe_session_unlock_ds(MGMTD_DS_CANDIDATE, mm->candidate_ds,
-					  *session, true, true);
-		mgmt_fe_session_unlock_ds(MGMTD_DS_RUNNING, mm->running_ds,
-					  *session, true, true);
 
-		mgmt_fe_sessions_del(&(*session)->adapter->fe_sessions,
+	if (session->adapter) {
-				     *session);
+		mgmt_fe_session_cfg_txn_cleanup(session);
-		assert((*session)->adapter->refcount > 1);
+		mgmt_fe_session_show_txn_cleanup(session);
-		mgmt_fe_adapter_unlock(&(*session)->adapter);
+		for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) {
+			ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id);
+			if (ds_ctx && session->ds_locked[ds_id])
+				mgmt_fe_session_unlock_ds(ds_id, ds_ctx,
+							  session);
+		}
+		mgmt_fe_sessions_del(&session->adapter->fe_sessions, session);
+		assert(session->adapter->refcount > 1);
+		mgmt_fe_adapter_unlock(&session->adapter);
 	}
 
-	hash_release(mgmt_fe_sessions, *session);
+	hash_release(mgmt_fe_sessions, session);
-	XFREE(MTYPE_MGMTD_FE_SESSION, *session);
+	XFREE(MTYPE_MGMTD_FE_SESSION, session);
-	*session = NULL;
+	*sessionp = NULL;
 }
 
 static struct mgmt_fe_session_ctx *
@@ -389,6 +319,7 @@ static int mgmt_fe_send_lockds_reply(struct mgmt_fe_session_ctx *session,
 {
 	Mgmtd__FeMessage fe_msg;
 	Mgmtd__FeLockDsReply lockds_reply;
+	bool scok = session->adapter->conn->is_short_circuit;
 
 	assert(session->adapter);
 
@@ -406,10 +337,10 @@ static int mgmt_fe_send_lockds_reply(struct mgmt_fe_session_ctx *session,
 	fe_msg.lockds_reply = &lockds_reply;
 
 	MGMTD_FE_ADAPTER_DBG(
-		"Sending LOCK_DS_REPLY message to MGMTD Frontend client '%s'",
+		"Sending LOCK_DS_REPLY message to MGMTD Frontend client '%s' scok: %d",
-		session->adapter->name);
+		session->adapter->name, scok);
 
-	return mgmt_fe_adapter_send_msg(session->adapter, &fe_msg, false);
+	return mgmt_fe_adapter_send_msg(session->adapter, &fe_msg, scok);
 }
 
 static int mgmt_fe_send_setcfg_reply(struct mgmt_fe_session_ctx *session,
@@ -432,6 +363,7 @@ static int mgmt_fe_send_setcfg_reply(struct mgmt_fe_session_ctx *session,
 	setcfg_reply.ds_id = ds_id;
 	setcfg_reply.req_id = req_id;
 	setcfg_reply.success = success;
+	setcfg_reply.implicit_commit = implicit_commit;
 	if (error_if_any)
 		setcfg_reply.error_if_any = (char *)error_if_any;
 
@@ -670,8 +602,7 @@ static int mgmt_fe_adapter_notify_disconnect(struct msg_conn *conn)
 }
 
 /*
- * XXX chopps: get rid of this, we should have deleted sessions when there was a
+ * Purge any old connections that share the same client name with `adapter`
- * disconnect
  */
 static void
 mgmt_fe_adapter_cleanup_old_conn(struct mgmt_fe_client_adapter *adapter)
@@ -679,18 +610,17 @@ mgmt_fe_adapter_cleanup_old_conn(struct mgmt_fe_client_adapter *adapter)
 	struct mgmt_fe_client_adapter *old;
 
 	FOREACH_ADAPTER_IN_LIST (old) {
-		if (old != adapter &&
+		if (old == adapter)
-		    !strncmp(adapter->name, old->name, sizeof(adapter->name))) {
+			continue;
-			/*
+		if (strncmp(adapter->name, old->name, sizeof(adapter->name)))
-			 * We have a Zombie lingering around
+			continue;
-			 */
+
 		MGMTD_FE_ADAPTER_DBG(
-				"Client '%s' (FD:%d) seems to have reconnected. Removing old connection (FD:%d)!",
+			"Client '%s' (FD:%d) seems to have reconnected. Removing old connection (FD:%d)",
 			adapter->name, adapter->conn->fd,
 			old->conn->fd);
 		msg_conn_disconnect(old->conn, false);
 	}
-	}
 }
 
 static int
@@ -699,16 +629,12 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session,
 {
 	struct mgmt_ds_ctx *ds_ctx;
 
-	/*
+	if (lockds_req->ds_id != MGMTD_DS_CANDIDATE &&
-	 * Next check first if the SETCFG_REQ is for Candidate DS
+	    lockds_req->ds_id != MGMTD_DS_RUNNING) {
-	 * or not. Report failure if its not. MGMTD currently only
-	 * supports editing the Candidate DS.
-	 */
-	if (lockds_req->ds_id != MGMTD_DS_CANDIDATE) {
 		mgmt_fe_send_lockds_reply(
 			session, lockds_req->ds_id, lockds_req->req_id,
 			lockds_req->lock, false,
-			"Lock/Unlock on datastores other than Candidate DS not permitted!");
+			"Lock/Unlock on DS other than candidate or running DS not supported");
 		return -1;
 	}
 
@@ -731,10 +657,8 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session,
 				"Lock already taken on DS by another session!");
 			return -1;
 		}
-
-		session->ds_locked_implict[lockds_req->ds_id] = false;
 	} else {
-		if (!session->ds_write_locked[lockds_req->ds_id]) {
+		if (!session->ds_locked[lockds_req->ds_id]) {
 			mgmt_fe_send_lockds_reply(
 				session, lockds_req->ds_id, lockds_req->req_id,
 				lockds_req->lock, false,
@@ -742,8 +666,7 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session,
 			return 0;
 		}
 
-		(void)mgmt_fe_session_unlock_ds(lockds_req->ds_id, ds_ctx,
+		mgmt_fe_session_unlock_ds(lockds_req->ds_id, ds_ctx, session);
-						    session, true, false);
 	}
 
 	if (mgmt_fe_send_lockds_reply(session, lockds_req->ds_id,
@@ -769,79 +692,49 @@ static int
 mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session,
 					  Mgmtd__FeSetConfigReq *setcfg_req)
 {
-	uint64_t cfg_session_id;
+	struct mgmt_ds_ctx *ds_ctx, *dst_ds_ctx = NULL;
-	struct mgmt_ds_ctx *ds_ctx, *dst_ds_ctx;
+	bool txn_created = false;
 
 	if (mm->perf_stats_en)
 		gettimeofday(&session->adapter->setcfg_stats.last_start, NULL);
 
-	/*
+	/* MGMTD currently only supports editing the candidate DS. */
-	 * Next check first if the SETCFG_REQ is for Candidate DS
-	 * or not. Report failure if its not. MGMTD currently only
-	 * supports editing the Candidate DS.
-	 */
 	if (setcfg_req->ds_id != MGMTD_DS_CANDIDATE) {
 		mgmt_fe_send_setcfg_reply(
 			session, setcfg_req->ds_id, setcfg_req->req_id, false,
-			"Set-Config on datastores other than Candidate DS not permitted!",
+			"Set-Config on datastores other than Candidate DS not supported",
 			setcfg_req->implicit_commit);
 		return 0;
 	}
-
-	/*
-	 * Get the DS handle.
-	 */
 	ds_ctx = mgmt_ds_get_ctx_by_id(mm, setcfg_req->ds_id);
-	if (!ds_ctx) {
+	assert(ds_ctx);
+
+	/* MGMTD currently only supports targetting the running DS. */
+	if (setcfg_req->implicit_commit &&
+	    setcfg_req->commit_ds_id != MGMTD_DS_RUNNING) {
 		mgmt_fe_send_setcfg_reply(
 			session, setcfg_req->ds_id, setcfg_req->req_id, false,
-			"No such DS exists!", setcfg_req->implicit_commit);
+			"Implicit commit on datastores other than running DS not supported",
+			setcfg_req->implicit_commit);
+		return 0;
+	}
+	dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, setcfg_req->commit_ds_id);
+	assert(dst_ds_ctx);
+
+	/* User should have write lock to change the DS */
+	if (!session->ds_locked[setcfg_req->ds_id]) {
+		mgmt_fe_send_setcfg_reply(session, setcfg_req->ds_id,
+					  setcfg_req->req_id, false,
+					  "Candidate DS is not locked",
+					  setcfg_req->implicit_commit);
 		return 0;
 	}
 
 	if (session->cfg_txn_id == MGMTD_TXN_ID_NONE) {
-		/*
+		/* as we have the lock no-one else should have a config txn */
-		 * Check first if the current session can run a CONFIG
+		assert(mgmt_config_txn_in_progress() == MGMTD_SESSION_ID_NONE);
-		 * transaction or not. Report failure if a CONFIG transaction
-		 * from another session is already in progress.
-		 */
-		cfg_session_id = mgmt_config_txn_in_progress();
-		if (cfg_session_id != MGMTD_SESSION_ID_NONE) {
-			assert(cfg_session_id != session->session_id);
-			mgmt_fe_send_setcfg_reply(
-				session, setcfg_req->ds_id, setcfg_req->req_id,
-				false,
-				"Configuration already in-progress through a different user session!",
-				setcfg_req->implicit_commit);
-			goto mgmt_fe_sess_handle_setcfg_req_failed;
-		}
 
-
+		/* Start a CONFIG Transaction (if not started already) */
-		/*
-		 * Try taking write-lock on the requested DS (if not already).
-		 */
-		if (!session->ds_write_locked[setcfg_req->ds_id]) {
-			MGMTD_FE_ADAPTER_ERR(
-				"SETCFG_REQ on session-id: %" PRIu64
-				" without obtaining lock",
-				session->session_id);
-			if (mgmt_fe_session_write_lock_ds(setcfg_req->ds_id,
-							      ds_ctx, session)
-			    != 0) {
-				mgmt_fe_send_setcfg_reply(
-					session, setcfg_req->ds_id,
-					setcfg_req->req_id, false,
-					"Failed to lock the DS!",
-					setcfg_req->implicit_commit);
-				goto mgmt_fe_sess_handle_setcfg_req_failed;
-			}
-
-			session->ds_locked_implict[setcfg_req->ds_id] = true;
-		}
-
-		/*
-		 * Start a CONFIG Transaction (if not started already)
-		 */
 		session->cfg_txn_id = mgmt_create_txn(session->session_id,
 						      MGMTD_TXN_TYPE_CONFIG);
 		if (session->cfg_txn_id == MGMTD_SESSION_ID_NONE) {
@@ -850,14 +743,15 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session,
 				false,
 				"Failed to create a Configuration session!",
 				setcfg_req->implicit_commit);
-			goto mgmt_fe_sess_handle_setcfg_req_failed;
+			return 0;
 		}
+		txn_created = true;
 
 		MGMTD_FE_ADAPTER_DBG("Created new Config txn-id: %" PRIu64
 				     " for session-id %" PRIu64,
 				     session->cfg_txn_id, session->session_id);
 	} else {
-		MGMTD_FE_ADAPTER_ERR("Config txn-id: %" PRIu64
+		MGMTD_FE_ADAPTER_DBG("Config txn-id: %" PRIu64
 				     " for session-id: %" PRIu64
 				     " already created",
 				     session->cfg_txn_id, session->session_id);
@@ -876,22 +770,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session,
 		}
 	}
 
-	dst_ds_ctx = 0;
+	/* Create the SETConfig request under the transaction. */
-	if (setcfg_req->implicit_commit) {
-		dst_ds_ctx =
-			mgmt_ds_get_ctx_by_id(mm, setcfg_req->commit_ds_id);
-		if (!dst_ds_ctx) {
-			mgmt_fe_send_setcfg_reply(
-				session, setcfg_req->ds_id, setcfg_req->req_id,
-				false, "No such commit DS exists!",
-				setcfg_req->implicit_commit);
-			return 0;
-		}
-	}
-
-	/*
-	 * Create the SETConfig request under the transaction.
-	 */
 	if (mgmt_txn_send_set_config_req(
 		    session->cfg_txn_id, setcfg_req->req_id, setcfg_req->ds_id,
 		    ds_ctx, setcfg_req->data, setcfg_req->n_data,
@@ -902,21 +781,11 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session,
 			session, setcfg_req->ds_id, setcfg_req->req_id, false,
 			"Request processing for SET-CONFIG failed!",
 			setcfg_req->implicit_commit);
-		goto mgmt_fe_sess_handle_setcfg_req_failed;
-	}
 
-	return 0;
+		/* delete transaction if we just created it */
-
+		if (txn_created)
-mgmt_fe_sess_handle_setcfg_req_failed:
-
-	/*
-	 * Delete transaction created recently.
-	 */
-	if (session->cfg_txn_id != MGMTD_TXN_ID_NONE)
 			mgmt_destroy_txn(&session->cfg_txn_id);
-	if (ds_ctx && session->ds_write_locked[setcfg_req->ds_id])
+	}
-		mgmt_fe_session_unlock_ds(setcfg_req->ds_id, ds_ctx, session,
-					      true, false);
 
 	return 0;
 }
@@ -926,6 +795,7 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session,
 					  Mgmtd__FeGetConfigReq *getcfg_req)
 {
 	struct mgmt_ds_ctx *ds_ctx;
+	struct nb_config *cfg_root = NULL;
 
 	/*
 	 * Get the DS handle.
@@ -938,11 +808,7 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session,
 		return 0;
 	}
 
-	/*
+	/* GETCFG must be on candidate or running DS */
-	 * Next check first if the GETCFG_REQ is for Candidate DS
-	 * or not. Report failure if its not. MGMTD currently only
-	 * supports editing the Candidate DS.
-	 */
 	if (getcfg_req->ds_id != MGMTD_DS_CANDIDATE
 	    && getcfg_req->ds_id != MGMTD_DS_RUNNING) {
 		mgmt_fe_send_getcfg_reply(
@@ -953,27 +819,6 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session,
 	}
 
 	if (session->txn_id == MGMTD_TXN_ID_NONE) {
-		/*
-		 * Try taking read-lock on the requested DS (if not already
-		 * locked). If the DS has already been write-locked by a ongoing
-		 * CONFIG transaction we may allow reading the contents of the
-		 * same DS.
-		 */
-		if (!session->ds_read_locked[getcfg_req->ds_id]
-		    && !session->ds_write_locked[getcfg_req->ds_id]) {
-			if (mgmt_fe_session_read_lock_ds(getcfg_req->ds_id,
-							     ds_ctx, session)
-			    != 0) {
-				mgmt_fe_send_getcfg_reply(
-					session, getcfg_req->ds_id,
-					getcfg_req->req_id, false, NULL,
-					"Failed to lock the DS! Another session might have locked it!");
-				goto mgmt_fe_sess_handle_getcfg_req_failed;
-			}
-
-			session->ds_locked_implict[getcfg_req->ds_id] = true;
-		}
-
 		/*
 		 * Start a SHOW Transaction (if not started already)
 		 */
@@ -997,13 +842,17 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session,
 				     session->txn_id, session->session_id);
 	}
 
+	/*
+	 * Get a copy of the datastore config root, avoids locking.
+	 */
+	cfg_root = nb_config_dup(mgmt_ds_get_nb_config(ds_ctx));
+
 	/*
 	 * Create a GETConfig request under the transaction.
 	 */
-	if (mgmt_txn_send_get_config_req(session->txn_id, getcfg_req->req_id,
+	if (mgmt_txn_send_get_config_req(
-					  getcfg_req->ds_id, ds_ctx,
+		    session->txn_id, getcfg_req->req_id, getcfg_req->ds_id,
-					  getcfg_req->data, getcfg_req->n_data)
+		    cfg_root, getcfg_req->data, getcfg_req->n_data) != 0) {
-	    != 0) {
 		mgmt_fe_send_getcfg_reply(
 			session, getcfg_req->ds_id, getcfg_req->req_id, false,
 			NULL, "Request processing for GET-CONFIG failed!");
@@ -1014,14 +863,13 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session,
 
 mgmt_fe_sess_handle_getcfg_req_failed:
 
+	if (cfg_root)
+		nb_config_free(cfg_root);
 	/*
 	 * Destroy the transaction created recently.
 	 */
 	if (session->txn_id != MGMTD_TXN_ID_NONE)
 		mgmt_destroy_txn(&session->txn_id);
-	if (ds_ctx && session->ds_read_locked[getcfg_req->ds_id])
-		mgmt_fe_session_unlock_ds(getcfg_req->ds_id, ds_ctx, session,
-					      false, true);
 
 	return -1;
 }
@@ -1043,28 +891,16 @@ mgmt_fe_session_handle_getdata_req_msg(struct mgmt_fe_session_ctx *session,
 		return 0;
 	}
 
-	if (session->txn_id == MGMTD_TXN_ID_NONE) {
+	/* GETDATA must be on operational DS */
-		/*
+	if (getdata_req->ds_id != MGMTD_DS_OPERATIONAL) {
-		 * Try taking read-lock on the requested DS (if not already
-		 * locked). If the DS has already been write-locked by a ongoing
-		 * CONFIG transaction we may allow reading the contents of the
-		 * same DS.
-		 */
-		if (!session->ds_read_locked[getdata_req->ds_id]
-		    && !session->ds_write_locked[getdata_req->ds_id]) {
-			if (mgmt_fe_session_read_lock_ds(getdata_req->ds_id,
-							     ds_ctx, session)
-			    != 0) {
 		mgmt_fe_send_getdata_reply(
-					session, getdata_req->ds_id,
+			session, getdata_req->ds_id, getdata_req->req_id, false,
-					getdata_req->req_id, false, NULL,
+			NULL,
-					"Failed to lock the DS! Another session might have locked it!");
+			"Get-Data on datastore other than Operational DS not permitted!");
-				goto mgmt_fe_sess_handle_getdata_req_failed;
+		return 0;
-			}
-
-			session->ds_locked_implict[getdata_req->ds_id] = true;
 	}
 
+	if (session->txn_id == MGMTD_TXN_ID_NONE) {
 		/*
 		 * Start a SHOW Transaction (if not started already)
 		 */
@@ -1091,9 +927,8 @@ mgmt_fe_session_handle_getdata_req_msg(struct mgmt_fe_session_ctx *session,
 	 * Create a GETData request under the transaction.
 	 */
 	if (mgmt_txn_send_get_data_req(session->txn_id, getdata_req->req_id,
-					getdata_req->ds_id, ds_ctx,
+				       getdata_req->ds_id, getdata_req->data,
-					getdata_req->data, getdata_req->n_data)
+				       getdata_req->n_data) != 0) {
-	    != 0) {
 		mgmt_fe_send_getdata_reply(
 			session, getdata_req->ds_id, getdata_req->req_id, false,
 			NULL, "Request processing for GET-CONFIG failed!");
@@ -1110,10 +945,6 @@ mgmt_fe_sess_handle_getdata_req_failed:
 	if (session->txn_id != MGMTD_TXN_ID_NONE)
 		mgmt_destroy_txn(&session->txn_id);
 
-	if (ds_ctx && session->ds_read_locked[getdata_req->ds_id])
-		mgmt_fe_session_unlock_ds(getdata_req->ds_id, ds_ctx,
-					      session, false, true);
-
 	return -1;
 }
 
@@ -1126,43 +957,30 @@ static int mgmt_fe_session_handle_commit_config_req_msg(
 	if (mm->perf_stats_en)
 		gettimeofday(&session->adapter->cmt_stats.last_start, NULL);
 	session->adapter->cmt_stats.commit_cnt++;
-	/*
+
-	 * Get the source DS handle.
+	/* Validate source and dest DS */
-	 */
+	if (commcfg_req->src_ds_id != MGMTD_DS_CANDIDATE ||
+	    commcfg_req->dst_ds_id != MGMTD_DS_RUNNING) {
+		mgmt_fe_send_commitcfg_reply(
+			session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id,
+			commcfg_req->req_id, MGMTD_INTERNAL_ERROR,
+			commcfg_req->validate_only,
+			"Source/Dest for commit must be candidate/running DS");
+		return 0;
+	}
 	src_ds_ctx = mgmt_ds_get_ctx_by_id(mm, commcfg_req->src_ds_id);
-	if (!src_ds_ctx) {
+	assert(src_ds_ctx);
-		mgmt_fe_send_commitcfg_reply(
-			session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id,
-			commcfg_req->req_id, MGMTD_INTERNAL_ERROR,
-			commcfg_req->validate_only,
-			"No such source DS exists!");
-		return 0;
-	}
-
-	/*
-	 * Get the destination DS handle.
-	 */
 	dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, commcfg_req->dst_ds_id);
-	if (!dst_ds_ctx) {
+	assert(dst_ds_ctx);
-		mgmt_fe_send_commitcfg_reply(
-			session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id,
-			commcfg_req->req_id, MGMTD_INTERNAL_ERROR,
-			commcfg_req->validate_only,
-			"No such destination DS exists!");
-		return 0;
-	}
 
-	/*
+	/* User should have lock on both source and dest DS */
-	 * Next check first if the COMMCFG_REQ is for running DS
+	if (!session->ds_locked[commcfg_req->dst_ds_id] ||
-	 * or not. Report failure if its not. MGMTD currently only
+	    !session->ds_locked[commcfg_req->src_ds_id]) {
-	 * supports editing the Candidate DS.
-	 */
-	if (commcfg_req->dst_ds_id != MGMTD_DS_RUNNING) {
 		mgmt_fe_send_commitcfg_reply(
 			session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id,
-			commcfg_req->req_id, MGMTD_INTERNAL_ERROR,
+			commcfg_req->req_id, MGMTD_DS_LOCK_FAILED,
 			commcfg_req->validate_only,
-			"Set-Config on datastores other than Running DS not permitted!");
+			"Commit requires lock on candidate and/or running DS");
 		return 0;
 	}
 
@@ -1187,26 +1005,6 @@ static int mgmt_fe_session_handle_commit_config_req_msg(
 				     session->cfg_txn_id, session->session_id);
 	}
 
-
-	/*
-	 * Try taking write-lock on the destination DS (if not already).
-	 */
-	if (!session->ds_write_locked[commcfg_req->dst_ds_id]) {
-		if (mgmt_fe_session_write_lock_ds(commcfg_req->dst_ds_id,
-						      dst_ds_ctx, session)
-		    != 0) {
-			mgmt_fe_send_commitcfg_reply(
-				session, commcfg_req->src_ds_id,
-				commcfg_req->dst_ds_id, commcfg_req->req_id,
-				MGMTD_DS_LOCK_FAILED,
-				commcfg_req->validate_only,
-				"Failed to lock the destination DS!");
-			return 0;
-		}
-
-		session->ds_locked_implict[commcfg_req->dst_ds_id] = true;
-	}
-
 	/*
 	 * Create COMMITConfig request under the transaction
 	 */
@@ -1214,8 +1012,7 @@ static int mgmt_fe_session_handle_commit_config_req_msg(
 		    session->cfg_txn_id, commcfg_req->req_id,
 		    commcfg_req->src_ds_id, src_ds_ctx, commcfg_req->dst_ds_id,
 		    dst_ds_ctx, commcfg_req->validate_only, commcfg_req->abort,
-		    false)
+		    false) != 0) {
-	    != 0) {
 		mgmt_fe_send_commitcfg_reply(
 			session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id,
 			commcfg_req->req_id, MGMTD_INTERNAL_ERROR,
@@ -1743,18 +1540,10 @@ void mgmt_fe_adapter_status_write(struct vty *vty, bool detail)
 				session->session_id);
 			vty_out(vty, "        DS-Locks:\n");
 			FOREACH_MGMTD_DS_ID (ds_id) {
-				if (session->ds_write_locked[ds_id]
+				if (session->ds_locked[ds_id]) {
-				    || session->ds_read_locked[ds_id]) {
 					locked = true;
-					vty_out(vty,
+					vty_out(vty, "          %s\n",
-						"          %s\t\t\t%s, %s\n",
+						mgmt_ds_id2name(ds_id));
-						mgmt_ds_id2name(ds_id),
-						session->ds_write_locked[ds_id]
-							? "Write"
-							: "Read",
-						session->ds_locked_implict[ds_id]
-							? "Implicit"
-							: "Explicit");
 				}
 			}
 			if (!locked)

mgmtd/mgmt_history.c

@@ -196,23 +196,21 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty,
 	}
 
 	src_ds_ctx = mgmt_ds_get_ctx_by_id(mm, MGMTD_DS_CANDIDATE);
-	if (!src_ds_ctx) {
-		vty_out(vty, "ERROR: Couldnot access Candidate datastore!\n");
-		return -1;
-	}
-
-	/*
-	 * Note: Write lock on src_ds is not required. This is already
-	 * taken in 'conf te'.
-	 */
 	dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, MGMTD_DS_RUNNING);
-	if (!dst_ds_ctx) {
+	assert(src_ds_ctx);
-		vty_out(vty, "ERROR: Couldnot access Running datastore!\n");
+	assert(dst_ds_ctx);
+
+	ret = mgmt_ds_lock(src_ds_ctx, vty->mgmt_session_id);
+	if (ret != 0) {
+		vty_out(vty,
+			"Failed to lock the DS %u for rollback Reason: %s!\n",
+			MGMTD_DS_RUNNING, strerror(ret));
 		return -1;
 	}
 
-	ret = mgmt_ds_write_lock(dst_ds_ctx);
+	ret = mgmt_ds_lock(dst_ds_ctx, vty->mgmt_session_id);
 	if (ret != 0) {
+		mgmt_ds_unlock(src_ds_ctx);
 		vty_out(vty,
 			"Failed to lock the DS %u for rollback Reason: %s!\n",
 			MGMTD_DS_RUNNING, strerror(ret));
@@ -223,26 +221,29 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty,
 		ret = mgmt_ds_load_config_from_file(
 			src_ds_ctx, cmt_info->cmt_json_file, false);
 		if (ret != 0) {
-			mgmt_ds_unlock(dst_ds_ctx);
 			vty_out(vty,
 				"Error with parsing the file with error code %d\n",
 				ret);
-			return ret;
+			goto failed_unlock;
 		}
 	}
 
 	/* Internally trigger a commit-request. */
 	ret = mgmt_txn_rollback_trigger_cfg_apply(src_ds_ctx, dst_ds_ctx);
 	if (ret != 0) {
-		mgmt_ds_unlock(dst_ds_ctx);
 		vty_out(vty,
 			"Error with creating commit apply txn with error code %d\n",
 			ret);
-		return ret;
+		goto failed_unlock;
 	}
 
 	mgmt_history_dump_cmt_record_index();
 
+	/*
+	 * TODO: Cleanup: the generic TXN code currently checks for rollback
+	 * and does the unlock when it completes.
+	 */
+
 	/*
 	 * Block the rollback command from returning till the rollback
 	 * is completed. On rollback completion mgmt_history_rollback_complete()
@@ -251,6 +252,11 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty,
 	vty->mgmt_req_pending_cmd = "ROLLBACK";
 	rollback_vty = vty;
 	return 0;
+
+failed_unlock:
+	mgmt_ds_unlock(src_ds_ctx);
+	mgmt_ds_unlock(dst_ds_ctx);
+	return ret;
 }
 
 void mgmt_history_rollback_complete(bool success)

mgmtd/mgmt_txn.c

@@ -164,7 +164,7 @@ struct mgmt_get_data_reply {
 
 struct mgmt_get_data_req {
 	Mgmtd__DatastoreId ds_id;
-	struct mgmt_ds_ctx *ds_ctx;
+	struct nb_config *cfg_root;
 	char *xpaths[MGMTD_MAX_NUM_DATA_REQ_IN_BATCH];
 	int num_xpaths;
 
@@ -576,6 +576,10 @@ static void mgmt_txn_req_free(struct mgmt_txn_req **txn_req)
 		if ((*txn_req)->req.get_data->reply)
 			XFREE(MTYPE_MGMTD_TXN_GETDATA_REPLY,
 			      (*txn_req)->req.get_data->reply);
+
+		if ((*txn_req)->req.get_data->cfg_root)
+			nb_config_free((*txn_req)->req.get_data->cfg_root);
+
 		XFREE(MTYPE_MGMTD_TXN_GETDATA_REQ, (*txn_req)->req.get_data);
 		break;
 	case MGMTD_TXN_PROC_GETDATA:
@@ -683,18 +687,18 @@ static void mgmt_txn_process_set_cfg(struct event *thread)
 			assert(mgmt_txn_reqs_count(&txn->set_cfg_reqs) == 1);
 			assert(txn_req->req.set_cfg->dst_ds_ctx);
 
-			ret = mgmt_ds_write_lock(
+			/* We expect the user to have locked the DST DS */
-				txn_req->req.set_cfg->dst_ds_ctx);
+			if (!mgmt_ds_is_locked(txn_req->req.set_cfg->dst_ds_ctx,
-			if (ret != 0) {
+					       txn->session_id)) {
 				MGMTD_TXN_ERR(
-					"Failed to lock DS %u txn-id: %" PRIu64
+					"DS %u not locked for implicit commit txn-id: %" PRIu64
 					" session-id: %" PRIu64 " err: %s",
 					txn_req->req.set_cfg->dst_ds_id,
 					txn->txn_id, txn->session_id,
 					strerror(ret));
 				mgmt_txn_send_commit_cfg_reply(
 					txn, MGMTD_DS_LOCK_FAILED,
-					"Lock running DS before implicit commit failed!");
+					"running DS not locked for implicit commit");
 				goto mgmt_txn_process_set_cfg_done;
 			}
 
@@ -703,8 +707,8 @@ static void mgmt_txn_process_set_cfg(struct event *thread)
 				txn_req->req.set_cfg->ds_id,
 				txn_req->req.set_cfg->ds_ctx,
 				txn_req->req.set_cfg->dst_ds_id,
-				txn_req->req.set_cfg->dst_ds_ctx, false,
+				txn_req->req.set_cfg->dst_ds_ctx, false, false,
-				false, true);
+				true);
 
 			if (mm->perf_stats_en)
 				gettimeofday(&cmt_stats->last_start, NULL);
@@ -746,7 +750,6 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
 					   enum mgmt_result result,
 					   const char *error_if_any)
 {
-	int ret = 0;
 	bool success, create_cmt_info_rec;
 
 	if (!txn->commit_cfg_req)
@@ -754,7 +757,12 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
 
 	success = (result == MGMTD_SUCCESS || result == MGMTD_NO_CFG_CHANGES);
 
+	/* TODO: these replies should not be send if it's a rollback
+	 * b/c right now that is special cased.. that special casing should be
+	 * removed; however...
+	 */
 	if (!txn->commit_cfg_req->req.commit_cfg.implicit && txn->session_id
+	    && !txn->commit_cfg_req->req.commit_cfg.rollback
 	    && mgmt_fe_send_commit_cfg_reply(
 		       txn->session_id, txn->txn_id,
 		       txn->commit_cfg_req->req.commit_cfg.src_ds_id,
@@ -770,6 +778,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
 	}
 
 	if (txn->commit_cfg_req->req.commit_cfg.implicit && txn->session_id
+	    && !txn->commit_cfg_req->req.commit_cfg.rollback
 	    && mgmt_fe_send_set_cfg_reply(
 		       txn->session_id, txn->txn_id,
 		       txn->commit_cfg_req->req.commit_cfg.src_ds_id,
@@ -784,6 +793,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
 
 	if (success) {
 		/* Stop the commit-timeout timer */
+		/* XXX why only on success? */
 		EVENT_OFF(txn->comm_cfg_timeout);
 
 		create_cmt_info_rec =
@@ -830,27 +840,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
 	}
 
 	if (txn->commit_cfg_req->req.commit_cfg.rollback) {
-		ret = mgmt_ds_unlock(
+		mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx);
-			txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx);
+		mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx);
-		if (ret != 0)
-			MGMTD_TXN_ERR(
-				"Failed to unlock the dst DS during rollback : %s",
-				strerror(ret));
-
 		/*
 		 * Resume processing the rollback command.
+		 *
+		 * TODO: there's no good reason to special case rollback, the
+		 * rollback boolean should be passed back to the FE client and it
+		 * can do the right thing.
 		 */
 		mgmt_history_rollback_complete(success);
 	}
 
-	if (txn->commit_cfg_req->req.commit_cfg.implicit)
-		if (mgmt_ds_unlock(
-			    txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx)
-		    != 0)
-			MGMTD_TXN_ERR(
-				"Failed to unlock the dst DS during implicit : %s",
-				strerror(ret));
-
 	txn->commit_cfg_req->req.commit_cfg.cmt_stats = NULL;
 	mgmt_txn_req_free(&txn->commit_cfg_req);
 
@@ -1724,8 +1725,7 @@ static void mgmt_txn_send_getcfg_reply_data(struct mgmt_txn_req *txn_req,
 	mgmt_reset_get_data_reply_buf(get_req);
 }
 
-static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx,
+static void mgmt_txn_iter_and_send_get_cfg_reply(const char *xpath,
-						 const char *xpath,
 						 struct lyd_node *node,
 						 struct nb_node *nb_node,
 						 void *ctx)
@@ -1770,7 +1770,7 @@ static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx,
 
 static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn,
 			       struct mgmt_txn_req *txn_req,
-			       struct mgmt_ds_ctx *ds_ctx)
+			       struct nb_config *root)
 {
 	int indx;
 	struct mgmt_get_data_req *get_data;
@@ -1805,7 +1805,8 @@ static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn,
 		 * want to also use an xpath regexp we need to add this
 		 * functionality.
 		 */
-		if (mgmt_ds_iter_data(get_data->ds_ctx, get_data->xpaths[indx],
+		if (mgmt_ds_iter_data(get_data->ds_id, root,
+				      get_data->xpaths[indx],
 				      mgmt_txn_iter_and_send_get_cfg_reply,
 				      (void *)txn_req) == -1) {
 			MGMTD_TXN_DBG("Invalid Xpath '%s",
@@ -1837,7 +1838,7 @@ static void mgmt_txn_process_get_cfg(struct event *thread)
 {
 	struct mgmt_txn_ctx *txn;
 	struct mgmt_txn_req *txn_req;
-	struct mgmt_ds_ctx *ds_ctx;
+	struct nb_config *cfg_root;
 	int num_processed = 0;
 	bool error;
 
@@ -1852,18 +1853,10 @@ static void mgmt_txn_process_get_cfg(struct event *thread)
 	FOREACH_TXN_REQ_IN_LIST (&txn->get_cfg_reqs, txn_req) {
 		error = false;
 		assert(txn_req->req_event == MGMTD_TXN_PROC_GETCFG);
-		ds_ctx = txn_req->req.get_data->ds_ctx;
+		cfg_root = txn_req->req.get_data->cfg_root;
-		if (!ds_ctx) {
+		assert(cfg_root);
-			mgmt_fe_send_get_cfg_reply(
-				txn->session_id, txn->txn_id,
-				txn_req->req.get_data->ds_id, txn_req->req_id,
-				MGMTD_INTERNAL_ERROR, NULL,
-				"No such datastore!");
-			error = true;
-			goto mgmt_txn_process_get_cfg_done;
-		}
 
-		if (mgmt_txn_get_config(txn, txn_req, ds_ctx) != 0) {
+		if (mgmt_txn_get_config(txn, txn_req, cfg_root) != 0) {
 			MGMTD_TXN_ERR(
 				"Unable to retrieve config from DS %d txn-id: %" PRIu64
 				" session-id: %" PRIu64 " req-id: %" PRIu64,
@@ -1872,8 +1865,6 @@ static void mgmt_txn_process_get_cfg(struct event *thread)
 			error = true;
 		}
 
-	mgmt_txn_process_get_cfg_done:
-
 		if (error) {
 			/*
 			 * Delete the txn request.
@@ -1904,9 +1895,7 @@ static void mgmt_txn_process_get_data(struct event *thread)
 {
 	struct mgmt_txn_ctx *txn;
 	struct mgmt_txn_req *txn_req;
-	struct mgmt_ds_ctx *ds_ctx;
 	int num_processed = 0;
-	bool error;
 
 	txn = (struct mgmt_txn_ctx *)EVENT_ARG(thread);
 	assert(txn);
@@ -1917,32 +1906,8 @@ static void mgmt_txn_process_get_data(struct event *thread)
 		      txn->session_id);
 
 	FOREACH_TXN_REQ_IN_LIST (&txn->get_data_reqs, txn_req) {
-		error = false;
 		assert(txn_req->req_event == MGMTD_TXN_PROC_GETDATA);
-		ds_ctx = txn_req->req.get_data->ds_ctx;
-		if (!ds_ctx) {
-			mgmt_fe_send_get_data_reply(
-				txn->session_id, txn->txn_id,
-				txn_req->req.get_data->ds_id, txn_req->req_id,
-				MGMTD_INTERNAL_ERROR, NULL,
-				"No such datastore!");
-			error = true;
-			goto mgmt_txn_process_get_data_done;
-		}
 
-		if (mgmt_ds_is_config(ds_ctx)) {
-			if (mgmt_txn_get_config(txn, txn_req, ds_ctx)
-			    != 0) {
-				MGMTD_TXN_ERR(
-					"Unable to retrieve config from DS %d txn-id %" PRIu64
-					" session-id: %" PRIu64
-					" req-id: %" PRIu64,
-					txn_req->req.get_data->ds_id,
-					txn->txn_id, txn->session_id,
-					txn_req->req_id);
-				error = true;
-			}
-		} else {
 		/*
 		 * TODO: Trigger GET procedures for Backend
 		 * For now return back error.
@@ -1952,19 +1917,12 @@ static void mgmt_txn_process_get_data(struct event *thread)
 			txn_req->req.get_data->ds_id, txn_req->req_id,
 			MGMTD_INTERNAL_ERROR, NULL,
 			"GET-DATA on Oper DS is not supported yet!");
-			error = true;
-		}
-
-	mgmt_txn_process_get_data_done:
-
-		if (error) {
 		/*
 		 * Delete the txn request.
 		 * Note: The following will remove it from the list
 		 * as well.
 		 */
 		mgmt_txn_req_free(&txn_req);
-		}
 
 		/*
 		 * Else the transaction would have been already deleted or
@@ -2395,9 +2353,8 @@ int mgmt_txn_notify_be_adapter_conn(struct mgmt_be_client_adapter *adapter,
 	memset(&dummy_stats, 0, sizeof(dummy_stats));
 	if (connect) {
 		/* Get config for this single backend client */
-		mgmt_be_get_adapter_config(adapter, mm->running_ds,
-					      &adapter_cfgs);
 
+		mgmt_be_get_adapter_config(adapter, &adapter_cfgs);
 		if (!adapter_cfgs || RB_EMPTY(nb_config_cbs, adapter_cfgs)) {
 			SET_FLAG(adapter->flags,
 				 MGMTD_BE_ADAPTER_FLAGS_CFG_SYNCED);
@@ -2620,7 +2577,7 @@ int mgmt_txn_notify_be_cfg_apply_reply(uint64_t txn_id, bool success,
 
 int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id,
 				 Mgmtd__DatastoreId ds_id,
-				  struct mgmt_ds_ctx *ds_ctx,
+				 struct nb_config *cfg_root,
 				 Mgmtd__YangGetDataReq **data_req,
 				 size_t num_reqs)
 {
@@ -2634,7 +2591,7 @@ int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id,
 
 	txn_req = mgmt_txn_req_alloc(txn, req_id, MGMTD_TXN_PROC_GETCFG);
 	txn_req->req.get_data->ds_id = ds_id;
-	txn_req->req.get_data->ds_ctx = ds_ctx;
+	txn_req->req.get_data->cfg_root = cfg_root;
 	for (indx = 0;
 	     indx < num_reqs && indx < MGMTD_MAX_NUM_DATA_REPLY_IN_BATCH;
 	     indx++) {
@@ -2651,7 +2608,6 @@ int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id,
 
 int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id,
 			       Mgmtd__DatastoreId ds_id,
-				struct mgmt_ds_ctx *ds_ctx,
 			       Mgmtd__YangGetDataReq **data_req,
 			       size_t num_reqs)
 {
@@ -2665,7 +2621,7 @@ int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id,
 
 	txn_req = mgmt_txn_req_alloc(txn, req_id, MGMTD_TXN_PROC_GETDATA);
 	txn_req->req.get_data->ds_id = ds_id;
-	txn_req->req.get_data->ds_ctx = ds_ctx;
+	txn_req->req.get_data->cfg_root = NULL;
 	for (indx = 0;
 	     indx < num_reqs && indx < MGMTD_MAX_NUM_DATA_REPLY_IN_BATCH;
 	     indx++) {
@@ -2703,10 +2659,11 @@ int mgmt_txn_rollback_trigger_cfg_apply(struct mgmt_ds_ctx *src_ds_ctx,
 					struct mgmt_ds_ctx *dst_ds_ctx)
 {
 	static struct nb_config_cbs changes;
+	static struct mgmt_commit_stats dummy_stats;
+
 	struct nb_config_cbs *cfg_chgs = NULL;
 	struct mgmt_txn_ctx *txn;
 	struct mgmt_txn_req *txn_req;
-	static struct mgmt_commit_stats dummy_stats;
 
 	memset(&changes, 0, sizeof(changes));
 	memset(&dummy_stats, 0, sizeof(dummy_stats));

mgmtd/mgmt_txn.h

@@ -183,7 +183,7 @@ extern int mgmt_txn_send_commit_config_req(uint64_t txn_id, uint64_t req_id,
  */
 extern int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id,
 					Mgmtd__DatastoreId ds_id,
-					 struct mgmt_ds_ctx *ds_ctx,
+					struct nb_config *cfg_root,
 					Mgmtd__YangGetDataReq **data_req,
 					size_t num_reqs);
 
@@ -194,7 +194,6 @@ extern int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id,
  */
 extern int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id,
 				       Mgmtd__DatastoreId ds_id,
-				       struct mgmt_ds_ctx *ds_ctx,
 				       Mgmtd__YangGetDataReq **data_req,
 				       size_t num_reqs);
 

tests/lib/test_grpc.cpp

@@ -114,7 +114,7 @@ static void static_startup(void)
 	// Add a route
 	vty = vty_new();
 	vty->type = vty::VTY_TERM;
-	vty_config_enter(vty, true, false);
+	vty_config_enter(vty, true, false, false);
 
 	auto ret = cmd_execute(vty, "ip route 11.0.0.0/8 Null0", NULL, 0);
 	assert(!ret);

vtysh/vtysh.c

@@ -2333,8 +2333,9 @@ DEFUNSH(VTYSH_REALLYALL, vtysh_disable, vtysh_disable_cmd, "disable",
 }
 
 DEFUNSH(VTYSH_REALLYALL, vtysh_config_terminal, vtysh_config_terminal_cmd,
-	"configure [terminal]",
+	"configure [terminal [file-lock]]",
 	"Configuration from vty interface\n"
+	"Configuration with locked datastores\n"
 	"Configuration terminal\n")
 {
 	vty->node = CONFIG_NODE;
@@ -2355,7 +2356,7 @@ static int vtysh_exit(struct vty *vty)
 	if (vty->node == CONFIG_NODE) {
 		/* resync in case one of the daemons is somewhere else */
 		vtysh_execute("end");
-		vtysh_execute("configure");
+		vtysh_execute("configure terminal file-lock");
 	}
 	return CMD_SUCCESS;
 }

vtysh/vtysh_config.c

@@ -607,7 +607,7 @@ static int vtysh_read_file(FILE *confp, bool dry_run)
 	vty->node = CONFIG_NODE;
 
 	vtysh_execute_no_pager("enable");
-	vtysh_execute_no_pager("configure terminal");
+	vtysh_execute_no_pager("configure terminal file-lock");
 
 	if (!dry_run)
 		vtysh_execute_no_pager("XFRR_start_configuration");