[ospfd] cleanup NSM neighbour delete through a new Deleted NSM state

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

	* ospf_nsm.h: Add a NSM_Deleted neighbour state, to act as dummy
	  state indicating the neighbour is to be deleted.
	* ospf_nsm.c: (general) Use the NSM_Deleted state to delete
	  neighbours, thus allowing code to be slightly more obvious
	  in its flow.
	  (nsm_timer_set) Add NSM_Deleted. Add another timer the code
	  missed.
	  (nsm_kill_nbr) No need for special case call to nsm_change_state
	  anymore.
	  Make the assert and error-handling for same case more readable
	  (Andrew Schorr)
	  Remove the call to ospf_nbr_delete, nsm_change_state can do
	  this generally now via NSM_Deleted.
	  (struct ... NSM) Add the dummy NSM_Deleted state, the 3 events
	  that can lead to nsm_kill_nbr all now transition the NBR to
	  NSM_Deleted and the general change_state function can be left
	  to do the work.
	  (ospf_nsm_event) Special casing of events and early-return can
	  be removed now.
	  On transition into Deleted, delete the nbr.
	* ospf_dump.c: (ospf_nsm_state_msg) Add Deleted.

diff --git a/ospfd/ChangeLog b/ospfd/ChangeLog
index b4f7d3e74b90ed2574b802c8a353a85167029dca..7c374fb82467ec08c41e852cf2169a34643ff6d7 100644 (file)
--- a/ospfd/ChangeLog
+++ b/ospfd/ChangeLog
@@ -1,3 +1,27 @@
+2006-07-07 Paul Jakma <paul.jakma@sun.com>
+
+       * ospf_nsm.h: Add a NSM_Deleted neighbour state, to act as dummy
+         state indicating the neighbour is to be deleted.
+       * ospf_nsm.c: (general) Use the NSM_Deleted state to delete
+         neighbours, thus allowing code to be slightly more obvious
+         in its flow.
+         (nsm_timer_set) Add NSM_Deleted. Add another timer the code
+         missed.
+         (nsm_kill_nbr) No need for special case call to nsm_change_state
+         anymore.
+         Make the assert and error-handling for same case more readable
+         (Andrew Schorr)
+         Remove the call to ospf_nbr_delete, nsm_change_state can do
+         this generally now via NSM_Deleted.
+         (struct ... NSM) Add the dummy NSM_Deleted state, the 3 events
+         that can lead to nsm_kill_nbr all now transition the NBR to
+         NSM_Deleted and the general change_state function can be left
+         to do the work.
+         (ospf_nsm_event) Special casing of events and early-return can
+         be removed now.
+         On transition into Deleted, delete the nbr.
+       * ospf_dump.c: (ospf_nsm_state_msg) Add Deleted.
+
 2006-07-06 Paul Jakma <paul.jakma@sun.com>
 
        * ospf_nsm.c: (ospf_nsm_event) LLDown event also results in nbr

diff --git a/ospfd/ospf_dump.c b/ospfd/ospf_dump.c
index 47b76fcb2c9ffb235579ac5cf055ce059f547515..b8dc7951b9c027ee5c4c650298bd4510def383c2 100644 (file)
--- a/ospfd/ospf_dump.c
+++ b/ospfd/ospf_dump.c
@@ -58,6 +58,7 @@ int ospf_ism_state_msg_max = OSPF_ISM_STATE_MAX;
 struct message ospf_nsm_state_msg[] =
 {
   { NSM_DependUpon, "DependUpon" },
+  { NSM_Deleted,    "Deleted"    },
   { NSM_Down,       "Down" },
   { NSM_Attempt,    "Attempt" },
   { NSM_Init,       "Init" },

diff --git a/ospfd/ospf_nsm.c b/ospfd/ospf_nsm.c
index fb736eba78539407b664c391ada3fd358403029f..56f818659011490d90d8bf457c83620337cfafa6 100644 (file)
--- a/ospfd/ospf_nsm.c
+++ b/ospfd/ospf_nsm.c
@@ -95,18 +95,24 @@ ospf_db_desc_timer (struct thread *thread)
   return 0;
 }
 
