1 files changed, 192 insertions, 141 deletions

diff --git a/lib/vty.c b/lib/vty.c
index fd00e11c5f..fc6bed6a0a 100644
--- a/lib/vty.c
+++ b/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(
-			    mgmt_fe_client, new->mgmt_client_id,
-			    (uintptr_t) new) != MGMTD_SUCCESS)
-			zlog_err(
-				"Failed to open a MGMTD Frontend session for VTY session %p!!",
-				new);
+		new->mgmt_session_id = 0;
+		mgmt_fe_create_client_session(
+			mgmt_fe_client, new->mgmt_client_id, (uintptr_t) new);
+		/* we short-circuit create the session so it must be set now */
+		assertf(new->mgmt_session_id != 0,
+			"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_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;
-	mgmt_candidate_ds_wr_locked = false;
+	vty->pending_allowed = 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) {
-			if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE,
-						     true) != 0) {
-				vty_out(vty, "Not able to lock candidate DS\n");
-				return CMD_WARNING;
-			}
-		} else {
+	/*
+	 * We only need to do a lock when reading a config file as we will be
+	 * sending a batch of setcfg changes followed by a single commit
+	 * message. For user interactive mode we are doing implicit commits
+	 * those will obtain the lock (or not) when they try and commit.
+	 */
+	if (file_lock && vty_mgmt_fe_enabled() && !private_config) {
+		if (vty_mgmt_lock_candidate_inline(vty)) {
 			vty_out(vty,
-				"Candidate DS already locked by different session\n");
-			return CMD_WARNING;
+				"%% Can't enter config; candidate datastore locked by another session\n");
+			return CMD_WARNING_CONFIG_FAILED;
 		}
-
-		vty->mgmt_locked_candidate_ds = true;
-		mgmt_candidate_ds_wr_locked = true;
+		if (vty_mgmt_lock_running_inline(vty)) {
+			vty_out(vty,
+				"%% 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,23 +2896,24 @@ 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 {
-		/*
-		 * NOTE: On the MGMTD daemon we point the VTY candidate DS to
-		 * the global MGMTD candidate DS. Else we point to the VTY
-		 * Shared Candidate Config.
-		 */
-		vty->candidate_config = vty_mgmt_candidate_config
-						? vty_mgmt_candidate_config
-						: vty_shared_candidate_config;
-		if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL)
-			vty->candidate_config_base =
-				nb_config_dup(running_config);
+		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
+	 * Shared Candidate Config.
+	 */
+	vty->candidate_config = vty_mgmt_candidate_config
+					? vty_mgmt_candidate_config
+					: vty_shared_candidate_config;
+	if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL)
+		vty->candidate_config_base = 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;
 
-	/*
-	 * 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;
-		}
+	/* TODO: could we check for un-commited changes here? */
 
-		vty->mgmt_locked_candidate_ds = false;
-		mgmt_candidate_ds_wr_locked = false;
-	}
+	if (vty->mgmt_locked_running_ds)
+		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) {
-		zlog_err("%socking for DS %u failed, Err: '%s'",
-			 lock_ds ? "L" : "Unl", ds_id, errmsg_if_any);
-		vty_out(vty, "ERROR: %socking for DS %u failed, Err: '%s'\n",
-			lock_ds ? "L" : "Unl", ds_id, errmsg_if_any);
-	} else {
+	assert(ds_id == MGMTD_DS_CANDIDATE || ds_id == MGMTD_DS_RUNNING);
+	if (!success)
+		zlog_err("%socking for DS %u failed, Err: '%s' vty %p",
+			 lock_ds ? "L" : "Unl", ds_id, errmsg_if_any, vty);
+	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;
 	}
 
