lib: mgmtd: fixes for startup config file processing

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

lib/mgmt_fe_client.c

@@ -361,6 +361,7 @@ static int mgmt_fe_client_handle_msg(struct mgmt_fe_client_ctx *client_ctx,
 				client_ctx, fe_msg->session_req->session_id);
 		}
 
+		/* The session state may be deleted by the callback */
 		if (session && session->client_ctx &&
 		    session->client_ctx->client_params.client_session_notify)
 			(*session->client_ctx->client_params
@@ -549,6 +550,7 @@ static int _notify_connect_disconnect(struct msg_client *client, bool connected)
 {
 	struct mgmt_fe_client_ctx *client_ctx =
 		container_of(client, struct mgmt_fe_client_ctx, client);
+	struct mgmt_fe_client_session *session;
 	int ret;
 
 	/* Send REGISTER_REQ message */
@@ -557,6 +559,32 @@ static int _notify_connect_disconnect(struct msg_client *client, bool connected)
 			return ret;
 	}
 
+	/* Walk list of sessions for this FE client deleting them */
+	if (!connected && mgmt_sessions_count(&client_ctx->client_sessions)) {
+		MGMTD_FE_CLIENT_DBG("Cleaning up existing sessions");
+
+		FOREACH_SESSION_IN_LIST (client_ctx, session) {
+			assert(session->client_ctx);
+
+			/* unlink from list first this avoids double free */
+			mgmt_sessions_del(&client_ctx->client_sessions,
+					  session);
+
+			/* notify FE client the session is being deleted */
+			if (session->client_ctx->client_params
+				    .client_session_notify) {
+				(*session->client_ctx->client_params
+					  .client_session_notify)(
+					(uintptr_t)client_ctx,
+					client_ctx->client_params.user_data,
+					session->client_id, false, true,
+					session->session_id, session->user_ctx);
+			}
+
+			XFREE(MTYPE_MGMTD_FE_SESSION, session);
+		}
+	}
+
 	/* Notify FE client through registered callback (if any). */
 	if (client_ctx->client_params.client_connect_notify)
 		(void)(*client_ctx->client_params.client_connect_notify)(
@@ -653,6 +681,14 @@ void mgmt_fe_client_lib_vty_init(void)
 	install_element(CONFIG_NODE, &debug_mgmt_client_fe_cmd);
 }
 
+uint mgmt_fe_client_session_count(uintptr_t lib_hndl)
+{
+	struct mgmt_fe_client_ctx *client_ctx =
+		(struct mgmt_fe_client_ctx *)lib_hndl;
+
+	return mgmt_sessions_count(&client_ctx->client_sessions);
+}
+
 /*
  * Create a new Session for a Frontend Client connection.
  */
@@ -705,8 +741,8 @@ enum mgmt_result mgmt_fe_destroy_client_session(uintptr_t lib_hndl,
 	if (session->session_id &&
 	    mgmt_fe_send_session_req(client_ctx, session, false) != 0)
 		MGMTD_FE_CLIENT_ERR(
-			"Failed to send session destroy request for the session-id %lu",
+			"Failed to send session destroy request for the session-id %" PRIu64,
-			(unsigned long)session->session_id);
+			session->session_id);
 
 	mgmt_sessions_del(&client_ctx->client_sessions, session);
 	XFREE(MTYPE_MGMTD_FE_SESSION, session);

lib/mgmt_fe_client.h

@@ -365,6 +365,11 @@ mgmt_fe_register_yang_notify(uintptr_t lib_hndl, uint64_t session_id,
  */
 extern void mgmt_fe_client_lib_destroy(void);
 
+/*
+ * Get count of open sessions.
+ */
+extern uint mgmt_fe_client_session_count(uintptr_t lib_hndl);
+
 #ifdef __cplusplus
 }
 #endif

lib/northbound_cli.c

@@ -195,9 +195,12 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 		va_end(ap);
 	}
 
