1 files changed, 49 insertions, 14 deletions

diff --git a/lib/vty.c b/lib/vty.c
index b701f3bc0f..fd00e11c5f 100644
--- a/lib/vty.c
+++ b/lib/vty.c
@@ -134,18 +134,22 @@ void vty_mgmt_resume_response(struct vty *vty, bool success)
 	uint8_t header[4] = {0, 0, 0, 0};
 	int ret = CMD_SUCCESS;
 
-	if (!vty->mgmt_req_pending) {
+	if (!vty->mgmt_req_pending_cmd) {
 		zlog_err(
-			"vty response called without setting mgmt_req_pending");
+			"vty resume response called without mgmt_req_pending_cmd");
 		return;
 	}
 
 	if (!success)
 		ret = CMD_WARNING_CONFIG_FAILED;
 
-	vty->mgmt_req_pending = false;
+	MGMTD_FE_CLIENT_DBG(
+		"resuming CLI cmd after %s on vty session-id: %" PRIu64
+		" with '%s'",
+		vty->mgmt_req_pending_cmd, vty->mgmt_session_id,
+		success ? "succeeded" : "failed");
 
-	MGMTD_FE_CLIENT_DBG("resuming: %s:", success ? "succeeded" : "failed");
+	vty->mgmt_req_pending_cmd = NULL;
 
 	if (vty->type != VTY_FILE) {
 		header[3] = ret;
@@ -2217,6 +2221,8 @@ bool mgmt_vty_read_configs(void)
 		line_num = 0;
 		(void)config_from_file(vty, confp, &line_num);
 		count++;
+
+		fclose(confp);
 	}
 
 	snprintf(path, sizeof(path), "%s/mgmtd.conf", frr_sysconfdir);
@@ -2240,6 +2246,8 @@ bool mgmt_vty_read_configs(void)
 		line_num = 0;
 		(void)config_from_file(vty, confp, &line_num);
 		count++;
+
+		fclose(confp);
 	}
 
 	vty->pending_allowed = false;
@@ -2270,6 +2278,19 @@ static void vtysh_read(struct event *thread)
 	sock = EVENT_FD(thread);
 	vty = EVENT_ARG(thread);
 
+	/*
+	 * This code looks like it can read multiple commands from the `buf`
+	 * value returned by read(); however, it cannot in some cases.
+	 *
+	 * There are multiple paths out of the "copying to vty->buf" loop, which
+	 * lose any content not yet copied from the stack `buf`, `passfd`,
+	 * `CMD_SUSPEND` and finally if a front-end for mgmtd (generally this
+	 * would be mgmtd itself). So these code paths are counting on vtysh not
+	 * sending us more than 1 command line before waiting on the reply to
+	 * that command.
+	 */
+	assert(vty->type == VTY_SHELL_SERV);
+
 	if ((nbytes = read(sock, buf, VTY_READ_BUFSIZ)) <= 0) {
 		if (nbytes < 0) {
 			if (ERRNO_IO_RETRY(errno)) {
@@ -2344,8 +2365,13 @@ static void vtysh_read(struct event *thread)
 				/* with new infra we need to stop response till
 				 * we get response through callback.
 				 */
-				if (vty->mgmt_req_pending)
+				if (vty->mgmt_req_pending_cmd) {
+					MGMTD_FE_CLIENT_DBG(
+						"postpone CLI cmd response pending mgmtd %s on vty session-id %" PRIu64,
+						vty->mgmt_req_pending_cmd,
+						vty->mgmt_session_id);
 					return;
+				}
 
 				/* warning: watchfrr hardcodes this result write
 				 */
@@ -2419,7 +2445,16 @@ void vty_close(struct vty *vty)
 
 	vty->status = VTY_CLOSE;
 
+	/*
+	 * If we reach here with pending config to commit we will be losing it
+	 * so warn the user.
+	 */
+	if (vty->mgmt_num_pending_setcfg)
+		MGMTD_FE_CLIENT_ERR(
+			"vty closed, uncommitted config will be lost.");
+
 	if (mgmt_fe_client && vty->mgmt_session_id) {
+		MGMTD_FE_CLIENT_DBG("closing vty session");
 		mgmt_fe_destroy_client_session(mgmt_fe_client,
 					       vty->mgmt_client_id);
 		vty->mgmt_session_id = 0;
@@ -3440,7 +3475,9 @@ static void vty_mgmt_session_notify(struct mgmt_fe_client *client,
 		vty->mgmt_session_id = session_id;
 	} else {
 		vty->mgmt_session_id = 0;
-		vty_close(vty);
+		/* We may come here by way of vty_close() and short-circuits */
+		if (vty->status != VTY_CLOSE)
+			vty_close(vty);
 	}
 }
 
@@ -3609,13 +3646,13 @@ int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
 			return -1;
 		}
 
-		vty->mgmt_req_pending = true;
+		vty->mgmt_req_pending_cmd = "MESSAGE_LOCKDS_REQ";
 	}
 
 	return 0;
 }
 
-int vty_mgmt_send_config_data(struct vty *vty)
+int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 {
 	Mgmtd__YangDataValue value[VTY_MAXCFGCHANGES];
 	Mgmtd__YangData cfg_data[VTY_MAXCFGCHANGES];
@@ -3623,7 +3660,6 @@ int vty_mgmt_send_config_data(struct vty *vty)
 	Mgmtd__YangCfgDataReq *cfgreq[VTY_MAXCFGCHANGES] = {0};
 	size_t indx;
 	int cnt;
-	bool implicit_commit = false;
 
 	if (vty->type == VTY_FILE) {
 		/*
@@ -3697,7 +3733,6 @@ int vty_mgmt_send_config_data(struct vty *vty)
 		}
 
 		vty->mgmt_req_id++;
-		implicit_commit = vty_needs_implicit_commit(vty);
 		if (cnt && mgmt_fe_send_setcfg_req(
 				   mgmt_fe_client, vty->mgmt_session_id,
 				   vty->mgmt_req_id, MGMTD_DS_CANDIDATE, cfgreq,
@@ -3709,7 +3744,7 @@ int vty_mgmt_send_config_data(struct vty *vty)
 			return -1;
 		}
 
-		vty->mgmt_req_pending = true;
+		vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ";
 	}
 
 	return 0;
@@ -3729,7 +3764,7 @@ int vty_mgmt_send_commit_config(struct vty *vty, bool validate_only, bool abort)
 			return -1;
 		}
 
-		vty->mgmt_req_pending = true;
+		vty->mgmt_req_pending_cmd = "MESSAGE_COMMCFG_REQ";
 		vty->mgmt_num_pending_setcfg = 0;
 	}
 
@@ -3766,7 +3801,7 @@ int vty_mgmt_send_get_config(struct vty *vty, Mgmtd__DatastoreId datastore,
 		return -1;
 	}
 
-	vty->mgmt_req_pending = true;
+	vty->mgmt_req_pending_cmd = "MESSAGE_GETCFG_REQ";
 
 	return 0;
 }
@@ -3800,7 +3835,7 @@ int vty_mgmt_send_get_data(struct vty *vty, Mgmtd__DatastoreId datastore,
 		return -1;
 	}
 
-	vty->mgmt_req_pending = true;
+	vty->mgmt_req_pending_cmd = "MESSAGE_GETDATA_REQ";
 
 	return 0;
 }