From 186c8c64f47ad8656d6810038afa1c60e9d1cf6d Mon Sep 17 00:00:00 2001
From: Ananth Suryanarayana <anantha@juniper.net>
Date: Thu, 14 Apr 2016 17:58:29 -0700
Subject: [PATCH] Do not call BgpPeer::SetAdminState() from main thread

To avoid data corruption, add a work queue and use a task off bgp::Config
exclusive task. Code is generic and will support other such functionality
in future, as needed

Change-Id: I16a17b349bc5ac8adbc8777c2f7eb5a134785b4a
Closes-Bug: #1570649
---
 src/base/task_annotations.h          |  1 +
 src/bgp/bgp_peer.h                   |  2 +-
 src/bgp/test/bgp_server_test_util.cc | 24 ++++++++++++++++++++-
 src/bgp/test/bgp_server_test_util.h  | 31 +++++++++++++++++++++++++++-
 4 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/src/base/task_annotations.h b/src/base/task_annotations.h
index 6f97b4cda4a..bd999352046 100644
--- a/src/base/task_annotations.h
+++ b/src/base/task_annotations.h
@@ -18,6 +18,7 @@ class ConcurrencyChecker {
     ConcurrencyChecker(const char *task_ids[], size_t count);
     void Check();
     void CheckIfMainThr();
+    static bool IsInMainThr() { return (Task::Running() == NULL); }
 private:
     typedef std::set<int> TaskIdSet;
     TaskIdSet id_set_;
diff --git a/src/bgp/bgp_peer.h b/src/bgp/bgp_peer.h
index 9d850072702..2d71584f52c 100644
--- a/src/bgp/bgp_peer.h
+++ b/src/bgp/bgp_peer.h
@@ -99,7 +99,7 @@ class BgpPeer : public IPeer {
 
     BgpSession *CreateSession();
 
-    void SetAdminState(bool down);
+    virtual void SetAdminState(bool down);
 
     // Messages
 
diff --git a/src/bgp/test/bgp_server_test_util.cc b/src/bgp/test/bgp_server_test_util.cc
index d76fef4086e..66d650b22ca 100644
--- a/src/bgp/test/bgp_server_test_util.cc
+++ b/src/bgp/test/bgp_server_test_util.cc
@@ -186,7 +186,9 @@ void BgpPeerTest::SetDataCollectionKey(BgpPeerInfo *peer_info) const {
 //
 BgpPeerTest::BgpPeerTest(BgpServer *server, RoutingInstance *rtinst,
                          const BgpNeighborConfig *config)
-        : BgpPeer(server, rtinst, config), id_(0) {
+        : BgpPeer(server, rtinst, config), id_(0),
+          work_queue_(TaskScheduler::GetInstance()->GetTaskId("bgp::Config"), 0,
+                      boost::bind(&BgpPeerTest::ProcessRequest, this, _1)) {
     SendUpdate_fnc_ = boost::bind(&BgpPeerTest::BgpPeerSendUpdate, this,
                                   _1, _2);
     MpNlriAllowed_fnc_ = boost::bind(&BgpPeerTest::BgpPeerMpNlriAllowed, this,
@@ -197,6 +199,26 @@ BgpPeerTest::BgpPeerTest(BgpServer *server, RoutingInstance *rtinst,
 BgpPeerTest::~BgpPeerTest() {
 }
 
+// Process requests and run them off bgp::Config exclusive task
+bool BgpPeerTest::ProcessRequest(Request *request) {
+    CHECK_CONCURRENCY("bgp::Config");
+    switch (request->type) {
+        case ADMIN_UP:
+            BgpPeer::SetAdminState(false);
+            request->result = true;
+            break;
+        case ADMIN_DOWN:
+            BgpPeer::SetAdminState(true);
+            request->result = true;
+            break;
+    }
+
+    // Notify waiting caller with the result
+    tbb::mutex::scoped_lock lock(work_mutex_);
+    cond_var_.notify_all();
+    return true;
+}
+
 //
 // Enable this if peer name uuid is also required in all bgp peer logs
 //
diff --git a/src/bgp/test/bgp_server_test_util.h b/src/bgp/test/bgp_server_test_util.h
index ec54e5d85f4..b9c5dd4b1f3 100644
--- a/src/bgp/test/bgp_server_test_util.h
+++ b/src/bgp/test/bgp_server_test_util.h
@@ -8,9 +8,12 @@
 #include <boost/any.hpp>
 #include <boost/foreach.hpp>
 #include <boost/shared_ptr.hpp>
+#include <tbb/compat/condition_variable>
+#include <tbb/mutex.h>
 
-#include "base/util.h"
+#include "base/task_annotations.h"
 #include "base/test/task_test_util.h"
+#include "base/util.h"
 #include "bgp/bgp_config.h"
 #include "bgp/bgp_lifetime.h"
 #include "bgp/bgp_log.h"
@@ -309,6 +312,21 @@ class BgpPeerTest : public BgpPeer {
     const int id() const { return id_; }
     void set_id(int id) { id_ = id; }
 
+    virtual void SetAdminState(bool down) {
+        if (!ConcurrencyChecker::IsInMainThr()) {
+            BgpPeer::SetAdminState(down);
+            return;
+        }
+        tbb::interface5::unique_lock<tbb::mutex> lock(work_mutex_);
+
+        Request request;
+        request.type = down ? ADMIN_DOWN : ADMIN_UP;
+        work_queue_.Enqueue(&request);
+
+        // Wait for the request to get processed.
+        cond_var_.wait(lock);
+    }
+
     boost::function<bool(const uint8_t *, size_t)> SendUpdate_fnc_;
     boost::function<bool(uint16_t, uint8_t)> MpNlriAllowed_fnc_;
     boost::function<bool()> IsReady_fnc_;
@@ -316,8 +334,19 @@ class BgpPeerTest : public BgpPeer {
     BgpTestUtil util_;
 
 private:
+    enum RequestType { ADMIN_UP, ADMIN_DOWN };
+    struct Request {
+        Request() : result(false) { }
+        RequestType type;
+        bool        result;
+    };
+    bool ProcessRequest(Request *request);
+
     static bool verbose_name_;
     int id_;
+    WorkQueue<Request *> work_queue_;
+    tbb::mutex work_mutex_;
+    tbb::interface5::condition_variable cond_var_;
 };
 
 class PeerManagerTest : public PeerManager {