[quagga-dev,15651] zserv: get rid of code duplication in nexthop_lookup[_mrib] functions

Message ID 1466197272-24178-1-git-send-email-jafar@atcorp.com
State Under Review
Headers show

Commit Message

Jafar Al-Gharaibeh June 17, 2016, 9:01 p.m.
z[send/read]_ipv4_nexthop_lookup functions have been duplicated for multicast mrib lookup. The mrib versions are identical to the unicast versions except for a couple of places. The differences do not justify duplicating two functions and 80 lines of codes. Code refactoring and an if statement with a few lines of code are enough to handle the differences with a lot less and cleaner code.

Signed-off-by: Jafar Al-Gharaibeh <jafar@atcorp.com>
---
 zebra/zserv.c | 121 +++++++++++++---------------------------------------------
 1 file changed, 27 insertions(+), 94 deletions(-)

Comments

Jafar Al-Gharaibeh June 17, 2016, 9:15 p.m. | #1
Between git getting confused a bit between the different versions of the 
functions and me adding extra comments, this patch looks a lot more 
complicated than it needs to be. Basically it boils down to reducing the 
zsend_ipv4_nexthop_lookup_mrib() and zread_ipv4_nexthop_lookup_mrib() 
functions  to the following code in zsend_ipv4_nexthop_lookup():

   /* Lookup nexthop - eBGP excluded */
   if (cmd == ZEBRA_IPV4_NEXTHOP_LOOKUP)
     rib = rib_match_ipv4_safi (addr, SAFI_UNICAST, 1, NULL, vrf_id);
   else  /* ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB */
     {
       rib = rib_match_ipv4_multicast (addr, NULL, vrf_id);
       /* handle the distance field here since
        * it is only needed for MRIB command */
       if (rib)
         stream_putc (s, rib->distance);
       else
         stream_putc (s, 0); /* distance */
     }

This replaced/augmented:
   /* Lookup nexthop - eBGP excluded */
   rib = rib_match_ipv4_safi (addr, SAFI_UNICAST, 1, NULL, vrf_id);


Cheers,
Jafar

