[bgpd] Fix peer prefix counts and make it slightly more robust

2006-02-05 Paul Jakma <paul.jakma@sun.com>

	* bgp_route.h: Add BGP_INFO_COUNTED to track whether
	  prefix has been counted or not.
	* bgp_route.c: (bgp_pcount_{inc,dec}rement) new helpers, to
	  centralise inc/dec of prefix-count,
	  (bgp_rib_remove) Remove pcount decrement, use helper.
	  (bgp_rib_withdraw) ditto, additionally use previous function
	  too.
	  (bgp_update_main) Use pcount helpers.
	  (bgp_clear_route_node) ditto, aslo REMOVED routes don't need
	  clearing.

diff --git a/bgpd/ChangeLog b/bgpd/ChangeLog
index 714de1d30151123d1ffc9300086f162c116e15c3..8e2a397dcdab6c1ce96e61f255fc05135b4f5397 100644 (file)
--- a/bgpd/ChangeLog
+++ b/bgpd/ChangeLog
@@ -1,3 +1,16 @@
+2006-02-05 Paul Jakma <paul.jakma@sun.com>
+
+       * bgp_route.h: Add BGP_INFO_COUNTED to track whether
+         prefix has been counted or not.
+       * bgp_route.c: (bgp_pcount_{inc,dec}rement) new helpers, to
+         centralise inc/dec of prefix-count, 
+         (bgp_rib_remove) Remove pcount decrement, use helper.
+         (bgp_rib_withdraw) ditto, additionally use previous function
+         too.
+         (bgp_update_main) Use pcount helpers.
+         (bgp_clear_route_node) ditto, aslo REMOVED routes don't need
+         clearing.
+ 
 2006-02-02 Paul Jakma <paul.jakma@sun.com>
 
        * bgp_route.c: (bgp_{clear_node,process}_queue_init) delay

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 3d9856b9910818c8ba710d5cdbdf222c7ff7879b..ba6412ee7ad8adbdcb6dcec8362cd07c764ff55c 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -1554,6 +1554,43 @@ bgp_maximum_prefix_overflow (struct peer *peer, afi_t afi,
   return 0;
 }
 
+static void
+bgp_pcount_increment (struct bgp_node *rn, struct bgp_info *ri, 
+                      afi_t afi, safi_t safi)
+{
+  assert (ri && rn);
+  
+  if (!CHECK_FLAG (ri->flags, BGP_INFO_COUNTED)
+      && rn->table->type == BGP_TABLE_MAIN)
+    {
+      SET_FLAG (ri->flags, BGP_INFO_COUNTED);
+      ri->peer->pcount[afi][safi]++;
+    }
+}
+    
+static void
+bgp_pcount_decrement (struct bgp_node *rn, struct bgp_info *ri,
+                      afi_t afi, safi_t safi)
+{
+  /* Ignore 'pcount' for RS-client tables */
+  if (CHECK_FLAG (ri->flags, BGP_INFO_COUNTED)
+      && rn->table->type == BGP_TABLE_MAIN)
+    {
+      UNSET_FLAG (ri->flags, BGP_INFO_COUNTED);
+      
+      /* slight hack, but more robust against errors. */
+      if (ri->peer->pcount[afi][safi])
+        ri->peer->pcount[afi][safi]--;
+      else
+        {
+          zlog_warn ("%s: Asked to decrement 0 prefix count for peer %s",
+                     __func__, ri->peer->host);
+          zlog_backtrace (LOG_WARNING);
+          zlog_warn ("%s: Please report to Quagga bugzilla", __func__);
+        }      
+    }
+}
+
 /* Unconditionally remove the route from the RIB, without taking
  * damping into consideration (eg, because the session went down)
  */
