[quagga-dev 3667] Re: PR & patch: rib_delete_ipv6 does not match gateway

David Young dyoung at pobox.com
Tue Sep 20 08:44:59 BST 2005


On Tue, Sep 20, 2005 at 12:06:52AM -0500, David Young wrote:
> On Sun, Sep 18, 2005 at 03:53:46AM -0500, David Young wrote:
> > rib_delete_ipv6 does not match routes in the RIB by their gateway as well
> > as by destination.  I was surprised to find this out; I think it is a bug.
> > Compare with rib_delete_ipv4, which does match by gateway.
> > 
> > I am attaching a patch that fixes the rib_delete_ipv6 bug and reduces the
> > height of some staircases in zebra/zebra_rib.c.  Also, I've added comments
> > noting that lots of common code can be factored out of rib_delete_ipv4 and
> > rib_delete_ipv6.  The net effect, I believe, is less code that works more
> > correctly. :-) Please review and let me know if I botched something up.
> > 
> > (I should mention that my patch is against Quagga 0.98.1.  I looked at
> > Quagga 0.98.5, though, and it is not very different.)
> 
> I have separated the functional part of the patch from the aesthetic part,
> see attachment.

And here is the aesthetic part.

Dave

-- 
David Young             OJC Technologies
dyoung at ojctech.com      Urbana, IL * (217) 278-3933
-------------- next part --------------
Index: zebra_rib.c
===================================================================
--- zebra_rib.c	(revision 3484)
+++ zebra_rib.c	(working copy)
@@ -1010,26 +1010,23 @@ rib_add_ipv4 (int type, int flags, struc
      withdraw. */
   for (rib = rn->info; rib; rib = rib->next)
     {
-      if (rib->type == ZEBRA_ROUTE_CONNECT)
-	{
-	  nexthop = rib->nexthop;
-
-	  /* Duplicate connected route comes in. */
-	  if (rib->type == type
-	      && nexthop && nexthop->type == NEXTHOP_TYPE_IFINDEX
-	      && nexthop->ifindex == ifindex)
-	    {
-	      rib->refcnt++;
-	      return 0 ;
-	    }
-	}
-      else if (rib->type == type)
+      if (rib->type != type)
+	continue;
+      if (rib->type != ZEBRA_ROUTE_CONNECT)
 	{
 	  same = rib;
 	  rib_delnode (rn, same);
 	  route_unlock_node (rn);
 	  break;
 	}
+      /* Duplicate connected route comes in. */
+      else if ((nexthop = rib->nexthop) &&
+	       nexthop->type == NEXTHOP_TYPE_IFINDEX &&
+	       nexthop->ifindex == ifindex)
+	{
+	  rib->refcnt++;
+	  return 0 ;
+	}
     }
 
   /* Allocate new rib structure. */
@@ -1133,6 +1130,7 @@ rib_add_ipv4_multipath (struct prefix_ip
   return 0;
 }
 
+/* XXX factor with rib_delete_ipv6 */
 int
 rib_delete_ipv4 (int type, int flags, struct prefix_ipv4 *p,
 		 struct in_addr *gate, unsigned int ifindex, u_int32_t vrf_id)
@@ -1188,46 +1186,29 @@ rib_delete_ipv4 (int type, int flags, st
       if (CHECK_FLAG (rib->flags, ZEBRA_FLAG_SELECTED))
 	fib = rib;
 
-      if (rib->type == ZEBRA_ROUTE_CONNECT)
+      if (rib->type != type)
+	continue;
+      if (rib->type == ZEBRA_ROUTE_CONNECT && (nexthop = rib->nexthop) &&
+	  nexthop->type == NEXTHOP_TYPE_IFINDEX && nexthop->ifindex == ifindex)
 	{
-	  nexthop = rib->nexthop;
-
-	  if (rib->type == type
-	      && nexthop && nexthop->type == NEXTHOP_TYPE_IFINDEX
-	      && nexthop->ifindex == ifindex)
+	  if (rib->refcnt)
 	    {
-	      if (rib->refcnt)
-		{
-		  rib->refcnt--;
-		  route_unlock_node (rn);
-		  route_unlock_node (rn);
-		  return 0;
-		}
-	      same = rib;
-	      break;
+	      rib->refcnt--;
+	      route_unlock_node (rn);
+	      route_unlock_node (rn);
+	      return 0;
 	    }
+	  same = rib;
+	  break;
 	}
-      else if (gate) 
+      /* Make sure that the route found has the same gateway. */
+      else if (gate == NULL ||
+	       ((nexthop = rib->nexthop) &&
+	        (IPV4_ADDR_SAME (&nexthop->gate.ipv4, gate) ||
+		 IPV4_ADDR_SAME (&nexthop->rgate.ipv4, gate)))) 
         {
-          nexthop = rib->nexthop;
-
-	  /* Make sure that the route found has the same gateway. */
-	  if (rib->type == type
-	      && nexthop &&
-	          (IPV4_ADDR_SAME (&nexthop->gate.ipv4, gate) || 
-		    IPV4_ADDR_SAME (&nexthop->rgate.ipv4, gate)) )
-	    {
-	      same = rib;
-	      break;
-	    }
-	}
-      else
-	{
-	  if (rib->type == type)
-	    {
-	      same = rib;
-	      break;
-	    }
+	  same = rib;
+	  break;
 	}
     }
 
@@ -1656,25 +1637,22 @@ rib_add_ipv6 (int type, int flags, struc
      withdraw. */
   for (rib = rn->info; rib; rib = rib->next)
     {
-      if (rib->type == ZEBRA_ROUTE_CONNECT)
-	{
-	  nexthop = rib->nexthop;
-
-	  if (rib->type == type
-	      && nexthop && nexthop->type == NEXTHOP_TYPE_IFINDEX
-	      && nexthop->ifindex == ifindex)
-	  {
-	    rib->refcnt++;
-	    return 0;
-	  }
-	}
-      else if (rib->type == type)
+      if (rib->type != type)
+	continue;
+      if (rib->type != ZEBRA_ROUTE_CONNECT)
 	{
 	  same = rib;
 	  rib_delnode (rn, same);
 	  route_unlock_node (rn);
 	  break;
 	}
+      else if ((nexthop = rib->nexthop) &&
+	       nexthop->type == NEXTHOP_TYPE_IFINDEX &&
+	       nexthop->ifindex == ifindex)
+	{
+	  rib->refcnt++;
+	  return 0;
+	}
     }
 
   /* Allocate new rib structure. */
@@ -1717,6 +1695,7 @@ rib_add_ipv6 (int type, int flags, struc
   return 0;
 }
 
+/* XXX factor with rib_delete_ipv6 */
 int
 rib_delete_ipv6 (int type, int flags, struct prefix_ipv6 *p,
 		 struct in6_addr *gate, unsigned int ifindex, u_int32_t vrf_id)
@@ -1765,46 +1744,29 @@ rib_delete_ipv6 (int type, int flags, st
       if (CHECK_FLAG (rib->flags, ZEBRA_FLAG_SELECTED))
 	fib = rib;
 
-      if (rib->type == ZEBRA_ROUTE_CONNECT)
+      if (rib->type != type)
+        continue;
+      if (rib->type == ZEBRA_ROUTE_CONNECT && (nexthop = rib->nexthop) &&
+	  nexthop->type == NEXTHOP_TYPE_IFINDEX && nexthop->ifindex == ifindex)
 	{
-	  nexthop = rib->nexthop;
-
-	  if (rib->type == type
-	      && nexthop && nexthop->type == NEXTHOP_TYPE_IFINDEX
-	      && nexthop->ifindex == ifindex)
-	    {
-	      if (rib->refcnt)
-		{
-		  rib->refcnt--;
-		  route_unlock_node (rn);
-		  route_unlock_node (rn);
-		  return 0;
-		}
-	      same = rib;
-	      break;
-	    }
-	}
-      else if (gate) 
-        {
-          nexthop = rib->nexthop;
-
-	  /* Make sure that the route found has the same gateway. */
-	  if (rib->type == type
-	      && nexthop &&
-	          (IPV6_ADDR_SAME (&nexthop->gate.ipv6, gate) || 
-		    IPV6_ADDR_SAME (&nexthop->rgate.ipv6, gate)) )
+	  if (rib->refcnt)
 	    {
-	      same = rib;
-	      break;
+	      rib->refcnt--;
+	      route_unlock_node (rn);
+	      route_unlock_node (rn);
+	      return 0;
 	    }
+	  same = rib;
+	  break;
 	}
-      else
+      /* Make sure that the route found has the same gateway. */
+      else if (gate == NULL ||
+	       ((nexthop = rib->nexthop) &&
+	        (IPV6_ADDR_SAME (&nexthop->gate.ipv6, gate) ||
+		 IPV6_ADDR_SAME (&nexthop->rgate.ipv6, gate))))
 	{
-	  if (rib->type == type)
-	    {
-	      same = rib;
-	      break;
-	    }
+	  same = rib;
+	  break;
 	}
     }
 


More information about the Quagga-dev mailing list