table: make path.Clone private

table_manager.go clones withdraw path before passing it to peer.go

clone() doesn't clone pathattributes. This patch adds a helper
funciton to clone PathAttributes to message.go (it is used by
createing BGPUpdate message to withdraw ipv6. later it will be used
for 2 byte AS peer).

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

4 files changed, 35 insertions, 52 deletions

diff --git a/table/message.go b/table/message.go
index 7ea1c361..a22c3b01 100644
--- a/table/message.go
+++ b/table/message.go
@@ -17,6 +17,7 @@ package table
 
 import (
 	"github.com/osrg/gobgp/packet"
+	"reflect"
 )
 
 func UpdatePathAttrs2ByteAs(msg *bgp.BGPUpdate) error {
@@ -108,6 +109,18 @@ func UpdatePathAttrs4ByteAs(msg *bgp.BGPUpdate) error {
 	return nil
 }
 
+func clonePathAttributes(attrs []bgp.PathAttributeInterface) []bgp.PathAttributeInterface {
+	clonedAttrs := []bgp.PathAttributeInterface(nil)
+	clonedAttrs = append(clonedAttrs, attrs...)
+	for i, attr := range clonedAttrs {
+		t, v := reflect.TypeOf(attr), reflect.ValueOf(attr)
+		newAttrObjp := reflect.New(t.Elem())
+		newAttrObjp.Elem().Set(v.Elem())
+		clonedAttrs[i] = newAttrObjp.Interface().(bgp.PathAttributeInterface)
+	}
+	return clonedAttrs
+}
+
 func CreateUpdateMsgFromPath(path Path, msg *bgp.BGPMessage) (*bgp.BGPMessage, error) {
 	rf := path.GetRouteFamily()
 
@@ -122,11 +135,13 @@ func CreateUpdateMsgFromPath(path Path, msg *bgp.BGPMessage) (*bgp.BGPMessage, e
 		}
 	} else if rf == bgp.RF_IPv6_UC {
 		if path.IsWithdraw() {
-			pathAttrs := path.GetPathAttrs()
-			return bgp.NewBGPUpdateMessage([]bgp.WithdrawnRoute{}, pathAttrs, []bgp.NLRInfo{}), nil
+			clonedAttrs := clonePathAttributes(path.GetPathAttrs())
+			idx, attr := path.GetPathAttr(bgp.BGP_ATTR_TYPE_MP_REACH_NLRI)
+			reach := attr.(*bgp.PathAttributeMpReachNLRI)
+			clonedAttrs[idx] = bgp.NewPathAttributeMpUnreachNLRI(reach.Value)
+			return bgp.NewBGPUpdateMessage([]bgp.WithdrawnRoute{}, clonedAttrs, []bgp.NLRInfo{}), nil
 		} else {
-			pathAttrs := path.GetPathAttrs()
-			return bgp.NewBGPUpdateMessage([]bgp.WithdrawnRoute{}, pathAttrs, []bgp.NLRInfo{}), nil
+			return bgp.NewBGPUpdateMessage([]bgp.WithdrawnRoute{}, path.GetPathAttrs(), []bgp.NLRInfo{}), nil
 		}
 	}
 	return nil, nil
diff --git a/table/path.go b/table/path.go
index 6c628448..96ffd250 100644
--- a/table/path.go
+++ b/table/path.go
@@ -41,7 +41,7 @@ type Path interface {
 	getPrefix() net.IP
 	setMedSetByTargetNeighbor(medSetByTargetNeighbor bool)
 	getMedSetByTargetNeighbor() bool
-	Clone(IsWithdraw bool) Path
+	clone(IsWithdraw bool) Path
 	setBest(isBest bool)
 	MarshalJSON() ([]byte, error)
 }
@@ -114,23 +114,16 @@ func (pd *PathDefault) MarshalJSON() ([]byte, error) {
 }
 
 // create new PathAttributes
-func (pd *PathDefault) Clone(isWithdraw bool) Path {
-	copiedAttrs := []bgp.PathAttributeInterface(nil)
+func (pd *PathDefault) clone(isWithdraw bool) Path {
 	nlri := pd.nlri
 	if isWithdraw {
-		if !pd.IsWithdraw() {
+		if pd.IsWithdraw() {
+			log.Fatal("Withdraw path is not supposed to be cloned")
+		} else {
 			nlri = &bgp.WithdrawnRoute{pd.nlri.(*bgp.NLRInfo).IPAddrPrefix}
 		}
-	} else {
-		copiedAttrs = append(copiedAttrs, pd.pathAttrs...)
-		for i, attr := range copiedAttrs {
-			t, v := reflect.TypeOf(attr), reflect.ValueOf(attr)
-			newAttrObjp := reflect.New(t.Elem())
-			newAttrObjp.Elem().Set(v.Elem())
-			copiedAttrs[i] = newAttrObjp.Interface().(bgp.PathAttributeInterface)
-		}
 	}
-	return CreatePath(pd.source, nlri, copiedAttrs, isWithdraw)
+	return CreatePath(pd.source, nlri, pd.pathAttrs, isWithdraw)
 }
 
 func (pd *PathDefault) GetRouteFamily() bgp.RouteFamily {
@@ -293,34 +286,14 @@ func NewIPv6Path(source *PeerInfo, nlri bgp.AddrPrefixInterface, sourceVerNum in
 	return ipv6Path
 }
 
-func (ipv6p *IPv6Path) Clone(isWithdraw bool) Path {
-	copiedAttrs := []bgp.PathAttributeInterface(nil)
+func (ipv6p *IPv6Path) clone(isWithdraw bool) Path {
 	nlri := ipv6p.nlri
 	if isWithdraw {
-		if !ipv6p.IsWithdraw() {
-			copiedAttrs = append(copiedAttrs, ipv6p.GetPathAttrs()...)
-			for i, attr := range copiedAttrs {
-				t, v := reflect.TypeOf(attr), reflect.ValueOf(attr)
-				newAttrObjp := reflect.New(t.Elem())
-				newAttrObjp.Elem().Set(v.Elem())
-				copiedAttrs[i] = newAttrObjp.Interface().(bgp.PathAttributeInterface)
-			}
-			idx, attr := ipv6p.GetPathAttr(bgp.BGP_ATTR_TYPE_MP_REACH_NLRI)
-			reach := attr.(*bgp.PathAttributeMpReachNLRI)
-			copiedAttrs[idx] = bgp.NewPathAttributeMpUnreachNLRI(reach.Value)
-		} else {
-			copiedAttrs = ipv6p.GetPathAttrs()
-		}
-	} else {
-		copiedAttrs = append(copiedAttrs, ipv6p.pathAttrs...)
-		for i, attr := range copiedAttrs {
-			t, v := reflect.TypeOf(attr), reflect.ValueOf(attr)
-			newAttrObjp := reflect.New(t.Elem())
-			newAttrObjp.Elem().Set(v.Elem())
-			copiedAttrs[i] = newAttrObjp.Interface().(bgp.PathAttributeInterface)
+		if ipv6p.IsWithdraw() {
+			log.Fatal("Withdraw path is not supposed to be cloned")
 		}
 	}
-	return CreatePath(ipv6p.source, nlri, copiedAttrs, isWithdraw)
+	return CreatePath(ipv6p.source, nlri, ipv6p.pathAttrs, isWithdraw)
 }
 
 func (ipv6p *IPv6Path) setPathDefault(pd *PathDefault) {
diff --git a/table/table_manager.go b/table/table_manager.go
index 4b9c0b97..7112c0ca 100644
--- a/table/table_manager.go
+++ b/table/table_manager.go
@@ -162,8 +162,9 @@ func (manager *TableManager) calculate(destinationList []Destination) ([]Path, [
 				// create withdraw path
 				if currentBestPath != nil {
 					log.Debug("best path is lost")
-					destination.setOldBestPath(destination.getBestPath())
-					lostPaths = append(lostPaths, destination.getBestPath())
+					p := destination.getBestPath()
+					destination.setOldBestPath(p)
+					lostPaths = append(lostPaths, p.clone(true))
 				}
 				destination.setBestPath(nil)
 			} else {
diff --git a/table/table_manager_test.go b/table/table_manager_test.go
index 66a5b2c5..761e6f54 100644
--- a/table/table_manager_test.go
+++ b/table/table_manager_test.go
@@ -1640,10 +1640,6 @@ func TestProcessBGPUpdate_bestpath_lost_ipv4(t *testing.T) {
 	// check destination
 	expectedPrefix := "10.10.10.0"
 	assert.Equal(t, expectedPrefix, path.getPrefix().String())
-	// check nexthop
-	expectedNexthop := "192.168.50.1"
-	assert.Equal(t, expectedNexthop, path.getNexthop().String())
-
 }
 
 func TestProcessBGPUpdate_bestpath_lost_ipv6(t *testing.T) {
@@ -1719,10 +1715,6 @@ func TestProcessBGPUpdate_bestpath_lost_ipv6(t *testing.T) {
 	// check destination
 	expectedPrefix := "2001:123:123:1::"
 	assert.Equal(t, expectedPrefix, path.getPrefix().String())
-	// check nexthop
-	expectedNexthop := "2001::192:168:50:1"
-	assert.Equal(t, expectedNexthop, path.getNexthop().String())
-
 }
 
 // test: implicit withdrawal case
@@ -2206,7 +2198,9 @@ func TestModifyPathAttribute(t *testing.T) {
 	assert.NoError(t, err)
 
 	path0 := pList[0]
-	path1 := path0.Clone(false)
+	path1 := path0.clone(false)
+	ipv4p := path1.(*IPv4Path)
+	ipv4p.PathDefault.pathAttrs = clonePathAttributes(path0.GetPathAttrs())
 
 	_, attr1 := path1.GetPathAttr(bgp.BGP_ATTR_TYPE_MULTI_EXIT_DISC)
 	mx1 := attr1.(*bgp.PathAttributeMultiExitDisc)