-	if (vty_mgmt_fe_enabled()) {
+	if (vty_mgmt_should_process_cli_apply_changes(vty)) {
 		VTY_CHECK_XPATH;
 
+		if (vty->type == VTY_FILE)
+			return CMD_SUCCESS;
+
 		implicit_commit = vty_needs_implicit_commit(vty);
 		ret = vty_mgmt_send_config_data(vty);
 		if (ret >= 0 && !implicit_commit)
@@ -224,7 +227,7 @@ int nb_cli_apply_changes_clear_pending(struct vty *vty,
 		va_end(ap);
 	}
 
-	if (vty_mgmt_fe_enabled()) {
+	if (vty_mgmt_should_process_cli_apply_changes(vty)) {
 		VTY_CHECK_XPATH;
 
 		implicit_commit = vty_needs_implicit_commit(vty);

lib/vty.c

@@ -74,13 +74,6 @@ static bool mgmt_candidate_ds_wr_locked;
 static uint64_t mgmt_client_id_next;
 static uint64_t mgmt_last_req_id = UINT64_MAX;
 
-static bool vty_debug;
-#define VTY_DBG(fmt, ...)                                                      \
-	do {                                                                   \
-		if (vty_debug)                                                 \
-			zlog_debug(fmt, ##__VA_ARGS__);                        \
-	} while (0)
-
 PREDECL_DLIST(vtyservs);
 
 struct vty_serv {
@@ -2203,6 +2196,7 @@ bool mgmt_vty_read_configs(void)
 	vty->type = VTY_FILE;
 	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;
@@ -2215,7 +2209,7 @@ bool mgmt_vty_read_configs(void)
 		if (!confp)
 			continue;
 
-		MGMTD_FE_CLIENT_DBG("mgmtd: reading config file %s", path);
+		zlog_info("mgmtd: reading config file: %s", path);
 
 		/* Execute configuration file */
 		line_num = 0;
@@ -2223,16 +2217,11 @@ bool mgmt_vty_read_configs(void)
 		count++;
 	}
 
-	MGMTD_FE_CLIENT_DBG(
-		"mgmtd: done with daemon configs, checking mgmtd config");
-
 	snprintf(path, sizeof(path), "%s/mgmtd.conf", frr_sysconfdir);
 	confp = vty_open_config(path, config_default);
 	if (!confp) {
 		char *orig;
 
-		MGMTD_FE_CLIENT_DBG("mgmtd: no mgmtd config file %s", path);
-
 		snprintf(path, sizeof(path), "%s/zebra.conf", frr_sysconfdir);
 		orig = XSTRDUP(MTYPE_TMP, host_config_get());
 
@@ -2244,22 +2233,25 @@ bool mgmt_vty_read_configs(void)
 	}
 
 	if (confp) {
+		zlog_info("mgmtd: reading config file: %s", path);
+
 		line_num = 0;
 		(void)config_from_file(vty, confp, &line_num);
 		count++;
 	}
 
-	if (!count) {
+	vty->pending_allowed = false;
-		MGMTD_FE_CLIENT_DBG("mgmtd: read no config files");
-		vty_close(vty);
-	} else {
-		MGMTD_FE_CLIENT_DBG("mgmtd: done reading all config files");
-		vty_read_file_finish(vty, vty->candidate_config);
-	}
 
 	vty->mgmt_locked_candidate_ds = false;
 	mgmt_candidate_ds_wr_locked = false;
 
+	if (!count)
+		vty_close(vty);
+	else
+		vty_read_file_finish(vty, NULL);
+
+	zlog_info("mgmtd: finished reading config files");
+
 	return true;
 }
 
@@ -2411,7 +2403,7 @@ void vty_close(struct vty *vty)
 
 	vty->status = VTY_CLOSE;
 
-	if (mgmt_lib_hndl) {
+	if (mgmt_lib_hndl && vty->mgmt_session_id) {
 		mgmt_fe_destroy_client_session(mgmt_lib_hndl,
 					       vty->mgmt_client_id);
 		vty->mgmt_session_id = 0;
@@ -3377,22 +3369,43 @@ void vty_init_vtysh(void)
 	/* currently nothing to do, but likely to have future use */
 }
 
+
+/*
+ * These functions allow for CLI handling to be placed inside daemons; however,
+ * currently they are only used by mgmtd, with mgmtd having each daemons CLI
+ * functionality linked into it. This design choice was taken for efficiency.
+ */
+
 static void vty_mgmt_server_connected(uintptr_t lib_hndl, uintptr_t usr_data,
 				      bool connected)
 {
-	VTY_DBG("%sGot %sconnected %s MGMTD Frontend Server",
+	MGMTD_FE_CLIENT_DBG("Got %sconnected %s MGMTD Frontend Server",
-		!connected ? "ERROR: " : "", !connected ? "dis: " : "",
+			    !connected ? "dis: " : "",
 			    !connected ? "from" : "to");
 
+	/*
+	 * We should not have any sessions for connecting or disconnecting case.
+	 * The  fe client library will delete all session on disconnect before
+	 * calling us.
+	 */
+	assert(mgmt_fe_client_session_count(lib_hndl) == 0);
+
 	mgmt_fe_connected = connected;
 
-	/*
+	/* Start or stop listening for vty connections */
-	 * TODO: Setup or teardown front-end sessions for existing
+	if (connected) {
-	 * VTY connections.
+		zlog_info("mgmtd: starting vty servers");
-	 */
+		frr_vty_serv_start();
+	} else {
+		zlog_info("mgmtd: stopping vty servers");
+		frr_vty_serv_stop();
+	}
 }
 
-static void vty_mgmt_session_created(uintptr_t lib_hndl, uintptr_t usr_data,
+/*
+ * A session has successfully been created for a vty.
+ */
+static void vty_mgmt_session_notify(uintptr_t lib_hndl, uintptr_t usr_data,
 				    uint64_t client_id, bool create,
 				    bool success, uintptr_t session_id,
 				    uintptr_t session_ctx)
@@ -3407,10 +3420,16 @@ static void vty_mgmt_session_created(uintptr_t lib_hndl, uintptr_t usr_data,
 		return;
 	}
 
-	VTY_DBG("%s session for client %" PRIu64 " successfully",
+	MGMTD_FE_CLIENT_DBG("%s session for client %" PRIu64 " successfully",
 			    create ? "Created" : "Destroyed", client_id);
-	if (create)
+
+	if (create) {
+		assert(session_id != 0);
 		vty->mgmt_session_id = session_id;
+	} else {
+		vty->mgmt_session_id = 0;
+		vty_close(vty);
+	}
 }
 
 static void vty_mgmt_ds_lock_notified(uintptr_t lib_hndl, uintptr_t usr_data,
@@ -3430,8 +3449,8 @@ static void vty_mgmt_ds_lock_notified(uintptr_t lib_hndl, uintptr_t usr_data,
 		vty_out(vty, "ERROR: %socking for DS %u failed, Err: '%s'\n",
 			lock_ds ? "L" : "Unl", ds_id, errmsg_if_any);
 	} else {
-		VTY_DBG("%socked DS %u successfully", lock_ds ? "L" : "Unl",
+		MGMTD_FE_CLIENT_DBG("%socked DS %u successfully",
-			ds_id);
+				    lock_ds ? "L" : "Unl", ds_id);
 	}
 
 	vty_mgmt_resume_response(vty, success);
@@ -3453,7 +3472,7 @@ static void vty_mgmt_set_config_result_notified(
 		vty_out(vty, "ERROR: SET_CONFIG request failed, Error: %s\n",
 			errmsg_if_any ? errmsg_if_any : "Unknown");
 	} else {
-		VTY_DBG("SET_CONFIG request for client 0x%" PRIx64
+		MGMTD_FE_CLIENT_DBG("SET_CONFIG request for client 0x%" PRIx64
 				    " req-id %" PRIu64 " was successfull",
 				    client_id, req_id);
 	}
@@ -3478,7 +3497,8 @@ static void vty_mgmt_commit_config_result_notified(
 		vty_out(vty, "ERROR: COMMIT_CONFIG request failed, Error: %s\n",
 			errmsg_if_any ? errmsg_if_any : "Unknown");
 	} else {
-		VTY_DBG("COMMIT_CONFIG request for client 0x%" PRIx64
+		MGMTD_FE_CLIENT_DBG(
+			"COMMIT_CONFIG request for client 0x%" PRIx64
 			" req-id %" PRIu64 " was successfull",
 			client_id, req_id);
 		if (errmsg_if_any)
@@ -3509,7 +3529,7 @@ static enum mgmt_result vty_mgmt_get_data_result_notified(
 		return MGMTD_INTERNAL_ERROR;
 	}
 
-	VTY_DBG("GET_DATA request succeeded, client 0x%" PRIx64
+	MGMTD_FE_CLIENT_DBG("GET_DATA request succeeded, client 0x%" PRIx64
 			    " req-id %" PRIu64,
 			    client_id, req_id);
 
@@ -3532,7 +3552,7 @@ static enum mgmt_result vty_mgmt_get_data_result_notified(
 
 static struct mgmt_fe_client_params client_params = {
 	.client_connect_notify = vty_mgmt_server_connected,
-	.client_session_notify = vty_mgmt_session_created,
+	.client_session_notify = vty_mgmt_session_notify,
 	.lock_ds_notify = vty_mgmt_ds_lock_notified,
 	.set_config_notify = vty_mgmt_set_config_result_notified,
 	.commit_config_notify = vty_mgmt_commit_config_result_notified,
@@ -3555,7 +3575,12 @@ void vty_init_mgmt_fe(void)
 
 bool vty_mgmt_fe_enabled(void)
 {
-	return mgmt_lib_hndl && mgmt_fe_connected ? true : false;
+	return mgmt_lib_hndl && mgmt_fe_connected;
+}
+
+bool vty_mgmt_should_process_cli_apply_changes(struct vty *vty)
+{
+	return vty->type != VTY_FILE && vty_mgmt_fe_enabled();
 }
 
 int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
@@ -3591,6 +3616,19 @@ int vty_mgmt_send_config_data(struct vty *vty)
 	int cnt;
 	bool implicit_commit = false;
 
+	if (vty->type == VTY_FILE) {
+		/*
+		 * if this is a config file read we will not send any of the
+		 * 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 (mgmt_lib_hndl && vty->mgmt_client_id && !vty->mgmt_session_id) {
 		/*
 		 * We are connected to mgmtd but we do not yet have an

lib/vty.h

@@ -226,6 +226,9 @@ struct vty {
 	uint64_t mgmt_session_id; /* FE adapter identifies session w/ this */
 	uint64_t mgmt_client_id;  /* FE vty client identifies w/ this ID */
 	uint64_t mgmt_req_id;
+	/* set when we have sent mgmtd a *REQ command in response to some vty
+	 * CLI command and we are waiting on the reply so we can respond to the
+	 * vty user. */
 	bool mgmt_req_pending;
 	bool mgmt_locked_candidate_ds;
 };
@@ -401,7 +404,7 @@ extern void vty_stdio_close(void);
 
 extern void vty_init_mgmt_fe(void);
 extern bool vty_mgmt_fe_enabled(void);
-extern bool vty_mgmt_should_process_cli_changes(struct vty *vty);
+extern bool vty_mgmt_should_process_cli_apply_changes(struct vty *vty);
 
 extern bool mgmt_vty_read_configs(void);
 extern int vty_mgmt_send_config_data(struct vty *vty);