On 6/17/2016 4:01 PM, Jafar Al-Gharaibeh wrote:
> z[send/read]_ipv4_nexthop_lookup functions have been duplicated for multicast mrib lookup. The mrib versions are identical to the unicast versions except for a couple of places. The differences do not justify duplicating two functions and 80 lines of codes. Code refactoring and an if statement with a few lines of code are enough to handle the differences with a lot less and cleaner code.
>
> Signed-off-by: Jafar Al-Gharaibeh <jafar@atcorp.com>
> ---
>   zebra/zserv.c | 121 +++++++++++++---------------------------------------------
>   1 file changed, 27 insertions(+), 94 deletions(-)
>
> diff --git a/zebra/zserv.c b/zebra/zserv.c
> index 6412f8d..e534656 100644
> --- a/zebra/zserv.c
> +++ b/zebra/zserv.c
> @@ -537,9 +537,14 @@ zsend_ipv6_nexthop_lookup (struct zserv *client, struct in6_addr *addr,
>   }
>   #endif /* HAVE_IPV6 */
>   
> +/*
> +  In the case of ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB:
> +    query unicast rib if nexthop is not found on mrib
> +    and return both route metric and protocol distance.
> +*/
>   static int
>   zsend_ipv4_nexthop_lookup (struct zserv *client, struct in_addr addr,
> -    vrf_id_t vrf_id)
> +			   int cmd, vrf_id_t vrf_id)
>   {
>     struct stream *s;
>     struct rib *rib;
> @@ -547,92 +552,32 @@ zsend_ipv4_nexthop_lookup (struct zserv *client, struct in_addr addr,
>     u_char num;
>     struct nexthop *nexthop;
>   
> -  /* Lookup nexthop - eBGP excluded */
> -  rib = rib_match_ipv4_safi (addr, SAFI_UNICAST, 1, NULL, vrf_id);
> -
>     /* Get output stream. */
>     s = client->obuf;
>     stream_reset (s);
>   
>     /* Fill in result. */
> -  zserv_create_header (s, ZEBRA_IPV4_NEXTHOP_LOOKUP, vrf_id);
> +  zserv_create_header (s, cmd, vrf_id);
>     stream_put_in_addr (s, &addr);
>   
> -  if (rib)
> +  /* Lookup nexthop - eBGP excluded */
> +  if (cmd == ZEBRA_IPV4_NEXTHOP_LOOKUP)
> +    rib = rib_match_ipv4_safi (addr, SAFI_UNICAST, 1, NULL, vrf_id);
> +  else  /* ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB */
>       {
> -      if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
> -        zlog_debug("%s: Matching rib entry found.", __func__);
> -      stream_putl (s, rib->metric);
> -      num = 0;
> -      nump = stream_get_endp(s);
> -      stream_putc (s, 0);
> -      /* Only non-recursive routes are elegible to resolve the nexthop we
> -       * are looking up. Therefore, we will just iterate over the top
> -       * chain of nexthops. */
> -      for (nexthop = rib->nexthop; nexthop; nexthop = nexthop->next)
> -	if (CHECK_FLAG (nexthop->flags, NEXTHOP_FLAG_ACTIVE))
> -	  {
> -	    stream_putc (s, nexthop->type);
> -	    switch (nexthop->type)
> -	      {
> -	      case ZEBRA_NEXTHOP_IPV4:
> -		stream_put_in_addr (s, &nexthop->gate.ipv4);
> -		break;
> -	      case ZEBRA_NEXTHOP_IPV4_IFINDEX:
> -		stream_put_in_addr (s, &nexthop->gate.ipv4);
> -		stream_putl (s, nexthop->ifindex);
> -		break;
> -	      case ZEBRA_NEXTHOP_IFINDEX:
> -	      case ZEBRA_NEXTHOP_IFNAME:
> -		stream_putl (s, nexthop->ifindex);
> -		break;
> -	      default:
> -                /* do nothing */
> -		break;
> -	      }
> -	    num++;
> -	  }
> -      stream_putc_at (s, nump, num);
> +      rib = rib_match_ipv4_multicast (addr, NULL, vrf_id);
> +      /* handle the distance field here since
> +       * it is only needed for MRIB command */
> +      if (rib)
> +	stream_putc (s, rib->distance);
> +      else
> +        stream_putc (s, 0); /* distance */
>       }
> -  else
> -    {
> -      if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
> -        zlog_debug("%s: No matching rib entry found.", __func__);
> -      stream_putl (s, 0);
> -      stream_putc (s, 0);
> -    }
> -
> -  stream_putw_at (s, 0, stream_get_endp (s));
> -
> -  return zebra_server_send_message(client);
> -}
> -
> -/*
> -  Modified version of zsend_ipv4_nexthop_lookup():
> -  Query unicast rib if nexthop is not found on mrib.
> -  Returns both route metric and protocol distance.
> -*/
> -static int
> -zsend_ipv4_nexthop_lookup_mrib (struct zserv *client, struct in_addr addr,
> -				struct rib *rib)
> -{
> -  struct stream *s;
> -  unsigned long nump;
> -  u_char num;
> -  struct nexthop *nexthop;
> -
> -  /* Get output stream. */
> -  s = client->obuf;
> -  stream_reset (s);
> -
> -  /* Fill in result. */
> -  zserv_create_header (s, ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB,
> -		       rib ? rib->vrf_id : VRF_DEFAULT);
> -  stream_put_in_addr (s, &addr);
>   
>     if (rib)
>       {
> -      stream_putc (s, rib->distance);
> +      if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
> +        zlog_debug("%s: Matching rib entry found.", __func__);
>         stream_putl (s, rib->metric);
>         num = 0;
>         nump = stream_get_endp(s); /* remember position for nexthop_num */
> @@ -663,12 +608,13 @@ zsend_ipv4_nexthop_lookup_mrib (struct zserv *client, struct in_addr addr,
>   	      }
>   	    num++;
>   	  }
> -
> +
>         stream_putc_at (s, nump, num); /* store nexthop_num */
>       }
>     else
>       {
> -      stream_putc (s, 0); /* distance */
> +      if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
> +        zlog_debug("%s: No matching rib entry found.", __func__);
>         stream_putl (s, 0); /* metric */
>         stream_putc (s, 0); /* nexthop_num */
>       }
> @@ -728,6 +674,7 @@ zsend_ipv4_import_lookup (struct zserv *client, struct prefix_ipv4 *p,
>   	      }
>   	    num++;
>   	  }
> +
>         stream_putc_at (s, nump, num);
>       }
>     else
> @@ -994,7 +941,7 @@ zread_ipv4_delete (struct zserv *client, u_short length, vrf_id_t vrf_id)
>   
>   /* Nexthop lookup for IPv4. */
>   static int
> -zread_ipv4_nexthop_lookup (struct zserv *client, u_short length,
> +zread_ipv4_nexthop_lookup (int cmd, struct zserv *client, u_short length,
>       vrf_id_t vrf_id)
>   {
>     struct in_addr addr;
> @@ -1004,20 +951,8 @@ zread_ipv4_nexthop_lookup (struct zserv *client, u_short length,
>     if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
>       zlog_debug("%s: looking up %s", __func__,
>                  inet_ntop (AF_INET, &addr, buf, BUFSIZ));
> -  return zsend_ipv4_nexthop_lookup (client, addr, vrf_id);
> -}
> -
> -/* MRIB Nexthop lookup for IPv4. */
> -static int
> -zread_ipv4_nexthop_lookup_mrib (struct zserv *client, u_short length,
> -    vrf_id_t vrf_id)
> -{
> -  struct in_addr addr;
> -  struct rib *rib;
>   
> -  addr.s_addr = stream_get_ipv4 (client->ibuf);
> -  rib = rib_match_ipv4_multicast (addr, NULL, vrf_id);
> -  return zsend_ipv4_nexthop_lookup_mrib (client, addr, rib);
> +  return zsend_ipv4_nexthop_lookup (client, addr, cmd, vrf_id);
>   }
>   
>   /* Nexthop lookup for IPv4. */
> @@ -1486,10 +1421,8 @@ zebra_client_read (struct thread *thread)
>         zebra_redistribute_default_delete (command, client, length, vrf_id);
>         break;
>       case ZEBRA_IPV4_NEXTHOP_LOOKUP:
> -      zread_ipv4_nexthop_lookup (client, length, vrf_id);
> -      break;
>       case ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB:
> -      zread_ipv4_nexthop_lookup_mrib (client, length, vrf_id);
> +      zread_ipv4_nexthop_lookup (command, client, length, vrf_id);
>         break;
>   #ifdef HAVE_IPV6
>       case ZEBRA_IPV6_NEXTHOP_LOOKUP:
cisystem@netdef.org June 17, 2016, 10:20 p.m. | #2
Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF CI System <cisystem@netdef.org>

This is an EXPERIMENTAL automated CI system.
For questions and feedback, feel free to email
Martin Winter <mwinter@opensourcerouting.org>.

Patches applied :
  Patchwork 2007: http://patchwork.quagga.net/patch/2007
       [quagga-dev,15651] zserv: get rid of code duplication in nexthop_lookup[_mrib] functions
Tested on top of Git : 5f67888 (as of 20160429.234845 UTC)
CI System Testrun URL: https://ci1.netdef.org/browse/QUAGGA-QPWORK-320/


Regards,
  NetDEF/OpenSourceRouting Continous Integration (CI) System

---
OpenSourceRouting.org is a project of the Network Device Education Foundation,
For more information, see www.netdef.org and www.opensourcerouting.org
For questions in regards to this CI System, contact Martin Winter, mwinter@netdef.org

Patch hide | download patch | download mbox

diff --git a/zebra/zserv.c b/zebra/zserv.c
index 6412f8d..e534656 100644
--- a/zebra/zserv.c
+++ b/zebra/zserv.c
@@ -537,9 +537,14 @@  zsend_ipv6_nexthop_lookup (struct zserv *client, struct in6_addr *addr,
 }
 #endif /* HAVE_IPV6 */
 
+/*
+  In the case of ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB:
+    query unicast rib if nexthop is not found on mrib
+    and return both route metric and protocol distance.
+*/
 static int
 zsend_ipv4_nexthop_lookup (struct zserv *client, struct in_addr addr,
-    vrf_id_t vrf_id)
+			   int cmd, vrf_id_t vrf_id)
 {
   struct stream *s;
   struct rib *rib;
@@ -547,92 +552,32 @@  zsend_ipv4_nexthop_lookup (struct zserv *client, struct in_addr addr,
   u_char num;
   struct nexthop *nexthop;
 
-  /* Lookup nexthop - eBGP excluded */
-  rib = rib_match_ipv4_safi (addr, SAFI_UNICAST, 1, NULL, vrf_id);
-
   /* Get output stream. */
   s = client->obuf;
   stream_reset (s);
 
   /* Fill in result. */
-  zserv_create_header (s, ZEBRA_IPV4_NEXTHOP_LOOKUP, vrf_id);
+  zserv_create_header (s, cmd, vrf_id);
   stream_put_in_addr (s, &addr);
 
-  if (rib)
+  /* Lookup nexthop - eBGP excluded */
+  if (cmd == ZEBRA_IPV4_NEXTHOP_LOOKUP)
+    rib = rib_match_ipv4_safi (addr, SAFI_UNICAST, 1, NULL, vrf_id);
+  else  /* ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB */
     {
-      if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
-        zlog_debug("%s: Matching rib entry found.", __func__);
-      stream_putl (s, rib->metric);
-      num = 0;
-      nump = stream_get_endp(s);
-      stream_putc (s, 0);
-      /* Only non-recursive routes are elegible to resolve the nexthop we
-       * are looking up. Therefore, we will just iterate over the top
-       * chain of nexthops. */
-      for (nexthop = rib->nexthop; nexthop; nexthop = nexthop->next)
-	if (CHECK_FLAG (nexthop->flags, NEXTHOP_FLAG_ACTIVE))
-	  {
-	    stream_putc (s, nexthop->type);
-	    switch (nexthop->type)
-	      {
-	      case ZEBRA_NEXTHOP_IPV4:
-		stream_put_in_addr (s, &nexthop->gate.ipv4);
-		break;
-	      case ZEBRA_NEXTHOP_IPV4_IFINDEX:
-		stream_put_in_addr (s, &nexthop->gate.ipv4);
-		stream_putl (s, nexthop->ifindex);
-		break;
-	      case ZEBRA_NEXTHOP_IFINDEX:
-	      case ZEBRA_NEXTHOP_IFNAME:
-		stream_putl (s, nexthop->ifindex);
-		break;
-	      default:
-                /* do nothing */
-		break;
-	      }
-	    num++;
-	  }
-      stream_putc_at (s, nump, num);
+      rib = rib_match_ipv4_multicast (addr, NULL, vrf_id);
+      /* handle the distance field here since
+       * it is only needed for MRIB command */
+      if (rib)
+	stream_putc (s, rib->distance);
+      else
+        stream_putc (s, 0); /* distance */
     }
-  else
-    {
-      if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
-        zlog_debug("%s: No matching rib entry found.", __func__);
-      stream_putl (s, 0);
-      stream_putc (s, 0);
-    }
-
-  stream_putw_at (s, 0, stream_get_endp (s));
-  
-  return zebra_server_send_message(client);
-}
-
-/*
-  Modified version of zsend_ipv4_nexthop_lookup():
-  Query unicast rib if nexthop is not found on mrib.
-  Returns both route metric and protocol distance.
-*/
-static int
-zsend_ipv4_nexthop_lookup_mrib (struct zserv *client, struct in_addr addr,
-				struct rib *rib)
-{
-  struct stream *s;
-  unsigned long nump;
-  u_char num;
-  struct nexthop *nexthop;
-
-  /* Get output stream. */
-  s = client->obuf;
-  stream_reset (s);
-
-  /* Fill in result. */
-  zserv_create_header (s, ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB, 
-		       rib ? rib->vrf_id : VRF_DEFAULT);
-  stream_put_in_addr (s, &addr);
 
   if (rib)
     {
-      stream_putc (s, rib->distance);
+      if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
+        zlog_debug("%s: Matching rib entry found.", __func__);
       stream_putl (s, rib->metric);
       num = 0;
       nump = stream_get_endp(s); /* remember position for nexthop_num */
@@ -663,12 +608,13 @@  zsend_ipv4_nexthop_lookup_mrib (struct zserv *client, struct in_addr addr,
 	      }
 	    num++;
 	  }
-    
+
       stream_putc_at (s, nump, num); /* store nexthop_num */
     }
   else
     {
-      stream_putc (s, 0); /* distance */
+      if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
+        zlog_debug("%s: No matching rib entry found.", __func__);
       stream_putl (s, 0); /* metric */
       stream_putc (s, 0); /* nexthop_num */
     }
@@ -728,6 +674,7 @@  zsend_ipv4_import_lookup (struct zserv *client, struct prefix_ipv4 *p,
 	      }
 	    num++;
 	  }
+
       stream_putc_at (s, nump, num);
     }
   else
