]> git.proxmox.com Git - mirror_frr.git/commitdiff
[bgpd] Fix 0.99 shutdown regression, introduce Clearing and Deleted states
authorPaul Jakma <paul.jakma@sun.com>
Thu, 14 Sep 2006 02:58:49 +0000 (02:58 +0000)
committerPaul Jakma <paul.jakma@sun.com>
Thu, 14 Sep 2006 02:58:49 +0000 (02:58 +0000)
2006-09-14 Paul Jakma <paul.jakma@sun.com>

* (general) Fix some niggly issues around 'shutdown' and clearing
  by adding a Clearing FSM wait-state and a hidden 'Deleted'
  FSM state, to allow deleted peers to 'cool off' and hit 0
  references. This introduces a slow memory leak of struct peer,
  however that's more a testament to the fragility of the
  reference counting than a bug in this patch, cleanup of
  reference counting to fix this is to follow.
* bgpd.h: Add Clearing, Deleted states and Clearing_Completed
  and event.
* bgp_debug.c: (bgp_status_msg[]) Add strings for Clearing and
  Deleted.
* bgp_fsm.h: Don't allow timer/event threads to set anything
  for Deleted peers.
* bgp_fsm.c: (bgp_timer_set) Add Clearing and Deleted. Deleted
  needs to stop everything.
  (bgp_stop) Remove explicit fsm_change_status call, the
  general framework handles the transition.
  (bgp_start) Log a warning if a start is attempted on a peer
  that should stay down, trying to start a peer.
  (struct .. FSM) Add Clearing_Completed
  events, has little influence except when in state
  Clearing to signal wait-state can end.
  Add Clearing and Deleted states, former is a wait-state,
  latter is a placeholder state to allow peers to disappear
  quietly once refcounts settle.
  (bgp_event) Try reduce verbosity of FSM state-change debug,
  changes to same state are not interesting (Established->Established)
  Allow NULL action functions in FSM.
* bgp_packet.c: (bgp_write) Use FSM events, rather than trying
  to twiddle directly with FSM state behind the back of FSM.
  (bgp_write_notify) ditto.
  (bgp_read) Remove the vague ACCEPT_PEER peer_unlock, or else
  this patch crashes, now it leaks instead.
* bgp_route.c: (bgp_clear_node_complete) Clearing_Completed
  event, to end clearing.
  (bgp_clear_route) See extensive comments.
* bgpd.c: (peer_free) should only be called while in Deleted,
  peer refcounting controls when peer_free is called.
  bgp_sync_delete should be here, not in peer_delete.
  (peer_delete) Initiate delete.
  Transition to Deleted state manually.
  When removing peer from indices that provide visibility of it,
  take great care to be idempotent wrt the reference counting
  of struct peer through those indices.
  Use bgp_timer_set, rather than replicating.
  Call to bgp_sync_delete isn't appropriate here, sync can be
  referenced while shutting down and finishing deletion.
  (peer_group_bind) Take care to be idempotent wrt list references
  indexing peers.

bgpd/ChangeLog
bgpd/bgp_debug.c
bgpd/bgp_fsm.c
bgpd/bgp_fsm.h
bgpd/bgp_packet.c
bgpd/bgp_route.c
bgpd/bgp_vty.c
bgpd/bgpd.c
bgpd/bgpd.h

