Vivek, On 1/5/2016 8:13 AM, Vivek Venkatraman wrote: > > On Thu, Dec 24, 2015 at 10:10 AM, Lou Berger <[email protected] > <mailto:[email protected]>> wrote: > > This is part of the core VPN and Encap SAFI changes. > > This fixes some minor mixups particularly in MPLS-related SAFIs, > as well > as doing some stylistic changes & adding comments. > > > I feel it would be good to separate the very specific fixes (related > to SAFI values) from the other changes - just an opinion. > fair enough -- I thing your and David's styles are similar. I tend to aggregate fixes and accept that I have to resist this tendency here...
> > Signed-off-by: Lou Berger <[email protected] <mailto:[email protected]>> > Reviewed-by: David Lamparter <[email protected] > <mailto:[email protected]>> > --- > @@ -2159,8 +2159,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) > @@ -2168,14 +2169,16 @@ bgp_packet_mpattr_start (struct stream *s, > afi_t afi, safi_t safi, > case AFI_IP: > switch (safi) > { > +#if 0 /* LB: doesn't exist */ > case SAFI_UNICAST: > +#endif > > > Just as a FYI, we have changed this part of the code as part of > implementing support for RFC 5549, where IPv4 prefixes can be > exchanged as MP NLRI with IPv6 nexthops. One can be reworked on top of > the other. We can drop this change as it does no harm to include the case. > > > case SAFI_MULTICAST: > stream_putc (s, 4); > stream_put_ipv4 (s, attr->nexthop.s_addr); > break; > > > > > @@ -2254,7 +2284,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 (); > @@ -2262,13 +2291,15 @@ bgp_packet_attribute (struct bgp *bgp, > struct peer *peer, > /* Remember current pointer. */ > cp = stream_get_endp (s); > > +#if 0 /* duplicates mpattr included in > bgp_packet.c */ > 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); > } > - > +#endif > > > This part of the code (which has been placed under "#if 0") is used by > the default originate code. I'm not sure how that will work after this > change. Can you please check? > yes, encap and vpn don't need this change. Not sure why I left the if 0 vs dropping the code. > > diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c > index 35c4719..9d88e11 100644 > --- a/bgpd/bgp_packet.c > +++ b/bgpd/bgp_packet.c > @@ -171,16 +171,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. > @@ -203,8 +211,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); > } > > > I don't see the need to pass the prefix in the above call; in fact, > since bgp_packet_attribute() is meant to set the non-prefix attributes > of the update, I feel the prefix shouldn't be passed. With the #if 0 code removed, I agree it's not needed. I'll make this change assuming no objections. Lou > The only case it is needed is in default originate > (default_update_send) as I mentioned earlier --- and ideally, that > function should be reworked to put the prefix itself. > > _______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
