mgmtd: fix commit request overwrite

There are places, where we can receive an existing commit transaction.
If we don't check that the request already exists, it gets overwritten
and we start having problems with transaction refcounters. Forbid having
multiple configuration sessions simultaneously.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>

diff --git a/mgmtd/mgmt_be_adapter.c b/mgmtd/mgmt_be_adapter.c
index 4f5ad6e0b0828787433570db6d03fad2c7ed366b..8e719457f6a38b01a53870a4169afa2ba90bb941 100644 (file)
--- a/mgmtd/mgmt_be_adapter.c
+++ b/mgmtd/mgmt_be_adapter.c
@@ -607,28 +607,17 @@ static void mgmt_be_adapter_conn_init(struct event *thread)
        adapter = (struct mgmt_be_client_adapter *)EVENT_ARG(thread);
        assert(adapter && adapter->conn->fd >= 0);
 
-       /*
-        * Check first if the current session can run a CONFIG
-        * transaction or not. Reschedule if a CONFIG transaction
-        * from another session is already in progress.
-        */
-       if (mgmt_config_txn_in_progress() != MGMTD_SESSION_ID_NONE) {
-               zlog_err("XXX txn in progress, retry init");
-               mgmt_be_adapter_sched_init_event(adapter);
-               return;
-       }
-
        /*
         * Notify TXN module to create a CONFIG transaction and
         * download the CONFIGs identified for this new client.
         * If the TXN module fails to initiate the CONFIG transaction
-        * disconnect from the client forcing a reconnect later.
-        * That should also take care of destroying the adapter.
+        * retry a bit later. It only fails if there's an existing config
+        * transaction in progress.
         */
        if (mgmt_txn_notify_be_adapter_conn(adapter, true) != 0) {
-               zlog_err("XXX notify be adapter conn fail");
-               msg_conn_disconnect(adapter->conn, false);
-               adapter = NULL;
+               zlog_err("XXX txn in progress, retry init");
+               mgmt_be_adapter_sched_init_event(adapter);
+               return;
        }
 }
 

diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c
index d91987d8884f91d132c6e139c9d7b11dd9bc804e..ff85f56e3e46e9213af3db9c803faea6d12e9da5 100644 (file)
--- a/mgmtd/mgmt_fe_adapter.c
+++ b/mgmtd/mgmt_fe_adapter.c
@@ -724,7 +724,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session,
 
        if (session->cfg_txn_id == MGMTD_TXN_ID_NONE) {
                /* as we have the lock no-one else should have a config txn */
-               assert(mgmt_config_txn_in_progress() == MGMTD_SESSION_ID_NONE);
+               assert(!mgmt_config_txn_in_progress());
 
                /* Start a CONFIG Transaction (if not started already) */
                session->cfg_txn_id = mgmt_create_txn(session->session_id,
@@ -890,6 +890,9 @@ static int mgmt_fe_session_handle_commit_config_req_msg(
        }
 
        if (session->cfg_txn_id == MGMTD_TXN_ID_NONE) {
+               /* as we have the lock no-one else should have a config txn */
+               assert(!mgmt_config_txn_in_progress());
+
                /*
                 * Start a CONFIG Transaction (if not started already)
                 */

diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c
index 45bc3a293004bdde920fbfed4650af671cfabdee..786fa0d0a3e12da5059be2ca0887cfcf088c43dd 100644 (file)
--- a/mgmtd/mgmt_txn.c
+++ b/mgmtd/mgmt_txn.c
@@ -1723,15 +1723,9 @@ static struct mgmt_txn_ctx *mgmt_txn_create_new(uint64_t session_id,
 {
        struct mgmt_txn_ctx *txn = NULL;
 
-       /*
-        * For 'CONFIG' transaction check if one is already created
-        * or not. TODO: figure out what code counts on this and fix it.
-        */
-       if (type == MGMTD_TXN_TYPE_CONFIG && mgmt_txn_mm->cfg_txn) {
-               if (mgmt_config_txn_in_progress() == session_id)
-                       txn = mgmt_txn_mm->cfg_txn;
-               goto mgmt_create_txn_done;
-       }
+       /* Do not allow multiple config transactions */
+       if (type == MGMTD_TXN_TYPE_CONFIG && mgmt_config_txn_in_progress())
+               return NULL;
 
        txn = mgmt_fe_find_txn_by_session_id(mgmt_txn_mm, session_id, type);
        if (!txn) {
@@ -1761,7 +1755,6 @@ static struct mgmt_txn_ctx *mgmt_txn_create_new(uint64_t session_id,
                MGMTD_TXN_LOCK(txn);
        }
 
-mgmt_create_txn_done:
        return txn;
 }
 
@@ -1949,12 +1942,12 @@ void mgmt_txn_destroy(void)
        mgmt_txn_hash_destroy();
 }
 
-uint64_t mgmt_config_txn_in_progress(void)
+bool mgmt_config_txn_in_progress(void)
 {
        if (mgmt_txn_mm && mgmt_txn_mm->cfg_txn)
-               return mgmt_txn_mm->cfg_txn->session_id;
+               return true;
 
-       return MGMTD_SESSION_ID_NONE;
+       return false;
 }
 
 uint64_t mgmt_create_txn(uint64_t session_id, enum mgmt_txn_type type)

diff --git a/mgmtd/mgmt_txn.h b/mgmtd/mgmt_txn.h
index 3f27f2f07b78e59653f0d97a621028b71641b0f1..02b2baa95fd9c7f9fc5473e37d2b04e7329e0996 100644 (file)
--- a/mgmtd/mgmt_txn.h
+++ b/mgmtd/mgmt_txn.h
@@ -71,12 +71,12 @@ extern int mgmt_txn_init(struct mgmt_master *cm, struct event_loop *tm);
 extern void mgmt_txn_destroy(void);
 
 /*
- * Check if transaction is in progress.
+ * Check if configuration transaction is in progress.
  *
  * Returns:
- *    session ID if in-progress, MGMTD_SESSION_ID_NONE otherwise.
+ *    true if in-progress, false otherwise.
  */
-extern uint64_t mgmt_config_txn_in_progress(void);
+extern bool mgmt_config_txn_in_progress(void);
 
 /**
  * Get the session ID associated with the given ``txn-id``.