[ospfd] Fix virtual-link handling in nbrs route-table, exposed by bug#234 fix

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

	* (general) Fix issues with handling of Vlinks and entries
	  in the nbrs route-table which were highlighted by the
	  nsm/nbr_self fixes from bug #234. Many thanks to Juergen
	  Kammer for his help and efforts in testing out debug patches to
	  pinpoint the issue.
	* ospf_interface.c: (ospf_vl_new) Add nbr_self for Vlink.
	* ospf_neighbor.c: (ospf_nbr_key) new static function, helper
	  to create key in nbrs table for a given nbr.
	  (ospf_nbr_delete) Use ospf_nbr_key. Add an assert() to
	  document an expected state.
	  (ospf_nbr_add_self) Ditto.
	  (ospf_nbr_lookup_by_addr) Add an assert.
	* ospf_nsm.c: (nsm_kill_nbr) Can never kill the nbr_self
	  psuedo-neighbour.

diff --git a/ospfd/ChangeLog b/ospfd/ChangeLog
index 265c9c7624da3598bdb95b8efeec258e4c6cd55c..ac596705dd7076d6e03821eee010db4f7466243a 100644 (file)
--- a/ospfd/ChangeLog
+++ b/ospfd/ChangeLog
@@ -1,3 +1,20 @@
+2006-04-03 Paul Jakma <paul.jakma@sun.com>
+
+       * (general) Fix issues with handling of Vlinks and entries
+         in the nbrs route-table which were highlighted by the
+         nsm/nbr_self fixes from bug #234. Many thanks to Juergen
+         Kammer for his help and efforts in testing out debug patches to
+         pinpoint the issue.
+       * ospf_interface.c: (ospf_vl_new) Add nbr_self for Vlink.
+       * ospf_neighbor.c: (ospf_nbr_key) new static function, helper
+         to create key in nbrs table for a given nbr.
+         (ospf_nbr_delete) Use ospf_nbr_key. Add an assert() to
+         document an expected state.
+         (ospf_nbr_add_self) Ditto.
+         (ospf_nbr_lookup_by_addr) Add an assert.
+       * ospf_nsm.c: (nsm_kill_nbr) Can never kill the nbr_self
+         psuedo-neighbour.
+
 2006-03-27 Paul Jakma <paul.jakma@sun.com>
 
        * ospf_lsa.c: (ospf_lsa_checksum) Add an explicit cast to avoid

diff --git a/ospfd/ospf_interface.c b/ospfd/ospf_interface.c
index 8df0280a9a7dd05ab2b59ac1e7b00fb9b5133ea7..52adc420871a93e88d77679f55569fe3692a5134 100644 (file)
--- a/ospfd/ospf_interface.c
+++ b/ospfd/ospf_interface.c
@@ -910,6 +910,7 @@ ospf_vl_new (struct ospf *ospf, struct ospf_vl_data *vl_data)
   if (IS_DEBUG_OSPF_EVENT)
     zlog_debug ("ospf_vl_new(): set associated area to the backbone");
 
+  ospf_nbr_add_self (voi);
   ospf_area_add_if (voi->area, voi);
 
   ospf_if_stream_set (voi);

diff --git a/ospfd/ospf_neighbor.c b/ospfd/ospf_neighbor.c
index 58752366ebe5efe6cb998b24e12d4fbf2a1e43a4..843e93f6b9a4efad70e719e843d46a67eb641c3b 100644 (file)
--- a/ospfd/ospf_neighbor.c
+++ b/ospfd/ospf_neighbor.c
@@ -43,6 +43,26 @@
 #include "ospfd/ospf_flood.h"
 #include "ospfd/ospf_dump.h"
 
