From f14892aaef96f75bd1564f8e6ea9bd8ff614d43a Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Fri, 26 Feb 2016 17:58:51 -0800 Subject: [PATCH] Fix concurrency issue in XmppMessageBuilder Do not access BgpPath from bgp::SendTask because the path can get deleted from db::DBTable task which could be running in parallel. Store vrf_originated_ attribute in RibOutAttr instead of accessing the best path to figure out if the route is local Vrf Originated. Change-Id: If5318153f83d1032495c7e7f0c56293745cb37bc Closes-Bug: 1551015 --- src/bgp/bgp_evpn.cc | 3 ++- src/bgp/bgp_multicast.cc | 2 +- src/bgp/bgp_ribout.cc | 31 +++++++++++++++++++++---------- src/bgp/bgp_ribout.h | 11 +++++++---- src/bgp/xmpp_message_builder.cc | 3 +-- 5 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/bgp/bgp_evpn.cc b/src/bgp/bgp_evpn.cc index 3c073ab22a8..255e4c20170 100644 --- a/src/bgp/bgp_evpn.cc +++ b/src/bgp/bgp_evpn.cc @@ -273,7 +273,8 @@ UpdateInfo *EvpnLocalMcastNode::GetUpdateInfo() { attr = attr_db->ReplaceLeafOListAndLocate(attr.get(), &leaf_olist_spec); UpdateInfo *uinfo = new UpdateInfo; - uinfo->roattr = RibOutAttr(partition_->table(), attr.get(), 0, false); + uinfo->roattr = + RibOutAttr(partition_->table(), route_, attr.get(), 0, false); return uinfo; } diff --git a/src/bgp/bgp_multicast.cc b/src/bgp/bgp_multicast.cc index b5d6d03f09f..9f6b8f85f96 100644 --- a/src/bgp/bgp_multicast.cc +++ b/src/bgp/bgp_multicast.cc @@ -347,7 +347,7 @@ UpdateInfo *McastForwarder::GetUpdateInfo(ErmVpnTable *table) { BgpAttrPtr attr = table->server()->attr_db()->Locate(attr_spec); UpdateInfo *uinfo = new UpdateInfo; - uinfo->roattr = RibOutAttr(table, attr.get(), label_); + uinfo->roattr = RibOutAttr(table, route_, attr.get(), label_); return uinfo; } diff --git a/src/bgp/bgp_ribout.cc b/src/bgp/bgp_ribout.cc index c011e5d794c..ee1eb2019dd 100644 --- a/src/bgp/bgp_ribout.cc +++ b/src/bgp/bgp_ribout.cc @@ -67,10 +67,8 @@ RibOutAttr::NextHop::NextHop(const BgpTable *table, IpAddress address, origin_vn_index_ = ext_community->GetOriginVnIndex(); } if (origin_vn_index_ < 0 && vrf_originated) { - if (table) - origin_vn_index_ = table->routing_instance()->virtual_network_index(); - else - origin_vn_index_ = 0; + origin_vn_index_ = + table ? table->routing_instance()->virtual_network_index() : 0; } } @@ -99,14 +97,27 @@ bool RibOutAttr::NextHop::operator!=(const NextHop &rhs) const { } RibOutAttr::RibOutAttr(const BgpTable *table, const BgpAttr *attr, - uint32_t label, bool include_nh) : attr_out_(attr) { + uint32_t label) + : attr_out_(attr), + vrf_originated_(false) { + if (attr) { + nexthop_list_.push_back(NextHop(table, attr->nexthop(), label, + attr->ext_community(), false)); + } +} + +RibOutAttr::RibOutAttr(const BgpTable *table, const BgpRoute *route, + const BgpAttr *attr, uint32_t label, bool include_nh) + : attr_out_(attr), + vrf_originated_(route->BestPath()->IsVrfOriginated()) { if (attr && include_nh) { nexthop_list_.push_back(NextHop(table, attr->nexthop(), label, - attr->ext_community(), false)); + attr->ext_community(), vrf_originated_)); } } -RibOutAttr::RibOutAttr(BgpRoute *route, const BgpAttr *attr, bool is_xmpp) { +RibOutAttr::RibOutAttr(const BgpRoute *route, const BgpAttr *attr, + bool is_xmpp) : vrf_originated_(false) { // Attribute should not be set already assert(!attr_out_); @@ -123,9 +134,9 @@ RibOutAttr::RibOutAttr(BgpRoute *route, const BgpAttr *attr, bool is_xmpp) { set_attr(table, attr, route->BestPath()->GetLabel(), route->BestPath()->IsVrfOriginated()); - for (Route::PathList::iterator it = route->GetPathList().begin(); - it != route->GetPathList().end(); it++) { - const BgpPath *path = static_cast(it.operator->()); + for (Route::PathList::const_iterator it = route->GetPathList().begin(); + it != route->GetPathList().end(); ++it) { + const BgpPath *path = static_cast(it.operator->()); // Skip the best path. if (path == route->BestPath()) diff --git a/src/bgp/bgp_ribout.h b/src/bgp/bgp_ribout.h index e255b72899b..597e76e0767 100644 --- a/src/bgp/bgp_ribout.h +++ b/src/bgp/bgp_ribout.h @@ -68,10 +68,11 @@ class RibOutAttr { typedef std::vector NextHopList; - RibOutAttr() : attr_out_(NULL) { } - RibOutAttr(const BgpTable *table, const BgpAttr *attr, uint32_t label, - bool include_nh = true); - RibOutAttr(BgpRoute *route, const BgpAttr *attr, bool is_xmpp); + RibOutAttr() : attr_out_(NULL), vrf_originated_(false) { } + RibOutAttr(const BgpTable *table, const BgpAttr *attr, uint32_t label); + RibOutAttr(const BgpTable *table, const BgpRoute *route, + const BgpAttr *attr, uint32_t label, bool include_nh = true); + RibOutAttr(const BgpRoute *route, const BgpAttr *attr, bool is_xmpp); bool IsReachable() const { return attr_out_.get() != NULL; } bool operator==(const RibOutAttr &rhs) const { return CompareTo(rhs) == 0; } @@ -89,12 +90,14 @@ class RibOutAttr { uint32_t label() const { return nexthop_list_.empty() ? 0 : nexthop_list_.at(0).label(); } + bool vrf_originated() const { return vrf_originated_; } private: int CompareTo(const RibOutAttr &rhs) const; BgpAttrPtr attr_out_; NextHopList nexthop_list_; + bool vrf_originated_; }; // diff --git a/src/bgp/xmpp_message_builder.cc b/src/bgp/xmpp_message_builder.cc index bdfc2ab08fd..336971da51a 100644 --- a/src/bgp/xmpp_message_builder.cc +++ b/src/bgp/xmpp_message_builder.cc @@ -457,8 +457,7 @@ string BgpXmppMessage::GetVirtualNetwork(const BgpRoute *route, if (!is_reachable_) { return "unresolved"; } else if (roattr->nexthop_list().empty()) { - const BgpPath *path = route->BestPath(); - if (path && path->IsVrfOriginated()) { + if (roattr->vrf_originated()) { return table_->routing_instance()->GetVirtualNetworkName(); } else { return "unresolved";