[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.

4 files changed, 105 insertions, 73 deletions

diff --git a/ospfd/ChangeLog b/ospfd/ChangeLog
index b4f7d3e7..7c374fb8 100644
--- 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 47b76fcb..b8dc7951 100644
--- 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 fb736eba..56f81865 100644
--- 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);
 
@@ -501,6 +497,23 @@ struct {
     { 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           */
     { nsm_hello_received,      NSM_Init       }, /* HelloReceived     */
@@ -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 fe42f7a0..1121dae6 100644
--- 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