+/* Fill in the the 'key' as appropriate to retrieve the entry for nbr
+ * from the ospf_interface's nbrs table. Indexed by interface address
+ * for all cases except Virtual-link interfaces, where neighbours are
+ * indexed by router-ID instead.
+ */
+static void
+ospf_nbr_key (struct ospf_interface *oi, struct ospf_neighbor *nbr,
+              struct prefix *key)
+{
+  key->family = AF_INET;
+  key->prefixlen = IPV4_MAX_BITLEN;
+
+  /* vlinks are indexed by router-id */
+  if (oi->type == OSPF_IFTYPE_VIRTUALLINK)
+    key->u.prefix4 = nbr->router_id;
+  else
+    key->u.prefix4 = nbr->src;
+  return;
+}
+
 struct ospf_neighbor *
 ospf_nbr_new (struct ospf_interface *oi)
 {
@@ -134,20 +154,22 @@ ospf_nbr_delete (struct ospf_neighbor *nbr)
   struct prefix p;
 
   oi = nbr->oi;
-
-  /* Unlink ospf neighbor from the interface. */
-  p.family = AF_INET;
-  p.prefixlen = IPV4_MAX_BITLEN;
-
-  /* vlinks are indexed by router-id */
-  if (oi->type == OSPF_IFTYPE_VIRTUALLINK)
-    p.u.prefix4 = nbr->router_id;
-  else
-  p.u.prefix4 = nbr->src;
+  
+  /* get appropriate prefix 'key' */
+  ospf_nbr_key (oi, nbr, &p);
 
   rn = route_node_lookup (oi->nbrs, &p);
   if (rn)
     {
+      /* If lookup for a NBR succeeds, the leaf route_node could
+       * only exist because there is (or was) a nbr there.
+       * If the nbr was deleted, the leaf route_node should have
+       * lost its last refcount too, and be deleted.
+       * Therefore a looked-up leaf route_node in nbrs table
+       * should never have NULL info.
+       */
+      assert (rn->info);
+      
       if (rn->info)
        {
          rn->info = NULL;
@@ -185,24 +207,9 @@ ospf_nbr_bidirectional (struct in_addr *router_id,
 void
 ospf_nbr_add_self (struct ospf_interface *oi)
 {
-  struct ospf_neighbor *nbr;
   struct prefix p;
   struct route_node *rn;
 
-  p.family = AF_INET;
-  p.prefixlen = 32;
-  p.u.prefix4 = oi->address->u.prefix4;
-
-  rn = route_node_get (oi->nbrs, &p);
-  if (rn->info)
-    {
-      /* There is already pseudo neighbor. */
-      nbr = rn->info;
-      route_unlock_node (rn);
-    }
-  else
-    rn->info = oi->nbr_self;
-  
   /* Initial state */
   oi->nbr_self->address = *oi->address;
   oi->nbr_self->priority = OSPF_IF_PARAM (oi, priority);
@@ -223,6 +230,19 @@ ospf_nbr_add_self (struct ospf_interface *oi)
         SET_FLAG (oi->nbr_self->options, OSPF_OPTION_NP);
         break;
     }
+  
+  /* Add nbr_self to nbrs table */
+  ospf_nbr_key (oi, oi->nbr_self, &p);
+  
+  rn = route_node_get (oi->nbrs, &p);
+  if (rn->info)
+    {
+      /* There is already pseudo neighbor. */
+      assert (oi->nbr_self == rn->info);
+      route_unlock_node (rn);
+    }
+  else
+    rn->info = oi->nbr_self;
 }
 
 /* Get neighbor count by status.
@@ -281,6 +301,9 @@ ospf_nbr_lookup_by_addr (struct route_table *nbrs,
   rn = route_node_lookup (nbrs, &p);
   if (! rn)
     return NULL;
+  
+  /* See comment in ospf_nbr_delete */
+  assert (rn->info);
 
   if (rn->info == NULL)
     {
@@ -439,4 +462,3 @@ ospf_nbr_get (struct ospf_interface *oi, struct ospf_header *ospfh,
 
   return nbr;
 }
-  

diff --git a/ospfd/ospf_nsm.c b/ospfd/ospf_nsm.c
index bfd565ef346b2abce951e53ce77f8b958caf6ea1..8a93f0e67160617952417884c344ac71adfadd97 100644 (file)
--- a/ospfd/ospf_nsm.c
+++ b/ospfd/ospf_nsm.c
@@ -440,6 +440,11 @@ 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 1;
+  
   /* Reset neighbor. */
   nsm_reset_nbr (nbr);