Merge pull request FRRouting#14019 from LabNConsulting/chopps/fix-mgm…

…td-assert

fix double lock bug and cmd resume early bugs

lib/mgmt_fe_client.c

@@ -629,6 +629,11 @@ uint mgmt_fe_client_session_count(struct mgmt_fe_client *client)
 	return mgmt_sessions_count(&client->sessions);
 }
 
+bool mgmt_fe_client_current_msg_short_circuit(struct mgmt_fe_client *client)
+{
+	return client->client.conn.is_short_circuit;
+}
+
 /*
  * Create a new Session for a Frontend Client connection.
  */

lib/mgmt_fe_client.h

@@ -373,6 +373,12 @@ extern void mgmt_fe_client_destroy(struct mgmt_fe_client *client);
  */
 extern uint mgmt_fe_client_session_count(struct mgmt_fe_client *client);
 
+/*
+ * True if the current handled message is being short-circuited
+ */
+extern bool
+mgmt_fe_client_current_msg_short_circuit(struct mgmt_fe_client *client);
+
 #ifdef __cplusplus
 }
 #endif

lib/vty.c

@@ -3524,6 +3524,7 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client,
 				      char *errmsg_if_any)
 {
 	struct vty *vty;
+	bool is_short_circuit = mgmt_fe_client_current_msg_short_circuit(client);
 
 	vty = (struct vty *)session_ctx;
 
@@ -3540,8 +3541,10 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client,
 			vty->mgmt_locked_running_ds = lock_ds;
 	}
 
-	if (vty->mgmt_req_pending_cmd)
+	if (!is_short_circuit && vty->mgmt_req_pending_cmd) {
+		assert(!strcmp(vty->mgmt_req_pending_cmd, "MESSAGE_LOCKDS_REQ"));
 		vty_mgmt_resume_response(vty, success);
+	}
 }
 
 static void vty_mgmt_set_config_result_notified(
@@ -3734,6 +3737,7 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 		} else if (vty_mgmt_lock_running_inline(vty)) {
 			vty_out(vty,
 				"%% command failed, could not lock running DS\n");
+			vty_mgmt_unlock_candidate_inline(vty);
 			return -1;
 		}
 	}

lib/vty.h

@@ -231,6 +231,10 @@ struct vty {
 	const char *mgmt_req_pending_cmd;
 	bool mgmt_locked_candidate_ds;
 	bool mgmt_locked_running_ds;
+	/* Need to track when we file-lock in vtysh to re-lock on end/conf t
+	 * workaround
+	 */
+	bool vtysh_file_locked;
 };
 
 static inline void vty_push_context(struct vty *vty, int node, uint64_t id)

mgmtd/mgmt_fe_adapter.c

@@ -593,9 +593,8 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session,
 	}
 
 	if (lockds_req->lock) {
-		if (mgmt_fe_session_write_lock_ds(lockds_req->ds_id,
-						      ds_ctx, session)
-		    != 0) {
+		if (mgmt_fe_session_write_lock_ds(lockds_req->ds_id, ds_ctx,
+						  session)) {
 			fe_adapter_send_lockds_reply(
 				session, lockds_req->ds_id, lockds_req->req_id,
 				lockds_req->lock, false,

tests/topotests/mgmt_config/r1/frr.conf

@@ -0,0 +1,15 @@
+debug northbound notifications
+! debug northbound libyang
+debug northbound events
+debug northbound callbacks
+debug mgmt backend datastore frontend transaction
+debug mgmt client backend
+debug mgmt client frontend
+
+log timestamp precision 6
+log file frr.log debug
+
+interface r1-eth0
+  ip address 101.0.0.1/24
+  ipv6 address 2101::1/64
+exit

tests/topotests/mgmt_config/test_regression.py

@@ -0,0 +1,53 @@
+# -*- coding: utf-8 eval: (blacken-mode 1) -*-
+# SPDX-License-Identifier: ISC
+#
+# July 13 2023, Christian Hopps <chopps@labn.net>
+#
+# Copyright (c) 2023, LabN Consulting, L.L.C.
+#
+"""
+Test mgmtd regressions
+
+"""
+import pytest
+from lib.topogen import Topogen
+
+pytestmark = [pytest.mark.staticd]
+
+
+@pytest.fixture(scope="module")
+def tgen(request):
+    "Setup/Teardown the environment and provide tgen argument to tests"
+
+    topodef = {"s1": ("r1",)}
+    tgen = Topogen(topodef, request.module.__name__)
+    tgen.start_topology()
+    tgen.gears["r1"].load_frr_config("frr.conf")
+    tgen.start_router()
+    yield tgen
+    tgen.stop_topology()
+
+
+def test_regression_issue_13920(tgen):
+    """Issue #13920
+
+    ubuntu2204# conf t
+    ubuntu2204(config)# ip route 3.2.4.0/24 6.5.5.11 loop3
+    ubuntu2204(config)# nexthop-group nh2
+    ubuntu2204(config-nh-group)# nexthop 6.5.5.12
+    ubuntu2204(config-nh-group)# exi
+    ubuntu2204(config)# ip route 3.22.4.0/24 6.5.5.12
+    crash
+    """
+
+    r1 = tgen.gears["r1"]
+    r1.vtysh_multicmd(
+        """
+    conf t
+    nexthop-group nh2
+    exit
+    ip route 3.22.4.0/24 6.5.5.12
+    """
+    )
+    output = r1.net.checkRouterCores()
+    assert not output.strip()

vtysh/vtysh.c

@@ -2356,8 +2356,14 @@ 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 terminal file-lock");
+		/* NOTE: a rather expensive thing to do, can we avoid it? */
+
+		if (vty->vtysh_file_locked)
+			vtysh_execute("configure terminal file-lock");
+		else
+			vtysh_execute("configure terminal");
 	}
+
 	return CMD_SUCCESS;
 }
 

vtysh/vtysh_config.c

@@ -607,7 +607,8 @@ 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 file-lock");
+	vtysh_execute_no_pager("conf term file-lock");
+	vty->vtysh_file_locked = true;
 
 	if (!dry_run)
 		vtysh_execute_no_pager("XFRR_start_configuration");
@@ -619,6 +620,7 @@ static int vtysh_read_file(FILE *confp, bool dry_run)
 		vtysh_execute_no_pager("XFRR_end_configuration");
 
 	vtysh_execute_no_pager("end");
+	vty->vtysh_file_locked = false;
 	vtysh_execute_no_pager("disable");
 
 	vty_close(vty);