@@ -1561,18 +1598,13 @@ static void
 bgp_rib_remove (struct bgp_node *rn, struct bgp_info *ri, struct peer *peer,
                afi_t afi, safi_t safi)
 {
-  if (!CHECK_FLAG (ri->flags, BGP_INFO_HISTORY)
-      && rn->table->type == BGP_TABLE_MAIN)
-    {
-      /* Ignore 'pcount' for RS-client tables */
-      if ( rn->table->type == BGP_TABLE_MAIN)
-        {
-          peer->pcount[afi][safi]--;
-          bgp_aggregate_decrement (peer->bgp, &rn->p, ri, afi, safi);
-        }
-    }
+  bgp_pcount_decrement (rn, ri, afi, safi);
+  bgp_aggregate_decrement (peer->bgp, &rn->p, ri, afi, safi);
+  
+  if (!CHECK_FLAG (ri->flags, BGP_INFO_HISTORY))
+    bgp_info_delete (rn, ri); /* keep historical info */
+    
   bgp_process (peer->bgp, rn, afi, safi);
-  bgp_info_delete (rn, ri);
 }
 
 static void
@@ -1581,17 +1613,6 @@ bgp_rib_withdraw (struct bgp_node *rn, struct bgp_info *ri, struct peer *peer,
 {
   int status = BGP_DAMP_NONE;
 
-  if (!CHECK_FLAG (ri->flags, BGP_INFO_HISTORY)
-      && rn->table->type == BGP_TABLE_MAIN)
-    {
-      /* Ignore 'pcount' for RS-client tables */
-      if ( rn->table->type == BGP_TABLE_MAIN)
-        {
-          peer->pcount[afi][safi]--;
-          bgp_aggregate_decrement (peer->bgp, &rn->p, ri, afi, safi);
-        }
-    }
-  
   /* apply dampening, if result is suppressed, we'll be retaining 
    * the bgp_info in the RIB for historical reference.
    */
@@ -1599,12 +1620,13 @@ bgp_rib_withdraw (struct bgp_node *rn, struct bgp_info *ri, struct peer *peer,
       && peer_sort (peer) == BGP_PEER_EBGP)
     if ( (status = bgp_damp_withdraw (ri, rn, afi, safi, 0)) 
          == BGP_DAMP_SUPPRESSED)
-      return;
-
-  bgp_process (peer->bgp, rn, afi, safi);
-
-  if (status != BGP_DAMP_USED)
-    bgp_info_delete (rn, ri);
+      {
+        bgp_pcount_decrement (rn, ri, afi, safi);
+        bgp_aggregate_decrement (peer->bgp, &rn->p, ri, afi, safi);
+        return;
+      }
+    
+  bgp_rib_remove (rn, ri, peer, afi, safi);
 }
 
 static void
@@ -1952,13 +1974,13 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
                  inet_ntop(p->family, &p->u.prefix, buf, SU_ADDRSTRLEN),
                  p->prefixlen);
 
-             peer->pcount[afi][safi]++;
-             ret = bgp_damp_update (ri, rn, afi, safi);
-             if (ret != BGP_DAMP_SUPPRESSED)
-               {
-                 bgp_aggregate_increment (bgp, p, ri, afi, safi);
-                 bgp_process (bgp, rn, afi, safi);
-               }
+             bgp_pcount_increment (rn, ri, afi, safi);
+             
+             if (bgp_damp_update (ri, rn, afi, safi) != BGP_DAMP_SUPPRESSED)
+               {
+                  bgp_aggregate_increment (bgp, p, ri, afi, safi);
+                  bgp_process (bgp, rn, afi, safi);
+                }
            }
          else
            {
@@ -1973,7 +1995,8 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
              if (CHECK_FLAG (ri->flags, BGP_INFO_STALE))
                {
                  UNSET_FLAG (ri->flags, BGP_INFO_STALE);
-                 peer->pcount[afi][safi]++;
+                 bgp_pcount_increment (rn, ri, afi, safi);
+                 bgp_process (bgp, rn, afi, safi);
                }
            }
 
@@ -1991,14 +2014,17 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
 
       /* graceful restart STALE flag unset. */
       if (CHECK_FLAG (ri->flags, BGP_INFO_STALE))