@@ -994,7 +941,7 @@  zread_ipv4_delete (struct zserv *client, u_short length, vrf_id_t vrf_id)
 
 /* Nexthop lookup for IPv4. */
 static int
-zread_ipv4_nexthop_lookup (struct zserv *client, u_short length,
+zread_ipv4_nexthop_lookup (int cmd, struct zserv *client, u_short length,
     vrf_id_t vrf_id)
 {
   struct in_addr addr;
@@ -1004,20 +951,8 @@  zread_ipv4_nexthop_lookup (struct zserv *client, u_short length,
   if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV)
     zlog_debug("%s: looking up %s", __func__,
                inet_ntop (AF_INET, &addr, buf, BUFSIZ));
-  return zsend_ipv4_nexthop_lookup (client, addr, vrf_id);
-}
-
-/* MRIB Nexthop lookup for IPv4. */
-static int
-zread_ipv4_nexthop_lookup_mrib (struct zserv *client, u_short length,
-    vrf_id_t vrf_id)
-{
-  struct in_addr addr;
-  struct rib *rib;
 
-  addr.s_addr = stream_get_ipv4 (client->ibuf);
-  rib = rib_match_ipv4_multicast (addr, NULL, vrf_id);
-  return zsend_ipv4_nexthop_lookup_mrib (client, addr, rib);
+  return zsend_ipv4_nexthop_lookup (client, addr, cmd, vrf_id);
 }
 
 /* Nexthop lookup for IPv4. */
@@ -1486,10 +1421,8 @@  zebra_client_read (struct thread *thread)
       zebra_redistribute_default_delete (command, client, length, vrf_id);
       break;
     case ZEBRA_IPV4_NEXTHOP_LOOKUP:
-      zread_ipv4_nexthop_lookup (client, length, vrf_id);
-      break;
     case ZEBRA_IPV4_NEXTHOP_LOOKUP_MRIB:
-      zread_ipv4_nexthop_lookup_mrib (client, length, vrf_id);
+      zread_ipv4_nexthop_lookup (command, client, length, vrf_id);
       break;
 #ifdef HAVE_IPV6
     case ZEBRA_IPV6_NEXTHOP_LOOKUP: