Merge "Handle error where generated bgp update is too big" into R2.0

src/bgp/bgp_message_builder.cc

@@ -5,16 +5,19 @@
 #include "bgp/bgp_message_builder.h"
 
 #include "base/parse_object.h"
+#include "bgp/bgp_log.h"
 #include "bgp/bgp_route.h"
 #include "net/bgp_af.h"
 
-BgpMessage::BgpMessage() {
+using std::auto_ptr;
+
+BgpMessage::BgpMessage(const BgpTable *table) : table_(table), datalen_(0) {
 }
 
 BgpMessage::~BgpMessage() {
 }
 
-void BgpMessage::StartReach(const RibOutAttr *roattr, const BgpRoute *route) {
+bool BgpMessage::StartReach(const RibOutAttr *roattr, const BgpRoute *route) {
     BgpProto::Update update;
     const BgpAttr *attr = roattr->attr();
 
@@ -103,42 +106,56 @@ void BgpMessage::StartReach(const RibOutAttr *roattr, const BgpRoute *route) {
         new BgpMpNlri(BgpAttribute::MPReachNlri, route->Afi(), route->Safi(), nh);
     update.path_attributes.push_back(nlri);
 
-    if (route) {
-        BgpProtoPrefix *prefix = new BgpProtoPrefix;
-        route->BuildProtoPrefix(prefix, attr, roattr->label());
-        nlri->nlri.push_back(prefix);
-        num_reach_route_++;
+    BgpProtoPrefix *prefix = new BgpProtoPrefix;
+    route->BuildProtoPrefix(prefix, attr, roattr->label());
+    nlri->nlri.push_back(prefix);
+
+    int result =
+        BgpProto::Encode(&update, data_, sizeof(data_), &encode_offsets_);
+    if (result <= 0) {
+        BGP_LOG_STR(BgpMessage, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL,
+            "Error encoding reach message for route " << route->ToString() <<
+            " in table " << (table_ ? table_->name() : "unknown"));
+        table_->server()->increment_message_build_error();
+        return false;
     }
 
-    datalen_ = BgpProto::Encode(&update, data_, sizeof(data_),
-            &encode_offsets_);
-    assert(datalen_ > 0);
+    num_reach_route_++;
+    datalen_ = result;
+    return true;
 }
 
-void BgpMessage::StartUnreach(const BgpRoute *route) {
+bool BgpMessage::StartUnreach(const BgpRoute *route) {
     BgpProto::Update update;
 
     BgpMpNlri *nlri =
         new BgpMpNlri(BgpAttribute::MPUnreachNlri, route->Afi(), route->Safi());
     update.path_attributes.push_back(nlri);
 
-    if (route) {
-        BgpProtoPrefix *prefix = new BgpProtoPrefix;
-        route->BuildProtoPrefix(prefix);
-        nlri->nlri.push_back(prefix);
-        num_unreach_route_++;
+    BgpProtoPrefix *prefix = new BgpProtoPrefix;
+    route->BuildProtoPrefix(prefix);
+    nlri->nlri.push_back(prefix);
+
+    int result =
+        BgpProto::Encode(&update, data_, sizeof(data_), &encode_offsets_);
+    if (result <= 0) {
+        BGP_LOG_STR(BgpMessage, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL,
+            "Error encoding unreach message for route " << route->ToString() <<
+            " in table " << (table_ ? table_->name() : "unknown"));
+        table_->server()->increment_message_build_error();
+        return false;
     }
 
-    datalen_ = BgpProto::Encode(&update, data_, sizeof(data_),
-            &encode_offsets_);
-    assert(datalen_ > 0);
+    num_unreach_route_++;
+    datalen_ = result;
+    return true;
 }
 
-void BgpMessage::Start(const RibOutAttr *roattr, const BgpRoute *route) {
+bool BgpMessage::Start(const RibOutAttr *roattr, const BgpRoute *route) {
     if (roattr->IsReachable()) {
-        StartReach(roattr, route);
+        return StartReach(roattr, route);
     } else {
-        StartUnreach(route);
+        return StartUnreach(route);
     }
 }
 
@@ -205,9 +222,12 @@ const uint8_t *BgpMessage::GetData(IPeerUpdate *ipeer_update, size_t *lenp) {
 
 Message *BgpMessageBuilder::Create(const BgpTable *table,
         const RibOutAttr *roattr, const BgpRoute *route) const {
-    BgpMessage *msg = new BgpMessage();
-    msg->Start(roattr, route);
-    return msg;
+    auto_ptr<BgpMessage> msg(new BgpMessage(table));
+    if (msg->Start(roattr, route)) {
+        return msg.release();
+    } else {
+        return NULL;
+    }
 }
 
 BgpMessageBuilder BgpMessageBuilder::instance_;

src/bgp/bgp_message_builder.h

@@ -10,21 +10,23 @@
 
 class BgpMessage : public Message {
 public:
-    BgpMessage();
+    BgpMessage(const BgpTable *table = NULL);
     virtual ~BgpMessage();
-    void Start(const RibOutAttr *roattr, const BgpRoute *route);
+    bool Start(const RibOutAttr *roattr, const BgpRoute *route);
     virtual bool AddRoute(const BgpRoute *route, const RibOutAttr *roattr);
     virtual void Finish();
     virtual const uint8_t *GetData(IPeerUpdate *ipeer_update, size_t *lenp);
 
 private:
-    void StartReach(const RibOutAttr *roattr, const BgpRoute *route);
-    void StartUnreach(const BgpRoute *route);
+    bool StartReach(const RibOutAttr *roattr, const BgpRoute *route);
+    bool StartUnreach(const BgpRoute *route);
     bool UpdateLength(const char *tag, int size, int delta);
 
+    const BgpTable *table_;
     EncodeOffsets encode_offsets_;
     uint8_t data_[BgpProto::kMaxMessageSize];
     size_t datalen_;
+
     DISALLOW_COPY_AND_ASSIGN(BgpMessage);
 };
 

src/bgp/bgp_ribout_updates.cc

@@ -105,20 +105,31 @@ bool RibOutUpdates::DequeueCommon(UpdateMarker *marker, RouteUpdate *rt_update,
             continue;
         }
 
-        // Generate the update and merge additional updates into that message.
+        // Generate the update, merge additional updates into that message and
+        // send it message to the target RibPeerSet.
+        //
+        // In the rare case that the first route and it's attributes don't fit
+        // into the message, clear the target bits in the UpdateInfo to ensure
+        // that the UpdateQueue doesn't get wedged. However, don't update the
+        // history bits in the RouteUpdate since the message did not get sent.
+        //
+        // The Create routine has the responsibility of logging an error and
+        // incrementing any counters.
+        RibPeerSet msg_blocked;
+        bool msg_sent = false;
         auto_ptr<Message> message(
             builder_->Create(table, &uinfo->roattr, rt_update->route()));
-        UpdatePack(rt_update->queue_id(), message.get(), uinfo, msgset);
-        message->Finish();
-
-        // Send the message to the target RibPeerSet.
-        RibPeerSet msg_blocked;
-        UpdateSend(message.get(), msgset, &msg_blocked);
+        if (message.get() != NULL) {
+            UpdatePack(rt_update->queue_id(), message.get(), uinfo, msgset);
+            message->Finish();
+            UpdateSend(message.get(), msgset, &msg_blocked);
+            msg_sent = true;
+        }
 
         // Reset bits in the UpdateInfo.  Note that this has already been done
         // via UpdatePack for all the other UpdateInfo elements that we packed
         // into this message.
-        bool empty = ClearAdvertisedBits(rt_update, uinfo, msgset);
+        bool empty = ClearAdvertisedBits(rt_update, uinfo, msgset, msg_sent);
         if (empty) {
             rt_update->RemoveUpdateInfo(uinfo);
         }
@@ -371,7 +382,7 @@ void RibOutUpdates::UpdatePack(int queue_id, Message *message,
         //
         // If the RouteUpdate itself is now empty i.e. there are no more
         // UpdateInfo elements associated with it, we can get rid of it.
-        bool empty = ClearAdvertisedBits(update.get(), uinfo, msgset);
+        bool empty = ClearAdvertisedBits(update.get(), uinfo, msgset, true);
         if (empty && update->RemoveUpdateInfo(uinfo)) {
             ClearUpdate(&update);
         }
@@ -499,11 +510,13 @@ void RibOutUpdates::ClearUpdate(RouteUpdatePtr *update) {
 // Return true if the UpdateInfo was removed from the set container.
 //
 bool RibOutUpdates::ClearAdvertisedBits(RouteUpdate *rt_update,
-        UpdateInfo *uinfo, const RibPeerSet &isect) {
+        UpdateInfo *uinfo, const RibPeerSet &isect, bool update_history) {
     CHECK_CONCURRENCY("bgp::SendTask");
 
+    if (update_history) {
+        rt_update->UpdateHistory(ribout_, &uinfo->roattr, isect);
+    }
     uinfo->target.Reset(isect);
-    rt_update->UpdateHistory(ribout_, &uinfo->roattr, isect);
     bool empty = uinfo->target.empty();
     if (empty) {
         UpdateQueue *queue = queue_vec_[rt_update->queue_id()];

src/bgp/bgp_ribout_updates.h

@@ -84,8 +84,8 @@ class RibOutUpdates {
     // Remove the advertised bits on an update. This updates the history
     // information. Returns true if the UpdateInfo should be deleted.
     bool ClearAdvertisedBits(RouteUpdate *rt_update, UpdateInfo *uinfo,
-                             const RibPeerSet &bits);
-    
+                             const RibPeerSet &bits, bool update_history);
+
     void StoreHistory(RouteUpdate *rt_update);
     void ClearState(RouteUpdate *rt_update);
     void ClearUpdate(RouteUpdatePtr *update);

src/bgp/bgp_server.h

@@ -123,6 +123,9 @@ class BgpServer {
     LifetimeActor *deleter();
     boost::asio::io_service *ioservice();
 
+    void increment_message_build_error() const { ++message_build_error_; }
+    uint64_t message_build_error() const { return message_build_error_; }
+
     int RegisterASNUpdateCallback(ASNUpdateCb callback);
     void UnregisterASNUpdateCallback(int listener);
     void NotifyASNUpdate(as_t old_asn);
@@ -182,6 +185,8 @@ class BgpServer {
     boost::scoped_ptr<BgpConfigManager> config_mgr_;
     boost::scoped_ptr<ConfigUpdater> updater_;
 
+    mutable uint64_t message_build_error_;
+
     DISALLOW_COPY_AND_ASSIGN(BgpServer);
 };