On Mon, 21 Dec 2015, Joakim Tjernlund wrote:
Perhaps add ASAN (gcc option) to Q would narrow it down? I think ASAN
should be added as a configure flag as it is a really good tool to find
memory related errors.
It's probably something silly like the vty command changing the type
before doing the down/up cycle. Down, then change, then up (as needed),
would be better.
I don't know if it fixes it, cause it's so hard to trigger, but I've got a
gdb running a breakpoint script to re-run after the command finished, and
an expect script in a loop to do run the command, and it's been going
quite a while without a crash.
See below.
I'd change from OSPF_ISM_EVENT_EXECUTE to ospf_if_{down,up} for the actual
commit, but left it on execute to keep it within vty context for testing.
Also, move the interface state twiddling stuff out of ospf_vty to
ospf_interface for better encapsulation.
-----8<---------8<---------8<---------8<---------8<---------8<----
From 09d4f74fb0eeec829cf02a0f7043db168dbc9c1e Mon Sep 17 00:00:00 2001
From: Paul Jakma <[email protected]>
Date: Mon, 21 Dec 2015 12:57:31 +0000
Subject: ospfd: 'ip ospf network' interface should down iface before changing
type
* ospf_vty.c: (ip_ospf_network) This function changes the interface type
and only then downs/ups the interface if already up. So the down happens
with the interface type already altered. However, the interface type
can have major ramifications for how underlying state is stored/indexed,
which may cause problems.
Further, bit of an encapsulation violation to twiddle state here.
(no_ip_ospf_network) ditto.
* ospf_interface.c: (ospf_if_reset_type) New function to reset the OSPF
interface type on an interface. Ensure the interface is downed before
the type is changed.
* ospf_interface.h: (ospf_if_reset_type) Export, for ospf_vty.c
diff --git a/ospfd/ospf_interface.c b/ospfd/ospf_interface.c
index f41c4f1..f05f116 100644
--- a/ospfd/ospf_interface.c
+++ b/ospfd/ospf_interface.c
@@ -144,6 +144,31 @@ ospf_if_reset_variables (struct ospf_interface *oi)
oi->v_ls_ack = 1;
}
+void
+ospf_if_reset_type (struct interface *ifp, u_char type)
+{
+ struct route_node *rn;
+
+ for (rn = route_top (IF_OIFS (ifp)); rn; rn = route_next (rn))
+ {
+ struct ospf_interface *oi = rn->info;
+ u_char orig_ism_state;
+
+ if (!oi)
+ continue;
+
+ orig_ism_state = oi->state;
+ //ospf_if_down (oi);
+ OSPF_ISM_EVENT_EXECUTE (oi, ISM_InterfaceDown);
+
+ oi->type = IF_DEF_PARAMS (ifp)->type;
+
+ if (orig_ism_state > ISM_Down)
+ OSPF_ISM_EVENT_EXECUTE (oi, ISM_InterfaceUp);
+ //ospf_if_up (oi);
+ }
+}
+
/* lookup oi for specified prefix/ifp */
struct ospf_interface *
ospf_if_table_lookup (struct interface *ifp, struct prefix *prefix)
diff --git a/ospfd/ospf_interface.h b/ospfd/ospf_interface.h
index 2ed426f..9154b0e 100644
--- a/ospfd/ospf_interface.h
+++ b/ospfd/ospf_interface.h
@@ -280,6 +280,7 @@ extern void ospf_if_init (void);
extern void ospf_if_stream_set (struct ospf_interface *);
extern void ospf_if_stream_unset (struct ospf_interface *);
extern void ospf_if_reset_variables (struct ospf_interface *);
+extern void ospf_if_reset_type (struct interface *, u_char type);
extern int ospf_if_is_enable (struct ospf_interface *);
extern int ospf_if_get_output_cost (struct ospf_interface *);
extern void ospf_if_recalculate_output_cost (struct interface *);
diff --git a/ospfd/ospf_vty.c b/ospfd/ospf_vty.c
index 45a19c0..084c5c1 100644
--- a/ospfd/ospf_vty.c
+++ b/ospfd/ospf_vty.c
@@ -5433,7 +5433,6 @@ DEFUN (ip_ospf_network,
{
struct interface *ifp = vty->index;
int old_type = IF_DEF_PARAMS (ifp)->type;
- struct route_node *rn;
if (old_type == OSPF_IFTYPE_LOOPBACK)
{
@@ -5454,23 +5453,7 @@ DEFUN (ip_ospf_network,
return CMD_SUCCESS;
SET_IF_PARAM (IF_DEF_PARAMS (ifp), type);
-
- for (rn = route_top (IF_OIFS (ifp)); rn; rn = route_next (rn))
- {
- struct ospf_interface *oi = rn->info;
-
- if (!oi)
- continue;
-
- oi->type = IF_DEF_PARAMS (ifp)->type;
-
- if (oi->state > ISM_Down)
- {
- OSPF_ISM_EVENT_EXECUTE (oi, ISM_InterfaceDown);
- OSPF_ISM_EVENT_EXECUTE (oi, ISM_InterfaceUp);
- }
- }
-
+ ospf_if_reset_type (ifp, IF_DEF_PARAMS (ifp)->type);
return CMD_SUCCESS;
}
@@ -5494,29 +5477,14 @@ DEFUN (no_ip_ospf_network,
{
struct interface *ifp = vty->index;
int old_type = IF_DEF_PARAMS (ifp)->type;
- struct route_node *rn;
IF_DEF_PARAMS (ifp)->type = ospf_default_iftype(ifp);
if (IF_DEF_PARAMS (ifp)->type == old_type)
return CMD_SUCCESS;
-
- for (rn = route_top (IF_OIFS (ifp)); rn; rn = route_next (rn))
- {
- struct ospf_interface *oi = rn->info;
-
- if (!oi)
- continue;
-
- oi->type = IF_DEF_PARAMS (ifp)->type;
-
- if (oi->state > ISM_Down)
- {
- OSPF_ISM_EVENT_EXECUTE (oi, ISM_InterfaceDown);
- OSPF_ISM_EVENT_EXECUTE (oi, ISM_InterfaceUp);
- }
- }
-
+
+ ospf_if_reset_type (ifp, IF_DEF_PARAMS (ifp)->type);
+
return CMD_SUCCESS;
}
regards,
--
Paul Jakma [email protected] @pjakma Key ID: 64A2FF6A
Fortune:
Freedom of the press is for those who happen to own one.
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
