mgmtd: KISS the locking code

Move away from things like "lock if not locked" type code, require the
user has locked prior to geting to that point.

For now we warn if we are taking a lock we already had; however, this
should really be a failure point.

New requirements:

SETCFG -
  not implicit commit - requires user has locked candidate DS and they
    must unlock after

  implicit commit - requires user has locked candidate and running DS
    both locks will be unlocked on reply to the SETCFG

COMMITCFG -
  requires user has locked candidate and running DS and they must unlock
  after

  rollback - this code now get both locks and then does an unlock and
  early return thing on the adapter side. It needs to be un-special
  cased in follow up work that would also include tests for this
  functionality.

Signed-off-by: Christian Hopps <chopps@labn.net>

7 files changed, 124 insertions, 98 deletions

diff --git a/lib/command.c b/lib/command.c
index 0995637219..8025ab534f 100644
--- a/lib/command.c
+++ b/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 */
diff --git a/lib/mgmt.proto b/lib/mgmt.proto
index 8a11ff0fa5..ac44eefd9e 100644
--- a/lib/mgmt.proto
+++ b/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 {
diff --git a/lib/mgmt_fe_client.c b/lib/mgmt_fe_client.c
index 22d085cbce..45d57175d6 100644
--- a/lib/mgmt_fe_client.c
+++ b/lib/mgmt_fe_client.c
@@ -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:
diff --git a/lib/mgmt_fe_client.h b/lib/mgmt_fe_client.h
index e3c016f9b6..532fee4397 100644
--- a/lib/mgmt_fe_client.h
+++ b/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,
diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c
index 9d6ec66689..8003679ed5 100644
--- a/lib/northbound_cli.c
+++ b/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,
diff --git a/lib/vty.c b/lib/vty.c
index c1f4d7f47d..fc6bed6a0a 100644
--- a/lib/vty.c
+++ b/lib/vty.c
@@ -1676,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;
@@ -2233,6 +2233,9 @@ bool mgmt_vty_read_configs(void)
 
 	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,
 			 mgmt_daemons[index]);
@@ -2276,6 +2279,14 @@ bool mgmt_vty_read_configs(void)
 		fclose(confp);
 	}
 
+	/* 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->pending_allowed = false;
 
 	if (!count)
@@ -2844,9 +2855,11 @@ 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;
 	}
@@ -2857,20 +2870,20 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive)
 	 * message. For user interactive mode we are doing implicit commits
 	 * those will obtain the lock (or not) when they try and commit.
 	 */
-	if (vty_mgmt_fe_enabled() && vty->pending_allowed && !private_config) {
-		/*
-		 * lock using short-circuit, we set the locked boolean to true
-		 * here so that it can be flipped to false by our locked_notify
-		 * handler during the synchronous call.
-		 */
-		vty->mgmt_locked_candidate_ds = true;
-		if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, true,
-					     true) ||
-		    !vty->mgmt_locked_candidate_ds) {
+	if (file_lock && vty_mgmt_fe_enabled() && !private_config) {
+		if (vty_mgmt_lock_candidate_inline(vty)) {
 			vty_out(vty,
 				"%% Can't enter config; candidate datastore locked by another session\n");
 			return CMD_WARNING_CONFIG_FAILED;
 		}
+		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;
@@ -2923,18 +2936,15 @@ void vty_config_exit(struct vty *vty)
 
 int vty_config_node_exit(struct vty *vty)
 {
-	int ret;
-
 	vty->xpath_index = 0;
 
-	if (vty->mgmt_locked_candidate_ds) {
-		assert(vty->type != VTY_FILE);
-		/* use short-circuit call to immediately unlock */
-		ret = vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false,
-					       true);
-		assert(!ret);
-		vty->mgmt_locked_candidate_ds = false;
-	}
+	/* TODO: could we check for un-commited changes here? */
+
+	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);
@@ -3537,7 +3547,8 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client,
 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;
 
@@ -3555,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);
 }
 
@@ -3688,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) {
 		/*
@@ -3701,80 +3717,87 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 		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;
 }
 
diff --git a/lib/vty.h b/lib/vty.h
index 7e27b52fe1..8fb1483e5b 100644
--- a/lib/vty.h
+++ b/lib/vty.h
@@ -391,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 *);