-/* Hook function called after ospf NSM event is occured. */
-
+/* Hook function called after ospf NSM event is occured.
+ *
+ * Set/clear any timers whose condition is implicit to the neighbour
+ * state. There may be other timers which are set/unset according to other
+ * state.
+ *
+ * We rely on this function to properly clear timers in lower states,
+ * particularly before deleting a neighbour.
+ */
 static void
 nsm_timer_set (struct ospf_neighbor *nbr)
 {
   switch (nbr->state)
     {
+    case NSM_Deleted:
     case NSM_Down:
-      /* This is here for documentation purposes, don't actually get here
-       * as Down neighbours are deleted typically, see nsm_kill_nbr
-       */
       OSPF_NSM_TIMER_OFF (nbr->t_inactivity);
+      OSPF_NSM_TIMER_OFF (nbr->t_hello_reply);
     case NSM_Attempt:
     case NSM_Init:
     case NSM_TwoWay:
@@ -332,9 +338,6 @@ nsm_exchange_done (struct ospf_neighbor *nbr)
   if (ospf_ls_request_isempty (nbr))
     return NSM_Full;
 
-  /* Cancel dd retransmit timer. */
-  /* OSPF_NSM_TIMER_OFF (nbr->t_db_desc); */
-
   /* Send Link State Request. */
   ospf_ls_req_send (nbr);
 
@@ -422,13 +425,12 @@ nsm_reset_nbr (struct ospf_neighbor *nbr)
 static int
 nsm_kill_nbr (struct ospf_neighbor *nbr)
 {
-  /* call it here because we cannot call it from ospf_nsm_event */
-  nsm_change_state (nbr, NSM_Down);
-  
   /* killing nbr_self is invalid */
-  assert (nbr != nbr->oi->nbr_self);
   if (nbr == nbr->oi->nbr_self)
-    return 0;
+    {
+      assert (nbr != nbr->oi->nbr_self);
+      return 0;
+    }
   
   /* Reset neighbor. */
   nsm_reset_nbr (nbr);
@@ -450,9 +452,6 @@ nsm_kill_nbr (struct ospf_neighbor *nbr)
                   IF_NAME (nbr->oi), inet_ntoa (nbr->address.u.prefix4));  
     }
 
-  /* Delete neighbor from interface. */
-  ospf_nbr_delete (nbr);
-
   return 0;
 }
 
