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.
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; the NLRI sanity also needs to be performed and should not be skipped
for the VPN SAFIs, correct?
On Tue, Jan 5, 2016 at 5:13 AM, Vivek Venkatraman <vi...@cumulusnetworks.com
> wrote:
>
> On Thu, Dec 24, 2015 at 10:10 AM, Lou Berger <lber...@labn.net> 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 <lber...@labn.net>
>> Reviewed-by: David Lamparter <equi...@opensourcerouting.org>
>> ---
>> @@ -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
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev
