lib: mgmtd: use short-circuit for locking

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

diff --git a/lib/mgmt_fe_client.c b/lib/mgmt_fe_client.c
index be7263f21b88b23c18138cf862628f08e98b6531..22d085cbce9836b37b5dbc1d9ec780d4b331ce60 100644 (file)
--- a/lib/mgmt_fe_client.c
+++ b/lib/mgmt_fe_client.c
@@ -164,7 +164,7 @@ static int mgmt_fe_send_session_req(struct mgmt_fe_client *client,
 
 int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client, uint64_t session_id,
                            uint64_t req_id, Mgmtd__DatastoreId ds_id,
-                           bool lock)
+                           bool lock, bool scok)
 {
        (void)req_id;
        Mgmtd__FeMessage fe_msg;
@@ -185,7 +185,7 @@ int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client, uint64_t session_id,
                lock ? "" : "UN", dsid2name(ds_id), session_id);
 
 
-       return mgmt_fe_client_send_msg(client, &fe_msg, false);
+       return mgmt_fe_client_send_msg(client, &fe_msg, scok);
 }
 
 int mgmt_fe_send_setcfg_req(struct mgmt_fe_client *client, uint64_t session_id,

diff --git a/lib/mgmt_fe_client.h b/lib/mgmt_fe_client.h
index b0ac44bb3ebf2a6592e82d9605ffd07222a4a33a..e3c016f9b66de328edab90f0d5d2747ba1843b64 100644 (file)
--- a/lib/mgmt_fe_client.h
+++ b/lib/mgmt_fe_client.h
@@ -219,7 +219,8 @@ mgmt_fe_destroy_client_session(struct mgmt_fe_client *client,
  */
 extern int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client,
                                   uint64_t session_id, uint64_t req_id,
-                                  Mgmtd__DatastoreId ds_id, bool lock_ds);
+                                  Mgmtd__DatastoreId ds_id, bool lock_ds,
+                                  bool scok);
 
 /*
  * Send SET_CONFIG_REQ to MGMTD for one or more config data(s).

diff --git a/lib/vty.c b/lib/vty.c
index fd00e11c5fb80933505dc024f1fd3287b0988202..c1f4d7f47d44b3875f7412cf2aa8830020de3eac 100644 (file)
--- a/lib/vty.c
+++ b/lib/vty.c
@@ -70,7 +70,6 @@ struct nb_config *vty_mgmt_candidate_config;
 
 static struct mgmt_fe_client *mgmt_fe_client;
 static bool mgmt_fe_connected;
-static bool mgmt_candidate_ds_wr_locked;
 static uint64_t mgmt_client_id_next;
 static uint64_t mgmt_last_req_id = UINT64_MAX;
 
@@ -129,6 +128,35 @@ char const *const mgmt_daemons[] = {
 };
 uint mgmt_daemons_count = array_size(mgmt_daemons);
 
+
+static int vty_mgmt_lock_candidate_inline(struct vty *vty)
+{
+       assert(!vty->mgmt_locked_candidate_ds);
+       (void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, true, true);
+       return vty->mgmt_locked_candidate_ds ? 0 : -1;
+}
+
+static int vty_mgmt_unlock_candidate_inline(struct vty *vty)
+{
+       assert(vty->mgmt_locked_candidate_ds);
+       (void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false, true);
+       return vty->mgmt_locked_candidate_ds ? -1 : 0;
+}
+
+static int vty_mgmt_lock_running_inline(struct vty *vty)
+{
+       assert(!vty->mgmt_locked_running_ds);
+       (void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_RUNNING, true, true);
+       return vty->mgmt_locked_running_ds ? 0 : -1;
+}
+
+static int vty_mgmt_unlock_running_inline(struct vty *vty)
+{
+       assert(vty->mgmt_locked_running_ds);
+       (void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_RUNNING, false, true);
+       return vty->mgmt_locked_running_ds ? -1 : 0;
+}
+
 void vty_mgmt_resume_response(struct vty *vty, bool success)
 {
        uint8_t header[4] = {0, 0, 0, 0};
@@ -2202,10 +2230,8 @@ bool mgmt_vty_read_configs(void)
        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;
 
+       vty->candidate_config = vty_shared_candidate_config;
 
        for (index = 0; index < array_size(mgmt_daemons); index++) {
                snprintf(path, sizeof(path), "%s/%s.conf", frr_sysconfdir,
@@ -2252,9 +2278,6 @@ bool mgmt_vty_read_configs(void)
 
        vty->pending_allowed = false;
 
-       vty->mgmt_locked_candidate_ds = false;
-       mgmt_candidate_ds_wr_locked = false;
-
        if (!count)
                vty_close(vty);
        else
@@ -2367,7 +2390,7 @@ static void vtysh_read(struct event *thread)
                                 */
                                if (vty->mgmt_req_pending_cmd) {
                                        MGMTD_FE_CLIENT_DBG(
-                                               "postpone CLI cmd response pending mgmtd %s on vty session-id %" PRIu64,
+                                               "postpone CLI response pending mgmtd %s on vty session-id %" PRIu64,
                                                vty->mgmt_req_pending_cmd,
                                                vty->mgmt_session_id);
                                        return;
@@ -2453,6 +2476,9 @@ void vty_close(struct vty *vty)
                MGMTD_FE_CLIENT_ERR(
                        "vty closed, uncommitted config will be lost.");
 
+       /* Drop out of configure / transaction if needed. */
+       vty_config_exit(vty);
+
        if (mgmt_fe_client && vty->mgmt_session_id) {
                MGMTD_FE_CLIENT_DBG("closing vty session");
                mgmt_fe_destroy_client_session(mgmt_fe_client,
@@ -2460,9 +2486,6 @@ void vty_close(struct vty *vty)
                vty->mgmt_session_id = 0;
        }
 
-       /* Drop out of configure / transaction if needed. */
-       vty_config_exit(vty);
-
        /* Cancel threads.*/
        EVENT_OFF(vty->t_read);
        EVENT_OFF(vty->t_write);
@@ -2828,21 +2851,26 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive)
                return CMD_WARNING;
        }
 
-       if (vty_mgmt_fe_enabled()) {
-               if (!mgmt_candidate_ds_wr_locked) {
-                       if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE,
-                                                    true) != 0) {
-                               vty_out(vty, "Not able to lock candidate DS\n");
-                               return CMD_WARNING;
-                       }
-               } else {
+       /*
+        * We only need to do a lock when reading a config file as we will be
+        * sending a batch of setcfg changes followed by a single commit
+        * 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) {
                        vty_out(vty,
-                               "Candidate DS already locked by different session\n");
-                       return CMD_WARNING;
+                               "%% Can't enter config; candidate datastore locked by another session\n");
+                       return CMD_WARNING_CONFIG_FAILED;
                }
-
-               vty->mgmt_locked_candidate_ds = true;
-               mgmt_candidate_ds_wr_locked = true;
        }
 
        vty->node = CONFIG_NODE;
@@ -2855,23 +2883,24 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive)
                vty->candidate_config_base = nb_config_dup(running_config);
                vty_out(vty,
                        "Warning: uncommitted changes will be discarded on exit.\n\n");
-       } else {
-               /*
-                * NOTE: On the MGMTD daemon we point the VTY candidate DS to
-                * the global MGMTD candidate DS. Else we point to the VTY
-                * Shared Candidate Config.
-                */
-               vty->candidate_config = vty_mgmt_candidate_config
-                                               ? vty_mgmt_candidate_config
-                                               : vty_shared_candidate_config;
-               if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL)
-                       vty->candidate_config_base =
-                               nb_config_dup(running_config);
+               return CMD_SUCCESS;
        }
 
+       /*
+        * NOTE: On the MGMTD daemon we point the VTY candidate DS to
+        * the global MGMTD candidate DS. Else we point to the VTY
+        * Shared Candidate Config.
+        */
+       vty->candidate_config = vty_mgmt_candidate_config
+                                       ? vty_mgmt_candidate_config
+                                       : vty_shared_candidate_config;
+       if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL)
+               vty->candidate_config_base = nb_config_dup(running_config);
+
        return CMD_SUCCESS;
 }
 
+
 void vty_config_exit(struct vty *vty)
 {
        enum node_type node = vty->node;
@@ -2894,22 +2923,17 @@ void vty_config_exit(struct vty *vty)
 
 int vty_config_node_exit(struct vty *vty)
 {
-       vty->xpath_index = 0;
+       int ret;
 
-       /*
-        * If we are not reading config file and we are mgmtd FE and we are
-        * locked then unlock.
-        */
-       if (vty->type != VTY_FILE && vty_mgmt_fe_enabled() &&
-           mgmt_candidate_ds_wr_locked && vty->mgmt_locked_candidate_ds) {
-               if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false) !=
-                   0) {
-                       vty_out(vty, "Not able to unlock candidate DS\n");
-                       return CMD_WARNING;
-               }
+       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;
-               mgmt_candidate_ds_wr_locked = false;
        }
 
        /* Perform any pending commits. */
@@ -3493,17 +3517,21 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client,
 
        vty = (struct vty *)session_ctx;
 
-       if (!success) {
-               zlog_err("%socking for DS %u failed, Err: '%s'",
-                        lock_ds ? "L" : "Unl", ds_id, errmsg_if_any);
-               vty_out(vty, "ERROR: %socking for DS %u failed, Err: '%s'\n",
-                       lock_ds ? "L" : "Unl", ds_id, errmsg_if_any);
-       } else {
+       assert(ds_id == MGMTD_DS_CANDIDATE || ds_id == MGMTD_DS_RUNNING);
+       if (!success)
+               zlog_err("%socking for DS %u failed, Err: '%s' vty %p",
+                        lock_ds ? "L" : "Unl", ds_id, errmsg_if_any, vty);
+       else {
                MGMTD_FE_CLIENT_DBG("%socked DS %u successfully",
                                    lock_ds ? "L" : "Unl", ds_id);
+               if (ds_id == MGMTD_DS_CANDIDATE)
+                       vty->mgmt_locked_candidate_ds = lock_ds;
+               else
+                       vty->mgmt_locked_running_ds = lock_ds;
        }
 
-       vty_mgmt_resume_response(vty, success);
+       if (vty->mgmt_req_pending_cmd)
+               vty_mgmt_resume_response(vty, success);
 }
 
 static void vty_mgmt_set_config_result_notified(
@@ -3632,23 +3660,24 @@ bool vty_mgmt_should_process_cli_apply_changes(struct vty *vty)
 }
 
 int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
-                            bool lock)
+                            bool lock, bool scok)
 {
-       if (mgmt_fe_client && vty->mgmt_session_id) {
-               vty->mgmt_req_id++;
-               if (mgmt_fe_send_lockds_req(mgmt_fe_client,
-                                           vty->mgmt_session_id,
-                                           vty->mgmt_req_id, ds_id, lock)) {
-                       zlog_err("Failed sending %sLOCK-DS-REQ req-id %" PRIu64,
-                                lock ? "" : "UN", vty->mgmt_req_id);
-                       vty_out(vty, "Failed to send %sLOCK-DS-REQ to MGMTD!\n",
-                               lock ? "" : "UN");
-                       return -1;
-               }
+       assert(mgmt_fe_client);
+       assert(vty->mgmt_session_id);
 
-               vty->mgmt_req_pending_cmd = "MESSAGE_LOCKDS_REQ";
+       vty->mgmt_req_id++;
+       if (mgmt_fe_send_lockds_req(mgmt_fe_client, vty->mgmt_session_id,
+                                   vty->mgmt_req_id, ds_id, lock, scok)) {
+               zlog_err("Failed sending %sLOCK-DS-REQ req-id %" PRIu64,
+                        lock ? "" : "UN", vty->mgmt_req_id);
+               vty_out(vty, "Failed to send %sLOCK-DS-REQ to MGMTD!\n",
+                       lock ? "" : "UN");
+               return -1;
        }
 
+       if (!scok)
+               vty->mgmt_req_pending_cmd = "MESSAGE_LOCKDS_REQ";
+
        return 0;
 }
 
@@ -3667,7 +3696,6 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
                 * 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;

diff --git a/lib/vty.h b/lib/vty.h
index 3b651d20a2b8ba2e07d15929968f0c2f151b9605..7e27b52fe17de7ddde5ee2df5f5d5c96a0921ab4 100644 (file)
--- a/lib/vty.h
+++ b/lib/vty.h
@@ -230,6 +230,7 @@ struct vty {
         * vty user. */
        const char *mgmt_req_pending_cmd;
        bool mgmt_locked_candidate_ds;
+       bool mgmt_locked_running_ds;
 };
 
 static inline void vty_push_context(struct vty *vty, int node, uint64_t id)
@@ -416,7 +417,7 @@ extern int vty_mgmt_send_get_config(struct vty *vty,
 extern int vty_mgmt_send_get_data(struct vty *vty, Mgmtd__DatastoreId datastore,
                                  const char **xpath_list, int num_req);
 extern int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
-                                   bool lock);
+                                   bool lock, bool scok);
 extern void vty_mgmt_resume_response(struct vty *vty, bool success);
 
 static inline bool vty_needs_implicit_commit(struct vty *vty)

diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c
index e9cbd444e84006f29b2092e4af1c096568aa63d8..4b104abb9866087ed6d48aed24a7c654cb5ed3bc 100644 (file)
--- a/mgmtd/mgmt_fe_adapter.c
+++ b/mgmtd/mgmt_fe_adapter.c
@@ -389,6 +389,7 @@ static int mgmt_fe_send_lockds_reply(struct mgmt_fe_session_ctx *session,
 {
        Mgmtd__FeMessage fe_msg;
        Mgmtd__FeLockDsReply lockds_reply;
+       bool scok = session->adapter->conn->is_short_circuit;
 
        assert(session->adapter);
 
@@ -406,10 +407,10 @@ static int mgmt_fe_send_lockds_reply(struct mgmt_fe_session_ctx *session,
        fe_msg.lockds_reply = &lockds_reply;
 
        MGMTD_FE_ADAPTER_DBG(
-               "Sending LOCK_DS_REPLY message to MGMTD Frontend client '%s'",
-               session->adapter->name);
+               "Sending LOCK_DS_REPLY message to MGMTD Frontend client '%s' scok: %d",
+               session->adapter->name, scok);
 
-       return mgmt_fe_adapter_send_msg(session->adapter, &fe_msg, false);
+       return mgmt_fe_adapter_send_msg(session->adapter, &fe_msg, scok);
 }
 
 static int mgmt_fe_send_setcfg_reply(struct mgmt_fe_session_ctx *session,