-       {
-         UNSET_FLAG (ri->flags, BGP_INFO_STALE);
-         peer->pcount[afi][safi]++;
-       }
+       UNSET_FLAG (ri->flags, BGP_INFO_STALE);
 
       /* The attribute is changed. */
       SET_FLAG (ri->flags, BGP_INFO_ATTR_CHANGED);
-
+      
+      /* implicit withdraw, decrement aggregate and pcount here.
+       * only if update is accepted, they'll increment below.
+       */
+      bgp_pcount_decrement (rn, ri, afi, safi);
+      bgp_aggregate_decrement (bgp, p, ri, afi, safi);
+      
       /* Update bgp route dampening information.  */
       if (CHECK_FLAG (bgp->af_flags[afi][safi], BGP_CONFIG_DAMPENING)
          && peer_sort (peer) == BGP_PEER_EBGP)
@@ -2007,12 +2033,8 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
             information.  */
          if (! CHECK_FLAG (ri->flags, BGP_INFO_HISTORY))
            bgp_damp_withdraw (ri, rn, afi, safi, 1);  
-         else
-           peer->pcount[afi][safi]++;
        }
        
-      bgp_aggregate_decrement (bgp, p, ri, afi, safi);
-
       /* Update to new attribute.  */
       bgp_attr_unintern (ri->attr);
       ri->attr = attr_new;
@@ -2050,6 +2072,7 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
        SET_FLAG (ri->flags, BGP_INFO_VALID);
 
       /* Process change. */
+      bgp_pcount_increment (rn, ri, afi, safi);
       bgp_aggregate_increment (bgp, p, ri, afi, safi);
 
       bgp_process (bgp, rn, afi, safi);
@@ -2066,9 +2089,6 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
            p->prefixlen);
     }
 
-  /* Increment prefix counter */
-  peer->pcount[afi][safi]++;
-
   /* Make new BGP info. */
   new = bgp_info_new ();
   new->type = type;
@@ -2096,7 +2116,8 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
   else
     SET_FLAG (new->flags, BGP_INFO_VALID);
 
-  /* Aggregate address increment. */
+  /* Increment prefix */
+  bgp_pcount_increment (rn, new, afi, safi);
   bgp_aggregate_increment (bgp, p, new, afi, safi);
   
   /* Register new BGP information. */
@@ -2469,10 +2490,11 @@ bgp_clear_route_node (struct work_queue *wq, void *data)
             && cq->peer->nsf[cq->afi][cq->safi]
             && ! CHECK_FLAG (ri->flags, BGP_INFO_STALE)
             && ! CHECK_FLAG (ri->flags, BGP_INFO_HISTORY)
-            && ! CHECK_FLAG (ri->flags, BGP_INFO_DAMPED))
+            && ! CHECK_FLAG (ri->flags, BGP_INFO_DAMPED)
+            && ! CHECK_FLAG (ri->flags, BGP_INFO_REMOVED))
           {
+            bgp_pcount_decrement (cq->rn, ri, cq->afi, cq->safi);
             SET_FLAG (ri->flags, BGP_INFO_STALE);
-            cq->peer->pcount[cq->afi][cq->safi]--;
           }
         else
           bgp_rib_remove (cq->rn, ri, cq->peer, cq->afi, cq->safi);

diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
index aa2c59ec9d95a91ff852ad1261244fd32e765686..24be30ff2d0c1b053c8fe782060d187222cb9f70 100644 (file)
--- a/bgpd/bgp_route.h
+++ b/bgpd/bgp_route.h
@@ -55,6 +55,7 @@ struct bgp_info
 #define BGP_INFO_DMED_SELECTED  (1 << 7)
 #define BGP_INFO_STALE          (1 << 8)
 #define BGP_INFO_REMOVED        (1 << 9)
+#define BGP_INFO_COUNTED       (1 << 10)
 
   /* Peer structure.  */
   struct peer *peer;