Vivek, On 1/5/2016 8:18 AM, Vivek Venkatraman wrote: > I missed a couple of other points. There are multiple bgp_attr_flush() > calls introduced in this patch. Can you elaborate on this? I looked at > a couple of calls and could not convince myself they were necessary. >
The changes run clean through valgrind. I couldn't swear that some are redundant (or not). If you have a question on a specific one, I can dig into why it was introduced. > This was another reason I had in my mind to state that this patch > should ideally be broken into more than 1 distinct patch. > > Also, in bgp_nlri_sanity_check(), I see some wrong/bad value for the > VPN SAFI; yes . > the NLRI sanity also needs to be performed and should not be skipped > for the VPN SAFIs, correct? > I think so. This test was added by Paul early on, I think it's obe and will confirm... Lou > On Tue, Jan 5, 2016 at 5:13 AM, Vivek Venkatraman > <[email protected] <mailto:[email protected]>> 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. > > > > 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. > > > 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? > > > > 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. > 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