-	vty_mgmt_resume_response(vty, success);
+	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,23 +3677,24 @@ 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) {
-		vty->mgmt_req_id++;
-		if (mgmt_fe_send_lockds_req(mgmt_fe_client,
-					    vty->mgmt_session_id,
-					    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",
-				lock ? "" : "UN");
-			return -1;
-		}
+	assert(mgmt_fe_client);
+	assert(vty->mgmt_session_id);
 
-		vty->mgmt_req_pending_cmd = "MESSAGE_LOCKDS_REQ";
+	vty->mgmt_req_id++;
+	if (mgmt_fe_send_lockds_req(mgmt_fe_client, vty->mgmt_session_id,
+				    vty->mgmt_req_id, ds_id, lock, scok)) {
+		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",
+			lock ? "" : "UN");
+		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,86 +3712,92 @@ 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) {
-		/*
-		 * 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");
+	if (!vty->num_cfg_changes)
 		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]);
-
-			if (vty->cfg_changes[indx].value) {
-				mgmt_yang_data_value_init(&value[cnt]);
-				value[cnt].encoded_str_val =
-					(char *)vty->cfg_changes[indx].value;
-				value[cnt].value_case =
-					MGMTD__YANG_DATA_VALUE__VALUE_ENCODED_STR_VAL;
-				cfg_data[cnt].value = &value[cnt];
-			}
+	for (indx = 0; indx < vty->num_cfg_changes; indx++) {
+		mgmt_yang_data_init(&cfg_data[indx]);
 
-			cfg_data[cnt].xpath = vty->cfg_changes[indx].xpath;
+		if (vty->cfg_changes[indx].value) {
+			mgmt_yang_data_value_init(&value[indx]);
+			value[indx].encoded_str_val =
+				(char *)vty->cfg_changes[indx].value;
+			value[indx].value_case =
+				MGMTD__YANG_DATA_VALUE__VALUE_ENCODED_STR_VAL;
+			cfg_data[indx].value = &value[indx];
+		}
 
-			mgmt_yang_cfg_data_req_init(&cfg_req[cnt]);
-			cfg_req[cnt].data = &cfg_data[cnt];
-			switch (vty->cfg_changes[indx].operation) {
-			case NB_OP_DESTROY:
-				cfg_req[cnt].req_type =
-					MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA;
-				break;
+		cfg_data[indx].xpath = vty->cfg_changes[indx].xpath;
 
-			case NB_OP_CREATE:
-			case NB_OP_MODIFY:
-			case NB_OP_MOVE:
-			case NB_OP_PRE_VALIDATE:
-			case NB_OP_APPLY_FINISH:
-				cfg_req[cnt].req_type =
-					MGMTD__CFG_DATA_REQ_TYPE__SET_DATA;
-				break;
-			case NB_OP_GET_ELEM:
-			case NB_OP_GET_NEXT:
-			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");
-			}
+		mgmt_yang_cfg_data_req_init(&cfg_req[indx]);
+		cfg_req[indx].data = &cfg_data[indx];
+		switch (vty->cfg_changes[indx].operation) {
+		case NB_OP_DESTROY:
+			cfg_req[indx].req_type =
+				MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA;
+			break;
 
-			cfgreq[cnt] = &cfg_req[cnt];
-			cnt++;
+		case NB_OP_CREATE:
+		case NB_OP_MODIFY:
+		case NB_OP_MOVE:
+		case NB_OP_PRE_VALIDATE:
+		case NB_OP_APPLY_FINISH:
+			cfg_req[indx].req_type =
+				MGMTD__CFG_DATA_REQ_TYPE__SET_DATA;
+			break;
+		case NB_OP_GET_ELEM:
+		case NB_OP_GET_NEXT:
+		case NB_OP_GET_KEYS:
+		case NB_OP_LOOKUP_ENTRY:
+		case NB_OP_RPC:
+		default:
+			assertf(false,
+				"Invalid operation type for send config: %d",
+				vty->cfg_changes[indx].operation);
+			/*NOTREACHED*/
+			abort();
 		}
 
-		vty->mgmt_req_id++;
-		if (cnt && mgmt_fe_send_setcfg_req(
-				   mgmt_fe_client, vty->mgmt_session_id,
-				   vty->mgmt_req_id, MGMTD_DS_CANDIDATE, cfgreq,
-				   cnt, implicit_commit,
-				   MGMTD_DS_RUNNING) != MGMTD_SUCCESS) {
-			zlog_err("Failed to send %d Config Xpaths to MGMTD!!",
-				 (int)indx);
-			vty_out(vty, "Failed to send SETCFG-REQ to MGMTD!\n");
-			return -1;
-		}
+		cfgreq[indx] = &cfg_req[indx];
+	}
+	if (!indx)
+		return 0;
 
-		vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ";
+	vty->mgmt_req_id++;
+	if (mgmt_fe_send_setcfg_req(mgmt_fe_client, vty->mgmt_session_id,
+				    vty->mgmt_req_id, MGMTD_DS_CANDIDATE,
+				    cfgreq, indx, implicit_commit,
+				    MGMTD_DS_RUNNING)) {
+		zlog_err("Failed to send %zu config xpaths to mgmtd", indx);
+		vty_out(vty, "%% Failed to send commands to mgmtd\n");
+		return -1;
 	}
 
+	vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ";
+
 	return 0;
 }