server: add missing mutex lock when updating policy configuration

The useage of policyMutex is very hacky. It should be moved to table/policy.go

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

2 files changed, 14 insertions, 15 deletions

diff --git a/gobgpd/main.go b/gobgpd/main.go
index 63d71024..32c63bcc 100644
--- a/gobgpd/main.go
+++ b/gobgpd/main.go
@@ -225,7 +225,7 @@ func main() {
 					log.Fatalf("failed to set mrt config: %s", err)
 				}
 				p := config.ConfigSetToRoutingPolicy(newConfig)
-				if err := bgpServer.SetRoutingPolicy(*p); err != nil {
+				if err := bgpServer.UpdatePolicy(*p); err != nil {
 					log.Fatalf("failed to set routing policy: %s", err)
 				}
 
diff --git a/server/server.go b/server/server.go
index 286ff85b..05a1f295 100644
--- a/server/server.go
+++ b/server/server.go
@@ -941,16 +941,19 @@ func (server *BgpServer) Shutdown() {
 	// TODO: call fsmincomingCh.Close()
 }
 
-func (server *BgpServer) UpdatePolicy(policy config.RoutingPolicy) {
+func (server *BgpServer) UpdatePolicy(policy config.RoutingPolicy) error {
 	ch := make(chan *GrpcResponse)
 	server.GrpcReqCh <- &GrpcRequest{
 		RequestType: REQ_RELOAD_POLICY,
 		Data:        policy,
 		ResponseCh:  ch,
 	}
-	<-ch
+	res := <-ch
+	return res.Err()
 }
 
+// This function MUST be called with policyMutex locked.
+// FIXME: move policyMutex to table/policy.go
 func (server *BgpServer) setPolicyByConfig(id string, c config.ApplyPolicy) {
 	for _, dir := range []table.PolicyDirection{table.POLICY_DIRECTION_IN, table.POLICY_DIRECTION_IMPORT, table.POLICY_DIRECTION_EXPORT} {
 		ps, def, err := server.policy.GetAssignmentFromConfig(dir, c)
@@ -966,7 +969,9 @@ func (server *BgpServer) setPolicyByConfig(id string, c config.ApplyPolicy) {
 	}
 }
 
-func (server *BgpServer) SetRoutingPolicy(pl config.RoutingPolicy) error {
+func (server *BgpServer) handlePolicy(pl config.RoutingPolicy) error {
+	policyMutex.Lock()
+	defer policyMutex.Unlock()
 	if err := server.policy.Reload(pl); err != nil {
 		log.WithFields(log.Fields{
 			"Topic": "Policy",
@@ -974,16 +979,6 @@ func (server *BgpServer) SetRoutingPolicy(pl config.RoutingPolicy) error {
 		return err
 	}
 	server.setPolicyByConfig(table.GLOBAL_RIB_NAME, server.bgpConfig.Global.ApplyPolicy)
-	return nil
-}
-
-func (server *BgpServer) handlePolicy(pl config.RoutingPolicy) error {
-	if err := server.SetRoutingPolicy(pl); err != nil {
-		log.WithFields(log.Fields{
-			"Topic": "Policy",
-		}).Errorf("failed to set new policy: %s", err)
-		return err
-	}
 	for _, peer := range server.neighborMap {
 		log.WithFields(log.Fields{
 			"Topic": "Peer",
@@ -1490,7 +1485,7 @@ func (server *BgpServer) handleModConfig(grpcReq *GrpcRequest) error {
 	server.globalRib = table.NewTableManager(rfs, c.MplsLabelRange.MinLabel, c.MplsLabelRange.MaxLabel)
 
 	p := config.RoutingPolicy{}
-	if err := server.SetRoutingPolicy(p); err != nil {
+	if err := server.handlePolicy(p); err != nil {
 		return err
 	}
 	server.bgpConfig.Global = *c
@@ -2220,7 +2215,9 @@ func (server *BgpServer) handleAddNeighbor(c *config.Neighbor) error {
 	log.Info("Add a peer configuration for ", addr)
 
 	peer := NewPeer(&server.bgpConfig.Global, c, server.globalRib, server.policy)
+	policyMutex.Lock()
 	server.setPolicyByConfig(peer.ID(), c.ApplyPolicy)
+	policyMutex.Unlock()
 	if peer.isRouteServerClient() {
 		pathList := make([]*table.Path, 0)
 		rfList := peer.configuredRFlist()
@@ -2279,8 +2276,10 @@ func (server *BgpServer) handleUpdateNeighbor(c *config.Neighbor) (bool, error)
 			"Topic": "Peer",
 			"Key":   addr,
 		}).Info("Update ApplyPolicy")
+		policyMutex.Lock()
 		server.setPolicyByConfig(peer.ID(), c.ApplyPolicy)
 		peer.fsm.pConf.ApplyPolicy = c.ApplyPolicy
+		policyMutex.Unlock()
 		policyUpdated = true
 	}
 	original := peer.fsm.pConf