]> git.puffer.fish Git - mirror/frr.git/commitdiff
zebra: don't change connected state from zebra/interface.c
authorChristian Franke <chris@opensourcerouting.org>
Thu, 24 Jan 2013 14:04:49 +0000 (14:04 +0000)
committerDavid Lamparter <equinox@opensourcerouting.org>
Thu, 19 Sep 2013 15:51:16 +0000 (17:51 +0200)
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 <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
zebra/connected.c
zebra/interface.c

index d474560c86aa687fcc1011dbe40d65d06c8c75a6..4802f2ba0c7cbf6538ea306dc8b6fe0c1b27442c 100644 (file)
@@ -37,7 +37,7 @@
 #include "zebra/connected.h"
 extern struct zebra_t zebrad;
 \f
-/* 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
-        }
     }
 }
 \f
@@ -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);
 }
 
index 470df0cdd432fdb6c72e01a71b3b8453f95637d4..51798ca6c511b965d266abb6e6d889eeb199ef1b 100644 (file)
@@ -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;
 }