index 482beedaa17ccb2da4a57bb5f816c7ab189eda14..02aaf3abfc84a22b529d6281d0268c7837ec0f5a 100644 (file)
@@ -1,3 +1,55 @@
+2006-09-14 Paul Jakma <paul.jakma@sun.com>
+
+       * (general) Fix some niggly issues around 'shutdown' and clearing
+         by adding a Clearing FSM wait-state and a hidden 'Deleted'
+         FSM state, to allow deleted peers to 'cool off' and hit 0
+         references. This introduces a slow memory leak of struct peer,
+         however that's more a testament to the fragility of the
+         reference counting than a bug in this patch, cleanup of
+         reference counting to fix this is to follow.
+       * bgpd.h: Add Clearing, Deleted states and Clearing_Completed
+         and event.
+       * bgp_debug.c: (bgp_status_msg[]) Add strings for Clearing and
+         Deleted.
+       * bgp_fsm.h: Don't allow timer/event threads to set anything
+         for Deleted peers.
+       * bgp_fsm.c: (bgp_timer_set) Add Clearing and Deleted. Deleted
+         needs to stop everything.
+         (bgp_stop) Remove explicit fsm_change_status call, the
+         general framework handles the transition.
+         (bgp_start) Log a warning if a start is attempted on a peer
+         that should stay down, trying to start a peer.
+         (struct .. FSM) Add Clearing_Completed
+         events, has little influence except when in state
+         Clearing to signal wait-state can end.
+         Add Clearing and Deleted states, former is a wait-state,
+         latter is a placeholder state to allow peers to disappear
+         quietly once refcounts settle.
+         (bgp_event) Try reduce verbosity of FSM state-change debug, 
+         changes to same state are not interesting (Established->Established)
+         Allow NULL action functions in FSM.
+       * bgp_packet.c: (bgp_write) Use FSM events, rather than trying
+         to twiddle directly with FSM state behind the back of FSM.
+         (bgp_write_notify) ditto.
+         (bgp_read) Remove the vague ACCEPT_PEER peer_unlock, or else
+         this patch crashes, now it leaks instead.
+       * bgp_route.c: (bgp_clear_node_complete) Clearing_Completed
+         event, to end clearing.
+         (bgp_clear_route) See extensive comments.
+       * bgpd.c: (peer_free) should only be called while in Deleted,
+         peer refcounting controls when peer_free is called.
+         bgp_sync_delete should be here, not in peer_delete.
+         (peer_delete) Initiate delete. 
+         Transition to Deleted state manually.
+         When removing peer from indices that provide visibility of it,
+         take great care to be idempotent wrt the reference counting
+         of struct peer through those indices.
+         Use bgp_timer_set, rather than replicating.
+         Call to bgp_sync_delete isn't appropriate here, sync can be
+         referenced while shutting down and finishing deletion.
+         (peer_group_bind) Take care to be idempotent wrt list references
+         indexing peers.
+
 2006-09-13 Paul Jakma <paul.jakma@sun.com>
 
        * bgp_aspath.c: (aspath_highest) new, return highest ASN in an
index 1b398ee8b1b681334809827701c99cb2d18169dd..1e0fcd1fd333a0bba28437275fec1b1461df206e 100644 (file)
@@ -62,6 +62,8 @@ struct message bgp_status_msg[] =
   { OpenSent, "OpenSent" },
   { OpenConfirm, "OpenConfirm" },
   { Established, "Established" },
+  { Clearing,    "Clearing"    },
+  { Deleted,     "Deleted"     },
 };
 int bgp_status_msg_max = BGP_STATUS_MAX;
 
index 770a7911960a8535140f61450b5092b6d9aea81e..bdb6517fdcc479af1cf30f32d3b6f3feff2c5617 100644 (file)
@@ -68,6 +68,11 @@ bgp_start_jitter (int time)
   return ((rand () % (time + 1)) - (time / 2));
 }
 
+/* Check if suppress start/restart of sessions to peer. */
+#define BGP_PEER_START_SUPPRESSED(P) \
+  (CHECK_FLAG ((P)->flags, PEER_FLAG_SHUTDOWN) \
+   || CHECK_FLAG ((P)->sflags, PEER_STATUS_PREFIX_OVERFLOW))
+
 /* Hook function called after bgp event is occered.  And vty's
    neighbor command invoke this function after making neighbor
    structure. */
@@ -82,10 +87,7 @@ bgp_timer_set (struct peer *peer)
       /* First entry point of peer's finite state machine.  In Idle
         status start timer is on unless peer is shutdown or peer is
         inactive.  All other timer must be turned off */
-      if (CHECK_FLAG (peer->flags, PEER_FLAG_SHUTDOWN)
-         || CHECK_FLAG (peer->sflags, PEER_STATUS_PREFIX_OVERFLOW)
-         || CHECK_FLAG (peer->sflags, PEER_STATUS_CLEARING)
-         || ! peer_active (peer))
+      if (BGP_PEER_START_SUPPRESSED (peer) || ! peer_active (peer))
        {
          BGP_TIMER_OFF (peer->t_start);
        }
@@ -197,6 +199,17 @@ bgp_timer_set (struct peer *peer)
        }
       BGP_TIMER_OFF (peer->t_asorig);
       break;
+    case Deleted:
+      BGP_TIMER_OFF (peer->t_gr_restart);
+      BGP_TIMER_OFF (peer->t_gr_stale);
+      BGP_TIMER_OFF (peer->t_pmax_restart);
+    case Clearing:
+      BGP_TIMER_OFF (peer->t_start);
+      BGP_TIMER_OFF (peer->t_connect);
+      BGP_TIMER_OFF (peer->t_holdtime);
+      BGP_TIMER_OFF (peer->t_keepalive);
+      BGP_TIMER_OFF (peer->t_asorig);
+      BGP_TIMER_OFF (peer->t_routeadv);
     }
 }
 
