From 02b4805f3914ef6ba0242c6f4dd1b6759ef97bf2 Mon Sep 17 00:00:00 2001 From: Christian Franke Date: Thu, 24 Jan 2013 14:04:49 +0000 Subject: zebra: don't change connected state from zebra/interface.c Try to avoid changing connected state from zebra/interface.c as this means making assumptions about kernel behaviour which may be or may become wrong. This state should rather be updated by events from the kernel. Signed-off-by: Christian Franke Signed-off-by: David Lamparter --- zebra/connected.c | 49 +++++++++++++-------- zebra/interface.c | 126 +++++++++++++++++------------------------------------- 2 files changed, 69 insertions(+), 106 deletions(-) diff --git a/zebra/connected.c b/zebra/connected.c index d474560c..4802f2ba 100644 --- a/zebra/connected.c +++ b/zebra/connected.c @@ -37,7 +37,7 @@ #include "zebra/connected.h" extern struct zebra_t zebrad; -/* withdraw a connected address */ +/* communicate the withdrawal of a connected address */ static void connected_withdraw (struct connected *ifc) { @@ -81,24 +81,19 @@ connected_announce (struct interface *ifp, struct connected *ifc) listnode_add (ifp->connected, ifc); /* Update interface address information to protocol daemon. */ - if (! CHECK_FLAG (ifc->conf, ZEBRA_IFC_REAL)) - { - if (ifc->address->family == AF_INET) - if_subnet_add (ifp, ifc); + if (ifc->address->family == AF_INET) + if_subnet_add (ifp, ifc); - SET_FLAG (ifc->conf, ZEBRA_IFC_REAL); + zebra_interface_address_add_update (ifp, ifc); - zebra_interface_address_add_update (ifp, ifc); - - if (if_is_operative(ifp)) - { - if (ifc->address->family == AF_INET) - connected_up_ipv4 (ifp, ifc); + if (if_is_operative(ifp)) + { + if (ifc->address->family == AF_INET) + connected_up_ipv4 (ifp, ifc); #ifdef HAVE_IPV6 - else - connected_up_ipv6 (ifp, ifc); + else + connected_up_ipv6 (ifp, ifc); #endif - } } } @@ -116,7 +111,7 @@ connected_check (struct interface *ifp, struct prefix *p) return NULL; } -/* Check if two ifc's describe the same address */ +/* Check if two ifc's describe the same address in the same state */ static int connected_same (struct connected *ifc1, struct connected *ifc2) { @@ -136,6 +131,9 @@ connected_same (struct connected *ifc1, struct connected *ifc2) if (ifc1->flags != ifc2->flags) return 0; + + if (ifc1->conf != ifc2->conf) + return 0; return 1; } @@ -156,7 +154,7 @@ connected_update(struct interface *ifp, struct connected *ifc) /* Avoid spurious withdraws, this might be just the kernel 'reflecting' * back an address we have already added. */ - if (connected_same (current, ifc) && CHECK_FLAG(current->conf, ZEBRA_IFC_REAL)) + if (connected_same (current, ifc)) { /* nothing to do */ connected_free (ifc); @@ -169,8 +167,9 @@ connected_update(struct interface *ifp, struct connected *ifc) connected_withdraw (current); /* implicit withdraw - freebsd does this */ } - /* If the connected is new or has changed, announce it */ - connected_announce(ifp, ifc); + /* If the connected is new or has changed, announce it, if it is usable */ + if (CHECK_FLAG(ifc->conf, ZEBRA_IFC_REAL)) + connected_announce(ifp, ifc); } /* Called from if_up(). */ @@ -279,6 +278,10 @@ connected_add_ipv4 (struct interface *ifp, int flags, struct in_addr *addr, if (label) ifc->label = XSTRDUP (MTYPE_CONNECTED_LABEL, label); + /* For all that I know an IPv4 address is always ready when we receive + * the notification. So it should be safe to set the REAL flag here. */ + SET_FLAG(ifc->conf, ZEBRA_IFC_REAL); + connected_update(ifp, ifc); } @@ -407,6 +410,14 @@ connected_add_ipv6 (struct interface *ifp, int flags, struct in6_addr *addr, if (label) ifc->label = XSTRDUP (MTYPE_CONNECTED_LABEL, label); + /* On Linux, we only get here when DAD is complete, therefore we can set + * ZEBRA_IFC_REAL. + * + * On BSD, there currently doesn't seem to be a way to check for completion of + * DAD, so we replicate the old behaviour and set ZEBRA_IFC_REAL, although DAD + * might still be running. + */ + SET_FLAG(ifc->conf, ZEBRA_IFC_REAL); connected_update(ifp, ifc); } diff --git a/zebra/interface.c b/zebra/interface.c index 470df0cd..51798ca6 100644 --- a/zebra/interface.c +++ b/zebra/interface.c @@ -193,6 +193,9 @@ if_subnet_delete (struct interface *ifp, struct connected *ifc) ifc = listgetdata (listhead (addr_list)); zebra_interface_address_delete_update (ifp, ifc); UNSET_FLAG (ifc->flags, ZEBRA_IFA_SECONDARY); + /* XXX: Linux kernel removes all the secondary addresses when the primary + * address is removed. We could try to work around that, though this is + * non-trivial. */ zebra_interface_address_add_update (ifp, ifc); } @@ -296,16 +299,23 @@ if_addr_wakeup (struct interface *ifp) { if (! if_is_up (ifp)) { - /* XXX: WTF is it trying to set flags here? - * caller has just gotten a new interface, has been - * handed the flags already. This code has no business - * trying to override administrative status of the interface. - * The only call path to here which doesn't originate from - * kernel event is irdp - what on earth is it trying to do? - * - * further RUNNING is not a settable flag on any system - * I (paulj) am aware of. - */ + /* Assume zebra is configured like following: + * + * interface gre0 + * ip addr 192.0.2.1/24 + * ! + * + * As soon as zebra becomes first aware that gre0 exists in the + * kernel, it will set gre0 up and configure its addresses. + * + * (This may happen at startup when the interface already exists + * or during runtime when the interface is added to the kernel) + * + * XXX: IRDP code is calling here via if_add_update - this seems + * somewhat weird. + * XXX: RUNNING is not a settable flag on any system + * I (paulj) am aware of. + */ if_set_flags (ifp, IFF_UP | IFF_RUNNING); if_refresh (ifp); } @@ -318,23 +328,17 @@ if_addr_wakeup (struct interface *ifp) continue; } - /* Add to subnet chain list. */ - if_subnet_add (ifp, ifc); - SET_FLAG (ifc->conf, ZEBRA_IFC_QUEUED); - SET_FLAG (ifc->conf, ZEBRA_IFC_REAL); - - zebra_interface_address_add_update (ifp, ifc); - - if (if_is_operative(ifp)) - connected_up_ipv4 (ifp, ifc); + /* The address will be advertised to zebra clients when the notification + * from the kernel has been received. + * It will also be added to the interface's subnet list then. */ } #ifdef HAVE_IPV6 if (p->family == AF_INET6) { if (! if_is_up (ifp)) { - /* XXX: See long comment above */ + /* See long comment above */ if_set_flags (ifp, IFF_UP | IFF_RUNNING); if_refresh (ifp); } @@ -346,13 +350,10 @@ if_addr_wakeup (struct interface *ifp) safe_strerror(errno)); continue; } - SET_FLAG (ifc->conf, ZEBRA_IFC_QUEUED); - SET_FLAG (ifc->conf, ZEBRA_IFC_REAL); - - zebra_interface_address_add_update (ifp, ifc); - if (if_is_operative(ifp)) - connected_up_ipv6 (ifp, ifc); + SET_FLAG (ifc->conf, ZEBRA_IFC_QUEUED); + /* The address will be advertised to zebra clients when the notification + * from the kernel has been received. */ } #endif /* HAVE_IPV6 */ } @@ -450,9 +451,11 @@ if_delete_update (struct interface *ifp) ifc = listgetdata (anode); p = ifc->address; - connected_down_ipv4 (ifp, ifc); + /* XXX: We have to send notifications here explicitly, because we destroy + * the ifc before receiving the notification about the address being deleted. + */ zebra_interface_address_delete_update (ifp, ifc); UNSET_FLAG (ifc->conf, ZEBRA_IFC_REAL); @@ -1251,19 +1254,10 @@ ip_address_install (struct vty *vty, struct interface *ifp, return CMD_WARNING; } - /* Add to subnet chain list (while marking secondary attribute). */ - if_subnet_add (ifp, ifc); - - /* IP address propery set. */ SET_FLAG (ifc->conf, ZEBRA_IFC_QUEUED); - SET_FLAG (ifc->conf, ZEBRA_IFC_REAL); - - /* Update interface address information to protocol daemon. */ - zebra_interface_address_add_update (ifp, ifc); - - /* If interface is up register connected route. */ - if (if_is_operative(ifp)) - connected_up_ipv4 (ifp, ifc); + /* The address will be advertised to zebra clients when the notification + * from the kernel has been received. + * It will also be added to the subnet chain list, then. */ } return CMD_SUCCESS; @@ -1317,35 +1311,9 @@ ip_address_uninstall (struct vty *vty, struct interface *ifp, safe_strerror(errno), VTY_NEWLINE); return CMD_WARNING; } - /* success! call returned that the address deletion went through. - * this is a synchronous operation, so we know it succeeded and can - * now update all internal state. */ - - /* the HAVE_NETLINK check is only here because, on BSD, although the - * call above is still synchronous, we get a second confirmation later - * through the route socket, and we don't want to touch that behaviour - * for now. It should work without the #ifdef, but why take the risk... - * -- equinox 2012-07-13 */ UNSET_FLAG (ifc->conf, ZEBRA_IFC_QUEUED); -#ifdef HAVE_NETLINK - - /* Remove connected route. */ - connected_down_ipv4 (ifp, ifc); - - /* Redistribute this information. */ - zebra_interface_address_delete_update (ifp, ifc); - - /* IP address propery set. */ - UNSET_FLAG (ifc->conf, ZEBRA_IFC_REAL); - - /* remove from interface, remark secondaries */ - if_subnet_delete (ifp, ifc); - - /* Free address information. */ - listnode_delete (ifp->connected, ifc); - connected_free (ifc); -#endif - + /* we will receive a kernel notification about this route being removed. + * this will trigger its removal from the connected list. */ return CMD_SUCCESS; } @@ -1462,16 +1430,9 @@ ipv6_address_install (struct vty *vty, struct interface *ifp, return CMD_WARNING; } - /* IP address propery set. */ SET_FLAG (ifc->conf, ZEBRA_IFC_QUEUED); - SET_FLAG (ifc->conf, ZEBRA_IFC_REAL); - - /* Update interface address information to protocol daemon. */ - zebra_interface_address_add_update (ifp, ifc); - - /* If interface is up register connected route. */ - if (if_is_operative(ifp)) - connected_up_ipv6 (ifp, ifc); + /* The address will be advertised to zebra clients when the notification + * from the kernel has been received. */ } return CMD_SUCCESS; @@ -1527,17 +1488,8 @@ ipv6_address_uninstall (struct vty *vty, struct interface *ifp, } UNSET_FLAG (ifc->conf, ZEBRA_IFC_QUEUED); - - /* Redistribute this information. */ - zebra_interface_address_delete_update (ifp, ifc); - - /* Remove connected route. */ - connected_down_ipv6 (ifp, ifc); - - /* Free address information. */ - listnode_delete (ifp->connected, ifc); - connected_free (ifc); - + /* This information will be propagated to the zclients when the + * kernel notification is received. */ return CMD_SUCCESS; } -- cgit v1.2.1