diff --git a/lib/mgmt_fe_client.c b/lib/mgmt_fe_client.c index da19db463f..7e42e1c09e 100644 --- a/lib/mgmt_fe_client.c +++ b/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. */ diff --git a/lib/mgmt_fe_client.h b/lib/mgmt_fe_client.h index 286141da44..349b7e4cf4 100644 --- a/lib/mgmt_fe_client.h +++ b/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 diff --git a/lib/vty.c b/lib/vty.c index c9de00a271..23aa2d1f38 100644 --- a/lib/vty.c +++ b/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; } } diff --git a/lib/vty.h b/lib/vty.h index ac3d2e5019..a8654f8b69 100644 --- a/lib/vty.h +++ b/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) diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index c12d8646b1..98b0e8bfcd 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/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, diff --git a/tests/topotests/mgmt_config/r1/frr.conf b/tests/topotests/mgmt_config/r1/frr.conf new file mode 100644 index 0000000000..076715c068 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/tests/topotests/mgmt_config/test_regression.py b/tests/topotests/mgmt_config/test_regression.py new file mode 100644 index 0000000000..00c3e01724 --- /dev/null +++ b/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 +# +# 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() diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index ee52a98adb..8b223d1aa4 100644 --- a/vtysh/vtysh.c +++ b/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; } diff --git a/vtysh/vtysh_config.c b/vtysh/vtysh_config.c index a5f790bbc6..593de7ffbc 100644 --- a/vtysh/vtysh_config.c +++ b/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);