bgpd: general MP/SAFI improvements

This fixes some minor mixups particularly in MPLS-related SAFIs, as well
as doing some stylistic changes & adding comments.

Signed-off-by: Lou Berger <lberger@labn.net>
Reviewed-by: David Lamparter <equinox@opensourcerouting.org>

8 files changed, 98 insertions, 64 deletions

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index ecf1c314..a25ea764 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1613,7 +1613,7 @@ bgp_mp_reach_parse (struct bgp_attr_parser_args *args,
  
   if (safi != SAFI_MPLS_LABELED_VPN)
     {
-      ret = bgp_nlri_sanity_check (peer, afi, stream_pnt (s), nlri_len);
+      ret = bgp_nlri_sanity_check (peer, afi, safi, stream_pnt (s), nlri_len);
       if (ret < 0) 
         {
           zlog_info ("%s: (%s) NLRI doesn't pass sanity check",
@@ -1662,7 +1662,7 @@ bgp_mp_unreach_parse (struct bgp_attr_parser_args *args,
 
   if (safi != SAFI_MPLS_LABELED_VPN)
     {
-      ret = bgp_nlri_sanity_check (peer, afi, stream_pnt (s), withdraw_len);
+      ret = bgp_nlri_sanity_check (peer, afi, safi, stream_pnt (s), withdraw_len);
       if (ret < 0)
 	return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
     }
@@ -2160,8 +2160,9 @@ bgp_packet_mpattr_start (struct stream *s, afi_t afi, safi_t safi,
   stream_putc (s, BGP_ATTR_MP_REACH_NLRI);
   sizep = stream_get_endp (s);
   stream_putw (s, 0);	/* Marker: Attribute length. */
-  stream_putw (s, afi);	/* AFI */
-  stream_putc (s, safi);	/* SAFI */
+
+  stream_putw (s, afi);
+  stream_putc (s, (safi == SAFI_MPLS_VPN) ? SAFI_MPLS_LABELED_VPN : safi);
 
   /* Nexthop */
   switch (afi)
@@ -2169,17 +2170,17 @@ bgp_packet_mpattr_start (struct stream *s, afi_t afi, safi_t safi,
     case AFI_IP:
       switch (safi)
 	{
-	case SAFI_UNICAST:
 	case SAFI_MULTICAST:
 	  stream_putc (s, 4);
 	  stream_put_ipv4 (s, attr->nexthop.s_addr);
 	  break;
 	case SAFI_MPLS_VPN:
 	  stream_putc (s, 12);
-	  stream_putl (s, 0);
+	  stream_putl (s, 0);   /* RD = 0, per RFC */
 	  stream_putl (s, 0);
 	  stream_put (s, &attr->extra->mp_nexthop_global_in, 4);
 	  break;
+	case SAFI_UNICAST:      /* invalid for IPv4 */
 	default:
 	  break;
 	}
@@ -2199,6 +2200,28 @@ bgp_packet_mpattr_start (struct stream *s, afi_t afi, safi_t safi,
 	  if (attre->mp_nexthop_len == 32)
 	    stream_put (s, &attre->mp_nexthop_local, 16);
 	}
+	break;
+      case SAFI_MPLS_VPN:
+	{
+	  struct attr_extra *attre = attr->extra;
+
+	  assert (attr->extra);
+          if (attre->mp_nexthop_len == 16) {
+            stream_putc (s, 24);
+            stream_putl (s, 0);   /* RD = 0, per RFC */
+            stream_putl (s, 0);
+            stream_put (s, &attre->mp_nexthop_global, 16);
+          } else if (attre->mp_nexthop_len == 32) {
+            stream_putc (s, 48);
+            stream_putl (s, 0);   /* RD = 0, per RFC */
+            stream_putl (s, 0);
+            stream_put (s, &attre->mp_nexthop_global, 16);
+            stream_putl (s, 0);   /* RD = 0, per RFC */
+            stream_putl (s, 0);
+            stream_put (s, &attre->mp_nexthop_local, 16);
+          }
+        }
+	break;
       default:
 	break;
       }
@@ -2218,20 +2241,25 @@ bgp_packet_mpattr_prefix (struct stream *s, afi_t afi, safi_t safi,
 			  struct prefix *p, struct prefix_rd *prd,
 			  u_char *tag)
 {
-  switch (safi)
+  if (safi == SAFI_MPLS_VPN)
     {
-    case SAFI_MPLS_VPN:
       /* Tag, RD, Prefix write. */
       stream_putc (s, p->prefixlen + 88);
       stream_put (s, tag, 3);
       stream_put (s, prd->val, 8);
       stream_put (s, &p->u.prefix, PSIZE (p->prefixlen));
-      break;
-    default:
-      /* Prefix write. */
-      stream_put_prefix (s, p);
-      break;
     }
+  else
+    stream_put_prefix (s, p);
+}
+
+size_t
+bgp_packet_mpattr_prefix_size (afi_t afi, safi_t safi, struct prefix *p)
+{
+  int size = PSIZE (p->prefixlen);
+  if (safi == SAFI_MPLS_VPN)
+      size += 88;
+  return size;
 }
 
 void
@@ -2255,7 +2283,6 @@ bgp_packet_attribute (struct bgp *bgp, struct peer *peer,
   int send_as4_path = 0;
   int send_as4_aggregator = 0;
   int use32bit = (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) ? 1 : 0;
-  size_t mpattrlen_pos = 0;
 
   if (! bgp)
     bgp = bgp_get_default ();
@@ -2265,6 +2292,7 @@ bgp_packet_attribute (struct bgp *bgp, struct peer *peer,
 
   if (p && !(afi == AFI_IP && safi == SAFI_UNICAST))
     {
+      size_t mpattrlen_pos = 0;
       mpattrlen_pos = bgp_packet_mpattr_start(s, afi, safi, attr);
       bgp_packet_mpattr_prefix(s, afi, safi, p, prd, tag);
       bgp_packet_mpattr_end(s, mpattrlen_pos);
@@ -2338,7 +2366,8 @@ bgp_packet_attribute (struct bgp *bgp, struct peer *peer,
       send_as4_path = 1; /* we'll do this later, at the correct place */
   
   /* Nexthop attribute. */
-  if (attr->flag & ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP) && afi == AFI_IP)
+  if (attr->flag & ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP) && afi == AFI_IP &&
+    safi ==  SAFI_UNICAST)   /* only write NH attr for unicast safi */
     {
       stream_putc (s, BGP_ATTR_FLAG_TRANS);
       stream_putc (s, BGP_ATTR_NEXT_HOP);
@@ -2612,8 +2641,7 @@ bgp_packet_mpunreach_start (struct stream *s, afi_t afi, safi_t safi)
   stream_putw (s, 0);		/* Length of this attribute. */
 
   stream_putw (s, afi);
-  safi = (safi == SAFI_MPLS_VPN) ? SAFI_MPLS_LABELED_VPN : safi;
-  stream_putc (s, safi);
+  stream_putc (s, (safi == SAFI_MPLS_VPN) ? SAFI_MPLS_LABELED_VPN : safi);
   return attrlen_pnt;
 }
 
@@ -2622,26 +2650,13 @@ bgp_packet_mpunreach_prefix (struct stream *s, struct prefix *p,
 			     afi_t afi, safi_t safi, struct prefix_rd *prd,
 			     u_char *tag)
 {
-  if (safi == SAFI_MPLS_VPN)
-    {
-      stream_putc (s, p->prefixlen + 88);
-      stream_put (s, tag, 3);
-      stream_put (s, prd->val, 8);
-      stream_put (s, &p->u.prefix, PSIZE (p->prefixlen));
-    }
-  else
-    stream_put_prefix (s, p);
+  bgp_packet_mpattr_prefix (s, afi, safi, p, prd, tag);
 }
 
 void
 bgp_packet_mpunreach_end (struct stream *s, size_t attrlen_pnt)
 {
-  bgp_size_t size;
-
-  /* Set MP attribute length. Don't count the (2) bytes used to encode
-     the attr length */
-  size = stream_get_endp (s) - attrlen_pnt - 2;
-  stream_putw_at (s, attrlen_pnt, size);
+  bgp_packet_mpattr_end (s, attrlen_pnt);
 }
 
 /* Initialization of attribute. */
diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index b59fa8e4..6fdf1c1a 100644
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -205,6 +205,8 @@ extern size_t bgp_packet_mpattr_start(struct stream *s, afi_t afi, safi_t safi,
 extern void bgp_packet_mpattr_prefix(struct stream *s, afi_t afi, safi_t safi,
 				     struct prefix *p, struct prefix_rd *prd,
 				     u_char *tag);
+extern size_t bgp_packet_mpattr_prefix_size(afi_t afi, safi_t safi,
+                                            struct prefix *p);
 extern void bgp_packet_mpattr_end(struct stream *s, size_t sizep);
 
 extern size_t bgp_packet_mpunreach_start (struct stream *s, afi_t afi,
diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c
index 78a71e70..e9226651 100644
--- a/bgpd/bgp_mplsvpn.c
+++ b/bgpd/bgp_mplsvpn.c
@@ -178,13 +178,10 @@ bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr,
           decode_rd_ip (pnt + 5, &rd_ip);
           break;
 
-        case RD_TYPE_EOI:
-          break;
-
-        default:
-          zlog_err ("Invalid RD type %d", type);
-          return -1;
-        }
+	default:
+	  zlog_err ("Unknown RD type %d", type);
+          break;  /* just report */
+      }
 
       p.prefixlen = prefixlen - VPN_PREFIXLEN_MIN_BYTES*8;
       memcpy (&p.u.prefix, pnt + VPN_PREFIXLEN_MIN_BYTES, 
@@ -323,14 +320,6 @@ prefix_rd2str (struct prefix_rd *prd, char *buf, size_t size)
       snprintf (buf, size, "%s:%d", inet_ntoa (rd_ip.ip), rd_ip.val);
       return buf;
     }
-  else if (type == RD_TYPE_EOI)
-    {
-      snprintf(buf, size, "LHI:%d, %02x:%02x:%02x:%02x:%02x:%02x",
-               pnt[1], /* LHI */
-               pnt[2], pnt[3], pnt[4], pnt[5], pnt[6], pnt[7]); /* MAC */
-      return buf;
-    }
-
   return NULL;
 }
 
diff --git a/bgpd/bgp_mplsvpn.h b/bgpd/bgp_mplsvpn.h
index 4e9b317b..3299b9cb 100644
--- a/bgpd/bgp_mplsvpn.h
+++ b/bgpd/bgp_mplsvpn.h
@@ -24,7 +24,6 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 #define RD_TYPE_AS      0
 #define RD_TYPE_IP      1
 #define RD_TYPE_AS4     2
-#define RD_TYPE_EOI	0xff00
 
 #define RD_ADDRSTRLEN  28
 
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 16e1435a..841eaab8 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -172,16 +172,24 @@ bgp_update_packet (struct peer *peer, afi_t afi, safi_t safi)
 
       /* When remaining space can't include NLRI and it's length.  */
       if (STREAM_CONCAT_REMAIN (s, snlri, STREAM_SIZE(s)) <=
-	  (BGP_NLRI_LENGTH + PSIZE (rn->p.prefixlen)))
+	  (BGP_NLRI_LENGTH + bgp_packet_mpattr_prefix_size(afi,safi,&rn->p)))
 	break;
 
       /* If packet is empty, set attribute. */
       if (stream_empty (s))
 	{
+	  struct prefix_rd *prd = NULL;
+	  u_char *tag = NULL;
 	  struct peer *from = NULL;
 
+	  if (rn->prn)
+	    prd = (struct prefix_rd *) &rn->prn->p;
           if (binfo)
-	    from = binfo->peer;
+            {
+              from = binfo->peer;
+              if (binfo->extra)
+                tag = binfo->extra->tag;
+            }
 
 	  /* 1: Write the BGP message header - 16 bytes marker, 2 bytes length,
 	   * one byte message type.
@@ -204,8 +212,8 @@ bgp_update_packet (struct peer *peer, afi_t afi, safi_t safi)
 	  /* 5: Encode all the attributes, except MP_REACH_NLRI attr. */
 	  total_attr_len = bgp_packet_attribute (NULL, peer, s,
 	                                         adv->baa->attr,
-	                                         NULL, afi, safi,
-	                                         from, NULL, NULL);
+	                                         &rn->p, afi, safi,
+	                                         from, prd, tag);
 	}
 
       if (afi == AFI_IP && safi == SAFI_UNICAST)
@@ -1653,7 +1661,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
   /* Unfeasible Route packet format check. */
   if (withdraw_len > 0)
     {
-      ret = bgp_nlri_sanity_check (peer, AFI_IP, stream_pnt (s), withdraw_len);
+      ret = bgp_nlri_sanity_check (peer, AFI_IP, SAFI_UNICAST, stream_pnt (s), withdraw_len);
       if (ret < 0)
 	return -1;
 
@@ -1713,6 +1721,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
       if (attr_parse_ret == BGP_ATTR_PARSE_ERROR)
 	{
 	  bgp_attr_unintern_sub (&attr);
+          bgp_attr_flush (&attr);
 	  return -1;
 	}
     }
@@ -1744,10 +1753,11 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
   if (update_len)
     {
       /* Check NLRI packet format and prefix length. */
-      ret = bgp_nlri_sanity_check (peer, AFI_IP, stream_pnt (s), update_len);
+      ret = bgp_nlri_sanity_check (peer, AFI_IP, SAFI_UNICAST, stream_pnt (s), update_len);
       if (ret < 0)
         {
           bgp_attr_unintern_sub (&attr);
+          bgp_attr_flush (&attr);
 	  return -1;
 	}
 
@@ -1933,6 +1943,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
   /* Everything is done.  We unintern temporary structures which
      interned in bgp_attr_parse(). */
   bgp_attr_unintern_sub (&attr);
+  bgp_attr_flush (&attr);
 
   /* If peering is stopped due to some reason, do not generate BGP
      event.  */
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 868fbe56..b024d80d 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -1480,6 +1480,9 @@ bgp_process_announce_selected (struct peer *peer, struct bgp_info *selected,
   struct attr attr;
   struct attr_extra extra;
 
+  memset (&attr, 0, sizeof(struct attr));
+  memset (&extra, 0, sizeof(struct attr_extra));
+
   p = &rn->p;
 
   /* Announce route to Established peer. */
@@ -1519,6 +1522,7 @@ bgp_process_announce_selected (struct peer *peer, struct bgp_info *selected,
         break;
     }
 
+  bgp_attr_flush (&attr);
   return 0;
 }
 
@@ -1661,7 +1665,7 @@ bgp_process_main (struct work_queue *wq, void *data)
 	}
     }
     
-  /* Reap old select bgp_info, it it has been removed */
+  /* Reap old select bgp_info, if it has been removed */
   if (old_select && CHECK_FLAG (old_select->flags, BGP_INFO_REMOVED))
     bgp_info_reap (rn, old_select);
   
@@ -1869,7 +1873,7 @@ bgp_rib_remove (struct bgp_node *rn, struct bgp_info *ri, struct peer *peer,
 
 static void
 bgp_rib_withdraw (struct bgp_node *rn, struct bgp_info *ri, struct peer *peer,
-		  afi_t afi, safi_t safi)
+		  afi_t afi, safi_t safi, struct prefix_rd *prd)
 {
   int status = BGP_DAMP_NONE;
 
@@ -1991,7 +1995,7 @@ bgp_update_rsclient (struct peer *rsclient, afi_t afi, safi_t safi,
 
           bgp_unlock_node (rn);
           bgp_attr_unintern (&attr_new);
-
+          bgp_attr_flush(&new_attr);
           return;
         }
 
@@ -2099,7 +2103,7 @@ bgp_withdraw_rsclient (struct peer *rsclient, afi_t afi, safi_t safi,
 
   /* Withdraw specified route from routing table. */
   if (ri && ! CHECK_FLAG (ri->flags, BGP_INFO_HISTORY))
-    bgp_rib_withdraw (rn, ri, peer, afi, safi);
+    bgp_rib_withdraw (rn, ri, peer, afi, safi, prd);
   else if (BGP_DEBUG (update, UPDATE_IN))
     zlog (peer->log, LOG_DEBUG,
           "%s Can't find the route %s/%d", peer->host,
@@ -2274,6 +2278,7 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
 
 	  bgp_unlock_node (rn);
 	  bgp_attr_unintern (&attr_new);
+          bgp_attr_flush (&new_attr);
 
 	  return 0;
 	}
@@ -2326,6 +2331,8 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
       if (safi == SAFI_MPLS_VPN)
         memcpy ((bgp_info_extra_get (ri))->tag, tag, 3);
 
+      bgp_attr_flush (&new_attr);
+
       /* Update bgp route dampening information.  */
       if (CHECK_FLAG (bgp->af_flags[afi][safi], BGP_CONFIG_DAMPENING)
 	  && peer->sort == BGP_PEER_EBGP)
@@ -2355,6 +2362,8 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
       else
         bgp_info_set_flag (rn, ri, BGP_INFO_VALID);
 
+      bgp_attr_flush (&new_attr);
+
       /* Process change. */
       bgp_aggregate_increment (bgp, p, ri, afi, safi);
 
@@ -2410,6 +2419,8 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
   /* route_node_get lock */
   bgp_unlock_node (rn);
 
+  bgp_attr_flush (&new_attr);
+
   /* If maximum prefix count is configured and current prefix
      count exeed it. */
   if (bgp_maximum_prefix_overflow (peer, afi, safi, 0))
@@ -2434,6 +2445,7 @@ bgp_update_main (struct peer *peer, struct prefix *p, struct attr *attr,
     bgp_rib_remove (rn, ri, peer, afi, safi);
 
   bgp_unlock_node (rn);
+  bgp_attr_flush (&new_attr);
 
   return 0;
 }
@@ -2520,7 +2532,7 @@ bgp_withdraw (struct peer *peer, struct prefix *p, struct attr *attr,
 
   /* Withdraw specified route from routing table. */
   if (ri && ! CHECK_FLAG (ri->flags, BGP_INFO_HISTORY))
-    bgp_rib_withdraw (rn, ri, peer, afi, safi);
+    bgp_rib_withdraw (rn, ri, peer, afi, safi, prd);
   else if (BGP_DEBUG (update, UPDATE_IN))
     zlog (peer->log, LOG_DEBUG, 
 	  "%s Can't find the route %s/%d", peer->host,
@@ -3006,8 +3018,7 @@ bgp_clear_route (struct peer *peer, afi_t afi, safi_t safi,
    * unlock happens at the end of this function.
    */
   if (!peer->clear_node_queue->thread)
-    peer_lock (peer);
-
+    peer_lock (peer); /* bgp_clear_node_complete */
   switch (purpose)
     {
     case BGP_CLEAR_ROUTE_NORMAL:
@@ -3026,6 +3037,11 @@ bgp_clear_route (struct peer *peer, afi_t afi, safi_t safi,
       break;
 
     case BGP_CLEAR_ROUTE_MY_RSCLIENT:
+      /*
+       * gpz 091009: TBD why don't we have special handling for
+       * SAFI_MPLS_VPN here in the original quagga code?
+       * (and, by extension, for SAFI_ENCAP)
+       */
       bgp_clear_route_table (peer, afi, safi, NULL, peer, purpose);
       break;
 
@@ -3304,8 +3320,8 @@ bgp_nlri_parse (struct peer *peer, struct attr *attr, struct bgp_nlri *packet)
 
 /* NLRI encode syntax check routine. */
 int
-bgp_nlri_sanity_check (struct peer *peer, int afi, u_char *pnt,
-		       bgp_size_t length)
+bgp_nlri_sanity_check (struct peer *peer, int afi, safi_t safi,
+                       u_char *pnt, bgp_size_t length)
 {
   u_char *end;
   u_char prefixlen;
@@ -6739,6 +6755,7 @@ bgp_show_route_in_table (struct vty *vty, struct bgp *bgp,
   struct bgp_info *ri;
   struct bgp_table *table;
 
+  memset (&match, 0, sizeof (struct prefix)); /* keep valgrind happy */
   /* Check IP address argument. */
   ret = str2prefix (ip_str, &match);
   if (! ret)
diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
index 5a93b445..8e9bcbd1 100644
--- a/bgpd/bgp_route.h
+++ b/bgpd/bgp_route.h
@@ -196,7 +196,7 @@ extern struct bgp_info_extra *bgp_info_extra_get (struct bgp_info *);
 extern void bgp_info_set_flag (struct bgp_node *, struct bgp_info *, u_int32_t);
 extern void bgp_info_unset_flag (struct bgp_node *, struct bgp_info *, u_int32_t);
 
-extern int bgp_nlri_sanity_check (struct peer *, int, u_char *, bgp_size_t);
+extern int bgp_nlri_sanity_check (struct peer *, int, safi_t, u_char *, bgp_size_t);
 extern int bgp_nlri_parse (struct peer *, struct attr *, struct bgp_nlri *);
 
 extern int bgp_maximum_prefix_overflow (struct peer *, afi_t, safi_t, int);
diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c
index b449cae0..a6d5a0e1 100644
--- a/bgpd/bgp_routemap.c
+++ b/bgpd/bgp_routemap.c
@@ -2090,6 +2090,7 @@ route_set_vpnv4_nexthop (void *rule, struct prefix *prefix,
     
       /* Set next hop value. */ 
       (bgp_attr_extra_get (bgp_info->attr))->mp_nexthop_global_in = *address;
+      (bgp_attr_extra_get (bgp_info->attr))->mp_nexthop_len = 4;
     }
 
   return RMAP_OKAY;