@@ -468,9 +467,6 @@ nsm_inactivity_timer (struct ospf_neighbor *nbr)
 static int
 nsm_ll_down (struct ospf_neighbor *nbr)
 {
-  /* Reset neighbor. */
-  /*nsm_reset_nbr (nbr);*/
-  
   /* Kill neighbor. */
   nsm_kill_nbr (nbr);
 
@@ -500,6 +496,23 @@ struct {
     { nsm_ignore,              NSM_DependUpon }, /* InactivityTimer   */
     { nsm_ignore,              NSM_DependUpon }, /* LLDown            */
   },
+  {
+    /* Deleted: dummy state. */
+    { nsm_ignore,              NSM_Deleted    }, /* NoEvent           */
+    { nsm_ignore,              NSM_Deleted    }, /* HelloReceived     */
+    { nsm_ignore,              NSM_Deleted    }, /* Start             */
+    { nsm_ignore,              NSM_Deleted    }, /* 2-WayReceived     */
+    { nsm_ignore,              NSM_Deleted    }, /* NegotiationDone   */
+    { nsm_ignore,              NSM_Deleted    }, /* ExchangeDone      */
+    { nsm_ignore,              NSM_Deleted    }, /* BadLSReq          */
+    { nsm_ignore,              NSM_Deleted    }, /* LoadingDone       */
+    { nsm_ignore,              NSM_Deleted    }, /* AdjOK?            */
+    { nsm_ignore,              NSM_Deleted    }, /* SeqNumberMismatch */
+    { nsm_ignore,              NSM_Deleted    }, /* 1-WayReceived     */
+    { nsm_ignore,              NSM_Deleted    }, /* KillNbr           */
+    { nsm_ignore,              NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ignore,              NSM_Deleted    }, /* LLDown            */
+  },
   {
     /* Down: */
     { nsm_ignore,              NSM_DependUpon }, /* NoEvent           */
@@ -513,9 +526,9 @@ struct {
     { nsm_ignore,              NSM_Down       }, /* AdjOK?            */
     { nsm_ignore,              NSM_Down       }, /* SeqNumberMismatch */
     { nsm_ignore,              NSM_Down       }, /* 1-WayReceived     */
-    { nsm_kill_nbr,            NSM_Down       }, /* KillNbr           */
-    { nsm_inactivity_timer,    NSM_Down       }, /* InactivityTimer   */
-    { nsm_ll_down,             NSM_Down       }, /* LLDown            */
+    { nsm_kill_nbr,            NSM_Deleted    }, /* KillNbr           */
+    { nsm_inactivity_timer,    NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ll_down,             NSM_Deleted    }, /* LLDown            */
   },
   {
     /* Attempt: */
@@ -530,9 +543,9 @@ struct {
     { nsm_ignore,              NSM_Attempt    }, /* AdjOK?            */
     { nsm_ignore,              NSM_Attempt    }, /* SeqNumberMismatch */
     { nsm_ignore,              NSM_Attempt    }, /* 1-WayReceived     */
-    { nsm_kill_nbr,            NSM_Down       }, /* KillNbr           */
-    { nsm_inactivity_timer,    NSM_Down       }, /* InactivityTimer   */
-    { nsm_ll_down,             NSM_Down       }, /* LLDown            */
+    { nsm_kill_nbr,            NSM_Deleted    }, /* KillNbr           */
+    { nsm_inactivity_timer,    NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ll_down,             NSM_Deleted    }, /* LLDown            */
   },
   {
     /* Init: */
@@ -547,9 +560,9 @@ struct {
     { nsm_ignore,              NSM_Init       }, /* AdjOK?            */
     { nsm_ignore,              NSM_Init       }, /* SeqNumberMismatch */
     { nsm_ignore,              NSM_Init       }, /* 1-WayReceived     */
-    { nsm_kill_nbr,            NSM_Down       }, /* KillNbr           */
-    { nsm_inactivity_timer,    NSM_Down       }, /* InactivityTimer   */
-    { nsm_ll_down,             NSM_Down       }, /* LLDown            */
+    { nsm_kill_nbr,            NSM_Deleted    }, /* KillNbr           */
+    { nsm_inactivity_timer,    NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ll_down,             NSM_Deleted    }, /* LLDown            */
   },
   {
     /* 2-Way: */
@@ -564,9 +577,9 @@ struct {
     { nsm_adj_ok,              NSM_DependUpon }, /* AdjOK?            */
     { nsm_ignore,              NSM_TwoWay     }, /* SeqNumberMismatch */
     { nsm_oneway_received,     NSM_Init       }, /* 1-WayReceived     */
-    { nsm_kill_nbr,            NSM_Down       }, /* KillNbr           */
-    { nsm_inactivity_timer,    NSM_Down       }, /* InactivityTimer   */
-    { nsm_ll_down,             NSM_Down       }, /* LLDown            */
+    { nsm_kill_nbr,            NSM_Deleted    }, /* KillNbr           */
+    { nsm_inactivity_timer,    NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ll_down,             NSM_Deleted    }, /* LLDown            */
   },
   {
     /* ExStart: */
@@ -581,9 +594,9 @@ struct {
     { nsm_adj_ok,              NSM_DependUpon }, /* AdjOK?            */
     { nsm_ignore,              NSM_ExStart    }, /* SeqNumberMismatch */
     { nsm_oneway_received,     NSM_Init       }, /* 1-WayReceived     */
-    { nsm_kill_nbr,            NSM_Down       }, /* KillNbr           */
-    { nsm_inactivity_timer,    NSM_Down       }, /* InactivityTimer   */
-    { nsm_ll_down,             NSM_Down       }, /* LLDown            */
+    { nsm_kill_nbr,            NSM_Deleted    }, /* KillNbr           */
+    { nsm_inactivity_timer,    NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ll_down,             NSM_Deleted    }, /* LLDown            */
   },
   {
     /* Exchange: */
@@ -598,9 +611,9 @@ struct {
     { nsm_adj_ok,              NSM_DependUpon }, /* AdjOK?            */
     { nsm_seq_number_mismatch, NSM_ExStart    }, /* SeqNumberMismatch */
     { nsm_oneway_received,     NSM_Init       }, /* 1-WayReceived     */
-    { nsm_kill_nbr,            NSM_Down       }, /* KillNbr           */
-    { nsm_inactivity_timer,    NSM_Down       }, /* InactivityTimer   */
-    { nsm_ll_down,             NSM_Down       }, /* LLDown            */
+    { nsm_kill_nbr,            NSM_Deleted    }, /* KillNbr           */
+    { nsm_inactivity_timer,    NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ll_down,             NSM_Deleted    }, /* LLDown            */
   },
   {
     /* Loading: */
@@ -615,9 +628,9 @@ struct {
     { nsm_adj_ok,              NSM_DependUpon }, /* AdjOK?            */
     { nsm_seq_number_mismatch, NSM_ExStart    }, /* SeqNumberMismatch */
     { nsm_oneway_received,     NSM_Init       }, /* 1-WayReceived     */
-    { nsm_kill_nbr,            NSM_Down       }, /* KillNbr           */
-    { nsm_inactivity_timer,    NSM_Down       }, /* InactivityTimer   */
-    { nsm_ll_down,             NSM_Down       }, /* LLDown            */
+    { nsm_kill_nbr,            NSM_Deleted    }, /* KillNbr           */
+    { nsm_inactivity_timer,    NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ll_down,             NSM_Deleted    }, /* LLDown            */
   },
   { /* Full: */
     { nsm_ignore,              NSM_DependUpon }, /* NoEvent           */
@@ -631,9 +644,9 @@ struct {
     { nsm_adj_ok,              NSM_DependUpon }, /* AdjOK?            */
     { nsm_seq_number_mismatch, NSM_ExStart    }, /* SeqNumberMismatch */
     { nsm_oneway_received,     NSM_Init       }, /* 1-WayReceived     */
-    { nsm_kill_nbr,            NSM_Down       }, /* KillNbr           */
-    { nsm_inactivity_timer,    NSM_Down       }, /* InactivityTimer   */
-    { nsm_ll_down,             NSM_Down       }, /* LLDown            */
+    { nsm_kill_nbr,            NSM_Deleted    }, /* KillNbr           */
+    { nsm_inactivity_timer,    NSM_Deleted    }, /* InactivityTimer   */
+    { nsm_ll_down,             NSM_Deleted    }, /* LLDown            */
   },
 };
 
@@ -689,9 +702,9 @@ nsm_change_state (struct ospf_neighbor *nbr, int state)
       (CHECK_FLAG(oi->ospf->config, OSPF_LOG_ADJACENCY_DETAIL) ||
        (state == NSM_Full) || (state < old_state)))
     zlog_notice("AdjChg: Nbr %s on %s: %s -> %s",
-               inet_ntoa (nbr->router_id), IF_NAME (nbr->oi),
-               LOOKUP (ospf_nsm_state_msg, old_state),
-               LOOKUP (ospf_nsm_state_msg, state));
+                inet_ntoa (nbr->router_id), IF_NAME (nbr->oi),
+                LOOKUP (ospf_nsm_state_msg, old_state),
+                LOOKUP (ospf_nsm_state_msg, state));
 
 #ifdef HAVE_SNMP
   /* Terminal state or regression */ 
@@ -701,7 +714,7 @@ nsm_change_state (struct ospf_neighbor *nbr, int state)
       if (oi->type == OSPF_IFTYPE_VIRTUALLINK)
         ospfTrapVirtNbrStateChange(nbr);
       /* ospfNbrStateChange trap  */
-      else
+      else     
         /* To/From FULL, only managed by DR */
         if (((state != NSM_Full) && (old_state != NSM_Full)) ||
             (oi->state == ISM_DR))
@@ -855,23 +868,6 @@ ospf_nsm_event (struct thread *thread)
   /* Call function. */
   next_state = (*(NSM [nbr->state][event].func))(nbr);
 
-  /* When event is NSM_KillNbr or InactivityTimer, the neighbor is
-     deleted. */
-  if (event == NSM_KillNbr 
-      || event == NSM_InactivityTimer
-      || event == NSM_LLDown)
-    {
-      if (IS_DEBUG_OSPF (nsm, NSM_EVENTS))
-       zlog_debug ("NSM[%s:%s]: neighbor deleted",
-                  IF_NAME (oi), inet_ntoa (router_id));
-
-      /* Timers are canceled in ospf_nbr_free, moreover we cannot call
-         nsm_timer_set here because nbr is freed already!!!*/
-      /*nsm_timer_set (nbr);*/
-
-      return 0;
-    }
-
   if (! next_state)
     next_state = NSM [nbr->state][event].next_state;
   else if (NSM [nbr->state][event].next_state != NSM_DependUpon)
@@ -904,6 +900,16 @@ ospf_nsm_event (struct thread *thread)
   /* Make sure timer is set. */
   nsm_timer_set (nbr);
 
+  /* When event is NSM_KillNbr, InactivityTimer or LLDown, the neighbor
+   * is deleted.
+   *
+   * Rather than encode knowledge here of which events lead to NBR
+   * delete, we take our cue from the NSM table, via the dummy
+   * 'Deleted' neighbour state.
+   */
+  if (nbr->state == NSM_Deleted)
+    ospf_nbr_delete (nbr);
+
   return 0;
 }
 

diff --git a/ospfd/ospf_nsm.h b/ospfd/ospf_nsm.h
index fe42f7a0a8c565de63eea1ca31fd3ebb45fdb88f..1121dae6108a856084fe72d0cc58a6dfc2fc2264 100644 (file)
--- a/ospfd/ospf_nsm.h
+++ b/ospfd/ospf_nsm.h
@@ -26,15 +26,16 @@
 
 /* OSPF Neighbor State Machine State. */
 #define NSM_DependUpon          0
-#define NSM_Down               1
-#define NSM_Attempt            2
-#define NSM_Init               3
-#define NSM_TwoWay             4
-#define NSM_ExStart            5
-#define NSM_Exchange           6
-#define NSM_Loading            7
-#define NSM_Full               8
-#define OSPF_NSM_STATE_MAX      9
+#define NSM_Deleted            1
+#define NSM_Down               2
+#define NSM_Attempt            3
+#define NSM_Init               4
+#define NSM_TwoWay             5
+#define NSM_ExStart            6
+#define NSM_Exchange           7
+#define NSM_Loading            8
+#define NSM_Full               9
+#define OSPF_NSM_STATE_MAX     10
 
 /* OSPF Neighbor State Machine Event. */
 #define NSM_NoEvent            0