[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 265c9c7..ac59670 100644
--- 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 8df0280..52adc42 100644
--- a/ospfd/ospf_interface.c
+++ b/ospfd/ospf_interface.c
@@ -910,6 +910,7 @@
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 5875236..843e93f 100644
--- 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 @@
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 @@
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 @@
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 @@
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 @@
return nbr;
}
-
diff --git a/ospfd/ospf_nsm.c b/ospfd/ospf_nsm.c
index bfd565e..8a93f0e 100644
--- a/ospfd/ospf_nsm.c
+++ b/ospfd/ospf_nsm.c
@@ -440,6 +440,11 @@
/* 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);