@@ -420,7 +433,6 @@ bgp_stop (struct peer *peer)
   if (peer->status == Established)
     {
       peer->dropped++;
-      bgp_fsm_change_status (peer, Idle);
 
       /* bgp log-neighbor-changes of neighbor Down */
       if (bgp_flag_check (peer->bgp, BGP_FLAG_LOG_NEIGHBOR_CHANGES))
@@ -625,6 +637,14 @@ bgp_start (struct peer *peer)
 {
   int status;
 
+  if (BGP_PEER_START_SUPPRESSED (peer))
+    {
+      if (BGP_DEBUG (fsm, FSM))
+        plog_err (peer->log, "%s [FSM] Trying to start suppressed peer"
+                  " - this is never supposed to happen!", peer->host);
+      return -1;
+    }
+
   /* Scrub some information that might be left over from a previous,
    * session
    */
@@ -903,6 +923,7 @@ struct {
     {bgp_ignore, Idle},                /* Receive_KEEPALIVE_message    */
     {bgp_ignore, Idle},                /* Receive_UPDATE_message       */
     {bgp_ignore, Idle},                /* Receive_NOTIFICATION_message */
+    {bgp_ignore, Idle},         /* Clearing_Completed           */
   },
   {
     /* Connect */
@@ -919,6 +940,7 @@ struct {
     {bgp_ignore,  Idle},       /* Receive_KEEPALIVE_message    */
     {bgp_ignore,  Idle},       /* Receive_UPDATE_message       */
     {bgp_stop,    Idle},       /* Receive_NOTIFICATION_message */
+    {bgp_ignore,  Idle},         /* Clearing_Completed           */
   },
   {
     /* Active, */
@@ -935,6 +957,7 @@ struct {
     {bgp_ignore,  Idle},       /* Receive_KEEPALIVE_message    */
     {bgp_ignore,  Idle},       /* Receive_UPDATE_message       */
     {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
+    {bgp_ignore, Idle},         /* Clearing_Completed           */
   },
   {
     /* OpenSent, */
@@ -951,6 +974,7 @@ struct {
     {bgp_ignore,  Idle},       /* Receive_KEEPALIVE_message    */
     {bgp_ignore,  Idle},       /* Receive_UPDATE_message       */
     {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
+    {bgp_ignore, Idle},         /* Clearing_Completed           */
   },
   {
     /* OpenConfirm, */
@@ -967,22 +991,58 @@ struct {
     {bgp_establish, Established}, /* Receive_KEEPALIVE_message    */
     {bgp_ignore,  Idle},       /* Receive_UPDATE_message       */
     {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
+    {bgp_ignore, Idle},         /* Clearing_Completed           */
   },
   {
     /* Established, */
-    {bgp_ignore,  Established},        /* BGP_Start                    */
-    {bgp_stop,    Idle},       /* BGP_Stop                     */
-    {bgp_stop,    Idle},       /* TCP_connection_open          */
-    {bgp_stop,    Idle},       /* TCP_connection_closed        */
-    {bgp_ignore,  Idle},       /* TCP_connection_open_failed   */
-    {bgp_stop,    Idle},       /* TCP_fatal_error              */
-    {bgp_ignore,  Idle},       /* ConnectRetry_timer_expired   */
-    {bgp_fsm_holdtime_expire, Idle}, /* Hold_Timer_expired           */
+    {bgp_ignore,               Established}, /* BGP_Start                    */
+    {bgp_stop,                    Clearing}, /* BGP_Stop                     */
+    {bgp_stop,                    Clearing}, /* TCP_connection_open          */
+    {bgp_stop,                    Clearing}, /* TCP_connection_closed        */
+    {bgp_ignore,                  Clearing}, /* TCP_connection_open_failed   */
+    {bgp_stop,                    Clearing}, /* TCP_fatal_error              */
+    {bgp_ignore,                  Clearing}, /* ConnectRetry_timer_expired   */
+    {bgp_fsm_holdtime_expire,     Clearing}, /* Hold_Timer_expired           */
     {bgp_fsm_keepalive_expire, Established}, /* KeepAlive_timer_expired      */
-    {bgp_stop, Idle},          /* Receive_OPEN_message         */
-    {bgp_fsm_keepalive, Established}, /* Receive_KEEPALIVE_message    */
-    {bgp_fsm_update,   Established}, /* Receive_UPDATE_message       */
-    {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
+    {bgp_stop,                    Clearing}, /* Receive_OPEN_message         */
+    {bgp_fsm_keepalive,        Established}, /* Receive_KEEPALIVE_message    */
+    {bgp_fsm_update,           Established}, /* Receive_UPDATE_message       */
+    {bgp_stop_with_error,         Clearing}, /* Receive_NOTIFICATION_message */
+    {bgp_ignore,                      Idle}, /* Clearing_Completed           */
+  },
+  {
+    /* Clearing, */
+    {bgp_ignore,  Clearing},   /* BGP_Start                    */
+    {bgp_ignore,  Clearing},   /* BGP_Stop                     */
+    {bgp_ignore,  Clearing},   /* TCP_connection_open          */
+    {bgp_ignore,  Clearing},   /* TCP_connection_closed        */
+    {bgp_ignore,  Clearing},   /* TCP_connection_open_failed   */
+    {bgp_ignore,  Clearing},   /* TCP_fatal_error              */
+    {bgp_ignore,  Clearing},   /* ConnectRetry_timer_expired   */
+    {bgp_ignore,  Clearing},   /* Hold_Timer_expired           */
+    {bgp_ignore,  Clearing},   /* KeepAlive_timer_expired      */
+    {bgp_ignore,  Clearing},   /* Receive_OPEN_message         */
+    {bgp_ignore,  Clearing},   /* Receive_KEEPALIVE_message    */
+    {bgp_ignore,  Clearing},   /* Receive_UPDATE_message       */
+    {bgp_ignore,  Clearing},   /* Receive_NOTIFICATION_message */
+    {bgp_ignore,  Idle    },   /* Clearing_Completed           */
+  },
+  {
+    /* Deleted, */
+    {bgp_ignore,  Deleted},    /* BGP_Start                    */
+    {bgp_ignore,  Deleted},    /* BGP_Stop                     */
+    {bgp_ignore,  Deleted},    /* TCP_connection_open          */
+    {bgp_ignore,  Deleted},    /* TCP_connection_closed        */
+    {bgp_ignore,  Deleted},    /* TCP_connection_open_failed   */
+    {bgp_ignore,  Deleted},    /* TCP_fatal_error              */
+    {bgp_ignore,  Deleted},    /* ConnectRetry_timer_expired   */
+    {bgp_ignore,  Deleted},    /* Hold_Timer_expired           */
+    {bgp_ignore,  Deleted},    /* KeepAlive_timer_expired      */
+    {bgp_ignore,  Deleted},    /* Receive_OPEN_message         */
+    {bgp_ignore,  Deleted},    /* Receive_KEEPALIVE_message    */
+    {bgp_ignore,  Deleted},    /* Receive_UPDATE_message       */
+    {bgp_ignore,  Deleted},    /* Receive_NOTIFICATION_message */
+    {bgp_ignore,  Deleted},    /* Clearing_Completed           */
   },
 };
 
@@ -1001,14 +1061,15 @@ static const char *bgp_event_str[] =
   "Receive_OPEN_message",
   "Receive_KEEPALIVE_message",
   "Receive_UPDATE_message",
-  "Receive_NOTIFICATION_message"
+  "Receive_NOTIFICATION_message",
+  "Clearing_Completed",
 };
 
 /* Execute event process. */
 int
 bgp_event (struct thread *thread)
 {
-  int ret;
+  int ret = 0;
   int event;
   int next;
   struct peer *peer;
@@ -1019,14 +1080,15 @@ bgp_event (struct thread *thread)
   /* Logging this event. */
   next = FSM [peer->status -1][event - 1].next_state;
 
-  if (BGP_DEBUG (fsm, FSM))
+  if (BGP_DEBUG (fsm, FSM) && peer->status != next)
     plog_debug (peer->log, "%s [FSM] %s (%s->%s)", peer->host, 
               bgp_event_str[event],
               LOOKUP (bgp_status_msg, peer->status),
               LOOKUP (bgp_status_msg, next));
 
   /* Call function. */
-  ret = (*(FSM [peer->status - 1][event - 1].func))(peer);
+  if (FSM [peer->status -1][event - 1].func)
+    ret = (*(FSM [peer->status - 1][event - 1].func))(peer);
 
   /* When function do not want proceed next job return -1. */
   if (ret >= 0)
index e90f3b433c97eba851cea200fe2602240cef635a..0a5d37157e441bdc9ecf0bb18a31227823a01804 100644 (file)
@@ -43,7 +43,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 
 #define BGP_WRITE_ON(T,F,V)                    \
   do {                                         \
-    if (!T)                                    \
+    if (!(T) && (peer->status != Deleted))     \
       {                                                \
         peer_lock (peer);                      \
         THREAD_WRITE_ON(master,(T),(F),peer,(V)); \
@@ -61,7 +61,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 
 #define BGP_TIMER_ON(T,F,V)                    \
   do {                                         \
-    if (!T)                                    \
+    if (!(T) && (peer->status != Deleted))     \
       {                                                \
         peer_lock (peer);                      \
         THREAD_TIMER_ON(master,(T),(F),peer,(V)); \
@@ -79,8 +79,11 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 
 #define BGP_EVENT_ADD(P,E)                     \
   do {                                         \
-    peer_lock (peer); /* bgp event reference */ \
-    thread_add_event (master, bgp_event, (P), (E)); \
+    if ((P)->status != Deleted)                        \
+      {                                                \
+        peer_lock (peer); /* bgp event reference */ \
+        thread_add_event (master, bgp_event, (P), (E)); \
+      }                                                \
   } while (0)
 
 #define BGP_EVENT_DELETE(P)                    \
@@ -90,6 +93,12 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
     thread_cancel_event (master, (P));                 \
   } while (0)
 
+#define BGP_EVENT_FLUSH_ADD(P,E)       \
+  do {                                 \
+    BGP_EVENT_DELETE(P);               \
+    BGP_EVENT_ADD(P,E);                        \
+  } while (0)
+
 /* Prototypes. */
 extern int bgp_event (struct thread *);
 extern int bgp_stop (struct peer *peer);
index 8b024a1c0fd14c1350457cce07c6d18cc0cacacd..da59d32983daef561a52adbc254d3d2f1aafefb5 100644 (file)
@@ -637,9 +637,7 @@ bgp_write (struct thread *thread)
          if (write_errno == EWOULDBLOCK || write_errno == EAGAIN)
              break;
 
-         BGP_EVENT_ADD (peer, BGP_Stop);
-         peer->status = Idle;
-         bgp_timer_set (peer);
+         BGP_EVENT_FLUSH_ADD (peer, TCP_fatal_error);
          return 0;
        }
       if (num != writenum)
@@ -673,10 +671,8 @@ bgp_write (struct thread *thread)
          if (peer->v_start >= (60 * 2))
            peer->v_start = (60 * 2);
 
-         BGP_EVENT_ADD (peer, BGP_Stop);
-         /*bgp_stop (peer);*/
-         peer->status = Idle;
-         bgp_timer_set (peer);
+         /* Flush any existing events */
+         BGP_EVENT_FLUSH_ADD (peer, BGP_Stop);
          return 0;
        case BGP_MSG_KEEPALIVE:
          peer->keepalive_out++;
@@ -721,9 +717,7 @@ bgp_write_notify (struct peer *peer)
   ret = writen (peer->fd, STREAM_DATA (s), stream_get_endp (s));
   if (ret <= 0)
     {
-      BGP_EVENT_ADD (peer, BGP_Stop);
-      peer->status = Idle;
-      bgp_timer_set (peer);
+      BGP_EVENT_FLUSH_ADD (peer, TCP_fatal_error);
       return 0;
     }
 
@@ -743,10 +737,7 @@ bgp_write_notify (struct peer *peer)
   if (peer->v_start >= (60 * 2))
     peer->v_start = (60 * 2);
 
-  /* We don't call event manager at here for avoiding other events. */
-  bgp_stop (peer);
-  peer->status = Idle;
-  bgp_timer_set (peer);
+  BGP_EVENT_FLUSH_ADD (peer, BGP_Stop);
 
   return 0;
 }
@@ -2375,14 +2366,6 @@ bgp_read (struct thread *thread)
       if (BGP_DEBUG (events, EVENTS))
        zlog_debug ("%s [Event] Accepting BGP peer delete", peer->host);
       peer_delete (peer);
-      /* we've lost track of a reference to ACCEPT_PEER somehow.  It doesnt
-       * _seem_ to be the 'update realpeer with accept peer' hack, yet it
-       * *must* be.. Very very odd, but I give up trying to
-       * root cause this - ACCEPT_PEER is a dirty hack, it should be fixed
-       * instead, which would make root-causing this a moot point..
-       * A hack because of a hack, appropriate.
-       */
-      peer_unlock (peer); /* god knows what reference... ACCEPT_PEER sucks */
     }
   return 0;
 }
index 2ce2ef4d81cfd8d31f28db1def0304da29bc38cf..5dde41de7a0258e032907905e12376375fbe114a 100644 (file)
@@ -2527,11 +2527,10 @@ bgp_clear_node_complete (struct work_queue *wq)
 {
   struct peer *peer = wq->spec.data;
   
-  UNSET_FLAG (peer->sflags, PEER_STATUS_CLEARING);
   peer_unlock (peer); /* bgp_clear_node_complete */
   
   /* Tickle FSM to start moving again */
-  BGP_EVENT_ADD (peer, BGP_Start);
+  BGP_EVENT_ADD (peer, Clearing_Completed);
 }
 
 static void
@@ -2593,9 +2592,10 @@ bgp_clear_route (struct peer *peer, afi_t afi, safi_t safi)
   if (peer->clear_node_queue == NULL)
     bgp_clear_node_queue_init (peer);
   
-  /* bgp_fsm.c will not bring CLEARING sessions out of Idle this
-   * protects against peers which flap faster than we can we clear,
-   * which could lead to:
+  /* bgp_fsm.c keeps sessions in state Clearing, not transitioning to
+   * Idle until it receives a Clearing_Completed event. This protects
+   * against peers which flap faster than we can we clear, which could
+   * lead to:
    *
    * a) race with routes from the new session being installed before
    *    clear_route_node visits the node (to delete the route of that
@@ -2604,11 +2604,8 @@ bgp_clear_route (struct peer *peer, afi_t afi, safi_t safi)
    *    on the process_main queue. Fast-flapping could cause that queue
    *    to grow and grow.
    */
-  if (!CHECK_FLAG (peer->sflags, PEER_STATUS_CLEARING))
-    {
-      SET_FLAG (peer->sflags, PEER_STATUS_CLEARING);
-      peer_lock (peer); /* bgp_clear_node_complete */
-    }
+  if (!peer->clear_node_queue->thread)
+    peer_lock (peer); /* bgp_clear_node_complete */
   
   if (safi != SAFI_MPLS_VPN)
     bgp_clear_route_table (peer, afi, safi, NULL, NULL);
@@ -2624,11 +2621,37 @@ bgp_clear_route (struct peer *peer, afi_t afi, safi_t safi)
         bgp_clear_route_table (peer, afi, safi, NULL, rsclient);
     }
   
-  /* If no routes were cleared, nothing was added to workqueue, run the
-   * completion function now.
+  /* If no routes were cleared, nothing was added to workqueue, the
+   * completion function won't be run by workqueue code - call it here.
+   *
+   * Additionally, there is a presumption in FSM that clearing is only
+   * needed if peer state is Established - peers in pre-Established states
+   * shouldn't have any route-update state associated with them (in or out).
+   *
+   * We still get here from FSM through bgp_stop->clear_route_all in
+   * pre-Established though, so this is a useful sanity check to ensure
+   * the assumption above holds.
+   *
+   * At some future point, this check could be move to the top of the
+   * function, and do a quick early-return when state is
+   * pre-Established, avoiding above list and table scans. Once we're
+   * sure it is safe..
    */
   if (!peer->clear_node_queue->thread)
     bgp_clear_node_complete (peer->clear_node_queue);
+  else
+    {
+      /* clearing queue scheduled. Normal if in Established state
+       * (and about to transition out of it), but otherwise...
+       */
+      if (peer->status != Established)
+        {
+          plog_err (peer->log, "%s [Error] State %s is not Established,"
+                    " but routes were cleared - bug!",
+                    peer->host, LOOKUP (bgp_status_msg, peer->status));
+          assert (peer->status == Established);
+        }
+    }
 }
   
 void
index ec4b6c224f211b1c05a368ebfb6d99fb671c33e2..b108164f3659850695fa73efd1ae419dc60ef7dd 100644 (file)
@@ -6693,8 +6693,6 @@ bgp_show_summary (struct vty *vty, struct bgp *bgp, int afi, int safi)
                vty_out (vty, " Idle (Admin)");
              else if (CHECK_FLAG (peer->sflags, PEER_STATUS_PREFIX_OVERFLOW))
                vty_out (vty, " Idle (PfxCt)");
-              else if (CHECK_FLAG (peer->sflags, PEER_STATUS_CLEARING))
-                vty_out (vty, " Idle (Clrng)");
              else
                vty_out (vty, " %-11s", LOOKUP(bgp_status_msg, peer->status));
            }
index 8ed598d282cd3431594e2fbddd0baa18693220d1..733b33a6aa42c0d7b6c9be4bfb82808d5ee0fb27 100644 (file)
@@ -687,6 +687,15 @@ peer_sort (struct peer *peer)
 static inline void
 peer_free (struct peer *peer)
 {
+  assert (peer->status == Deleted);
+  
+  /* this /ought/ to have been done already through bgp_stop earlier,
+   * but just to be sure.. 
+   */
+  bgp_timer_set (peer);
+  BGP_READ_OFF (peer->t_read);
+  BGP_WRITE_OFF (peer->t_write);
+  
   if (peer->desc)
     XFREE (MTYPE_PEER_DESC, peer->desc);
   
@@ -704,6 +713,7 @@ peer_free (struct peer *peer)
   if (peer->clear_node_queue)
     work_queue_free (peer->clear_node_queue);
   
+  bgp_sync_delete (peer);
   memset (peer, 0, sizeof (struct peer));
   
   XFREE (MTYPE_BGP_PEER, peer);
@@ -714,7 +724,8 @@ struct peer *
 peer_lock (struct peer *peer)
 {
   assert (peer && (peer->lock >= 0));
-  
+  assert (peer->status != Deleted);
+    
   peer->lock++;
   
   return peer;
@@ -761,18 +772,17 @@ peer_new ()
   struct servent *sp;
 
   /* Allocate new peer. */
-  peer = XMALLOC (MTYPE_BGP_PEER, sizeof (struct peer));
-  memset (peer, 0, sizeof (struct peer));
+  peer = XCALLOC (MTYPE_BGP_PEER, sizeof (struct peer));
 
   /* Set default value. */
   peer->fd = -1;
-  peer->lock = 1;
   peer->v_start = BGP_INIT_START_TIMER;
   peer->v_connect = BGP_DEFAULT_CONNECT_RETRY;
   peer->v_asorig = BGP_DEFAULT_ASORIGINATE;
   peer->status = Idle;
   peer->ostatus = Idle;
   peer->weight = 0;
+  peer = peer_lock (peer); /* initial reference */
 
   /* Set default flags.  */
   for (afi = AFI_IP; afi < AFI_MAX; afi++)
@@ -1139,7 +1149,17 @@ peer_nsf_stop (struct peer *peer)
   bgp_clear_route_all (peer);
 }
 
-/* Delete peer from confguration. */
+/* Delete peer from confguration.
+ *
+ * The peer is moved to a dead-end "Deleted" neighbour-state, to allow
+ * it to "cool off" and refcounts to hit 0, at which state it is freed.
+ *
+ * This function /should/ take care to be idempotent, to guard against
+ * it being called multiple times through stray events that come in
+ * that happen to result in this function being called again.  That
+ * said, getting here for a "Deleted" peer is a bug in the neighbour
+ * FSM.
+ */
 int
 peer_delete (struct peer *peer)
 {
@@ -1149,6 +1169,8 @@ peer_delete (struct peer *peer)
   struct bgp *bgp;
   struct bgp_filter *filter;
 
+  assert (peer->status != Deleted);
+  
   bgp = peer->bgp;
 
   if (CHECK_FLAG (peer->sflags, PEER_STATUS_NSF_WAIT))
@@ -1158,8 +1180,13 @@ peer_delete (struct peer *peer)
      relationship.  */
   if (peer->group)
     {
-      peer = peer_unlock (peer); /* peer-group reference */
-      listnode_delete (peer->group->peer, peer);
+      struct listnode *pn;
+
+      if ((pn = listnode_lookup (peer->group->peer, peer)))
+        {
+          peer = peer_unlock (peer); /* group->peer list reference */
+          list_delete_node (peer->group->peer, pn);
+        }
       peer->group = NULL;
     }
   
@@ -1169,29 +1196,25 @@ peer_delete (struct peer *peer)
    */
   peer->last_reset = PEER_DOWN_NEIGHBOR_DELETE;
   bgp_stop (peer);
-  bgp_fsm_change_status (peer, Idle); /* stops all timers */
+  bgp_fsm_change_status (peer, Deleted);
+  bgp_timer_set (peer); /* stops all timers for Deleted */
   
-  /* Stop all timers - should already have been done in bgp_stop */
-  BGP_TIMER_OFF (peer->t_start);
-  BGP_TIMER_OFF (peer->t_connect);
-  BGP_TIMER_OFF (peer->t_holdtime);
-  BGP_TIMER_OFF (peer->t_keepalive);
-  BGP_TIMER_OFF (peer->t_asorig);
-  BGP_TIMER_OFF (peer->t_routeadv);
-  BGP_TIMER_OFF (peer->t_pmax_restart);
-  BGP_TIMER_OFF (peer->t_gr_restart);
-  BGP_TIMER_OFF (peer->t_gr_stale);
-
   /* Delete from all peer list. */
   if (! CHECK_FLAG (peer->sflags, PEER_STATUS_GROUP))
     {
-      peer_unlock (peer); /* bgp peer list reference */
-      listnode_delete (bgp->peer, peer);
+      struct listnode *pn;
+      
+      if ((pn = listnode_lookup (bgp->peer, peer)))
+        {
+          peer_unlock (peer); /* bgp peer list reference */
+          list_delete_node (bgp->peer, pn);
+        }
       
-      if (peer_rsclient_active (peer))
+      if (peer_rsclient_active (peer)
+          && (pn = listnode_lookup (bgp->rsclient, peer)))
         {
           peer_unlock (peer); /* rsclient list reference */
-          listnode_delete (bgp->rsclient, peer);
+          list_delete_node (bgp->rsclient, pn);
         }
     }
 
@@ -1219,8 +1242,6 @@ peer_delete (struct peer *peer)
     sockunion_free (peer->su_remote);
   peer->su_local = peer->su_remote = NULL;
   
-  bgp_sync_delete (peer);
-
   /* Free filter related memory.  */
   for (afi = AFI_IP; afi < AFI_MAX; afi++)
     for (safi = SAFI_UNICAST; safi < SAFI_MAX; safi++)
@@ -1692,7 +1713,7 @@ peer_group_bind (struct bgp *bgp, union sockunion *su,
       peer->group = group;
       peer->af_group[afi][safi] = 1;
 
-      peer = peer_lock (peer); /* peer-group group->peer reference */
+      peer = peer_lock (peer); /* group->peer list reference */
       listnode_add (group->peer, peer);
       peer_group2peer_config_copy (group, peer, afi, safi);
 
@@ -1733,9 +1754,11 @@ peer_group_bind (struct bgp *bgp, union sockunion *su,
     {
       peer->group = group;
       
-      peer = peer_lock (peer); /* peer-group group->peer reference */
+      peer = peer_lock (peer); /* group->peer list reference */
       listnode_add (group->peer, peer);
     }
+  else
+    assert (group && peer->group == group);
 
   if (first_member)
     {
@@ -1759,13 +1782,16 @@ peer_group_bind (struct bgp *bgp, union sockunion *su,
 
   if (CHECK_FLAG(peer->af_flags[afi][safi], PEER_FLAG_RSERVER_CLIENT))
     {
+      struct listnode *pn;
+      
       /* If it's not configured as RSERVER_CLIENT in any other address
           family, without being member of a peer_group, remove it from
           list bgp->rsclient.*/
-      if (! peer_rsclient_active (peer))
+      if (! peer_rsclient_active (peer)
+          && (pn = listnode_lookup (bgp->rsclient, peer)))
         {
           peer_unlock (peer); /* peer rsclient reference */
-          listnode_delete (bgp->rsclient, peer);
+          list_delete_node (bgp->rsclient, pn);
         }
 
       bgp_table_finish (peer->rib[afi][safi]);
@@ -1821,6 +1847,7 @@ peer_group_unbind (struct bgp *bgp, struct peer *peer,
 
   if (! peer_group_active (peer))
     {
+      assert (listnode_lookup (group->peer, peer));
       peer_unlock (peer); /* peer group list reference */
       listnode_delete (group->peer, peer);
       peer->group = NULL;
index b8ae30aeb3a4a3c0da79285e558a12b5639519e9..8b180a43230f414b488c05f15886e87f50e3f6da 100644 (file)
@@ -386,7 +386,6 @@ struct peer
 #define PEER_STATUS_GROUP             (1 << 4) /* peer-group conf */
 #define PEER_STATUS_NSF_MODE          (1 << 5) /* NSF aware peer */
 #define PEER_STATUS_NSF_WAIT          (1 << 6) /* wait comeback peer */
-#define PEER_STATUS_CLEARING          (1 << 7) /* peers table being cleared */
 
   /* Peer status af flags (reset in bgp_stop) */
   u_int16_t af_sflags[AFI_MAX][SAFI_MAX];
@@ -662,7 +661,9 @@ struct bgp_nlri
 #define OpenSent                                 4
 #define OpenConfirm                              5
 #define Established                              6
-#define BGP_STATUS_MAX                           7
+#define Clearing                                 7
+#define Deleted                                  8
+#define BGP_STATUS_MAX                           9
 
 /* BGP finite state machine events.  */
 #define BGP_Start                                1
@@ -678,7 +679,8 @@ struct bgp_nlri
 #define Receive_KEEPALIVE_message               11
 #define Receive_UPDATE_message                  12
 #define Receive_NOTIFICATION_message            13
-#define BGP_EVENTS_MAX                          14
+#define Clearing_Completed                      14
+#define BGP_EVENTS_MAX                          15
 
 /* BGP timers default value.  */
 #define BGP_INIT_START_TIMER                     5