From 8e41bf768a90199c85dfcbf7ea4f1fac99f77fe8 Mon Sep 17 00:00:00 2001
From: Nischal Sheth <nsheth@juniper.net>
Date: Fri, 8 Apr 2016 12:19:39 -0700
Subject: [PATCH] Fix corner case in bgp peer close processing

Do not initialize the state machine on admin up if close process is in
progress.  The state machine will eventually be initialized when close
finishes, via BgpPeer::BgpPeerClose::CloseComplete.

Note that this only impacts unit tests since we don't expose admin/up
down functionality to users.

Change-Id: Id3c18186dbe4fb232cdde2e0e1eb749faa3bc19e
Partial-Bug: 1548570
---
 src/bgp/state_machine.cc             | 7 ++++---
 src/bgp/test/bgp_server_test.cc      | 6 +++++-
 src/bgp/test/bgp_server_test_util.cc | 3 +++
 src/bgp/test/bgp_stress_test.cc      | 2 ++
 src/bgp/test/state_machine_test.cc   | 2 ++
 5 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/bgp/state_machine.cc b/src/bgp/state_machine.cc
index 7f0eab44cb9..c997ff45802 100644
--- a/src/bgp/state_machine.cc
+++ b/src/bgp/state_machine.cc
@@ -1137,11 +1137,12 @@ void StateMachine::SetAdminState(bool down) {
     if (down) {
         Enqueue(fsm::EvStop(BgpProto::Notification::AdminShutdown));
     } else {
+        // Reset all previous state.
         reset_idle_hold_time();
-        peer_->reset_flap_count();
-        // On fresh restart of state machine, all previous state should be reset
         reset_last_info();
-        Enqueue(fsm::EvStart());
+        peer_->reset_flap_count();
+        if (!peer_->IsCloseInProgress())
+            Enqueue(fsm::EvStart());
     }
 }
 
diff --git a/src/bgp/test/bgp_server_test.cc b/src/bgp/test/bgp_server_test.cc
index c17d4fa4ea2..b3223691715 100644
--- a/src/bgp/test/bgp_server_test.cc
+++ b/src/bgp/test/bgp_server_test.cc
@@ -2,7 +2,7 @@
  * Copyright (c) 2013 Juniper Networks, Inc. All rights reserved.
  */
 
-
+#include "base/task_annotations.h"
 #include "bgp/bgp_config_parser.h"
 #include "bgp/bgp_factory.h"
 #include "bgp/bgp_peer_membership.h"
@@ -1224,6 +1224,7 @@ TEST_F(BgpServerUnitTest, ChangePeerAddressFamilies) {
 }
 
 TEST_F(BgpServerUnitTest, AdminDown) {
+    ConcurrencyScope scope("bgp::Config");
     int peer_count = 3;
 
     BgpPeerTest::verbose_name(true);
@@ -1867,6 +1868,7 @@ TEST_F(BgpServerUnitTest, Passive2) {
 }
 
 TEST_F(BgpServerUnitTest, ResetStatsOnFlap) {
+    ConcurrencyScope scope("bgp::Config");
     int peer_count = 3;
 
     StateMachineTest::set_keepalive_time_msecs(10);
@@ -2650,6 +2652,7 @@ TEST_F(BgpServerUnitTest, DeleteInProgress) {
 }
 
 TEST_F(BgpServerUnitTest, CloseInProgress) {
+    ConcurrencyScope scope("bgp::Config");
     int peer_count = 3;
 
     vector<string> families_a;
@@ -2702,6 +2705,7 @@ TEST_F(BgpServerUnitTest, CloseInProgress) {
 }
 
 TEST_F(BgpServerUnitTest, CloseDeferred) {
+    ConcurrencyScope scope("bgp::Config");
     int peer_count = 3;
 
     vector<string> families_a;
diff --git a/src/bgp/test/bgp_server_test_util.cc b/src/bgp/test/bgp_server_test_util.cc
index 66d650b22ca..7cf8cd787e1 100644
--- a/src/bgp/test/bgp_server_test_util.cc
+++ b/src/bgp/test/bgp_server_test_util.cc
@@ -6,6 +6,7 @@
 
 #include <boost/foreach.hpp>
 
+#include "base/task_annotations.h"
 #include "bgp/bgp_config_ifmap.h"
 #include "bgp/bgp_config_parser.h"
 #include "bgp/bgp_factory.h"
@@ -138,6 +139,7 @@ BgpPeer *BgpServerTest::FindMatchingPeer(const string &routing_instance,
 }
 
 void BgpServerTest::DisableAllPeers() {
+    ConcurrencyScope scope("bgp::Config");
     for (BgpPeerList::iterator it = peer_list_.begin();
          it != peer_list_.end(); ++it) {
         it->second->SetAdminState(true);
@@ -145,6 +147,7 @@ void BgpServerTest::DisableAllPeers() {
 }
 
 void BgpServerTest::EnableAllPeers() {
+    ConcurrencyScope scope("bgp::Config");
     for (BgpPeerList::iterator it = peer_list_.begin();
          it != peer_list_.end(); ++it) {
         it->second->SetAdminState(false);
diff --git a/src/bgp/test/bgp_stress_test.cc b/src/bgp/test/bgp_stress_test.cc
index 408bd23d25c..f76a66e8801 100644
--- a/src/bgp/test/bgp_stress_test.cc
+++ b/src/bgp/test/bgp_stress_test.cc
@@ -11,6 +11,7 @@
 #include <map>
 #include <string>
 
+#include "base/task_annotations.h"
 #include "base/test/addr_test_util.h"
 
 #include "bgp/bgp_config_parser.h"
@@ -2103,6 +2104,7 @@ void BgpStressTest::DeleteBgpPeers(int npeers) {
 }
 
 void BgpStressTest::ClearBgpPeer(vector<int> peer_ids) {
+    ConcurrencyScope scope("bgp::Config");
     map<int, bool> established;
     map<int, uint32_t> flap_count;
 
diff --git a/src/bgp/test/state_machine_test.cc b/src/bgp/test/state_machine_test.cc
index 3d1152d35a4..2de5e43bccc 100644
--- a/src/bgp/test/state_machine_test.cc
+++ b/src/bgp/test/state_machine_test.cc
@@ -394,10 +394,12 @@ class StateMachineUnitTest : public ::testing::Test {
         sm_->Shutdown(BgpProto::Notification::AdminShutdown);
     }
     void EvAdminUp() {
+        ConcurrencyScope scope("bgp::Config");
         peer_->SetAdminState(false);
         sm_->set_idle_hold_time(1);
     }
     void EvAdminDown() {
+        ConcurrencyScope scope("bgp::Config");
         peer_->SetAdminState(true);
         sm_->set_idle_hold_time(1);
     }