On Fri, Jun 17, 2016 at 4:58 AM, Paul Jakma <[email protected]> wrote:
> Hi,
>
> Is it just me, or does this patch leak attr->aspath? I've highlighted the
> bits that made me ask that. Though, to be fair, if so then it looks like
> the old remove-private-as also leaked.
>
> I'd suggest separating lifetime management from the manipulation. Have
> aspath_replace_private_asns and aspath_remove_private_asns just act on the
> given aspath, and have the caller handle lifetime.
>
>
Agreed on both...looks like this is leaking and it seems better to just
have these modify the aspath instead of creating a dup that is modified.
Daniel
> regards,
>
> Paul
>
> On Fri, 11 Dec 2015, Donald Sharp wrote:
>
> +/* Replace all private ASNs with our own ASN */
>> +struct aspath *
>> +aspath_replace_private_asns (struct aspath *aspath, as_t asn)
>> +{
>> + struct aspath *new;
>> + struct assegment *seg;
>> +
>> + new = aspath_dup(aspath);
>>
> ^^^^^^^^^^^^^^^^^^^^^^^^^
>
>> + seg = new->segments;
>> +
>> + while (seg)
>> + {
>> + int i;
>> +
>> + for (i = 0; i < seg->length; i++)
>> + {
>> + if (BGP_AS_IS_PRIVATE(seg->as[i]))
>> + seg->as[i] = asn;
>> + }
>> + seg = seg->next;
>> + }
>> +
>> + aspath_str_update(new);
>> + return new;
>> +}
>> +
>> +/* Remove all private ASNs */
>> +struct aspath *
>> +aspath_remove_private_asns (struct aspath *aspath)
>> +{
>> + struct aspath *new;
>> + struct assegment *seg;
>> + struct assegment *new_seg;
>> + struct assegment *last_new_seg;
>> + int i;
>> + int j;
>> + int public = 0;
>> +
>> + new = XCALLOC (MTYPE_AS_PATH, sizeof (struct aspath));
>> +
>> + new_seg = NULL;
>> + last_new_seg = NULL;
>> + seg = aspath->segments;
>> + while (seg)
>> + {
>> + public = 0;
>> + for (i = 0; i < seg->length; i++)
>> + {
>> + // ASN is public
>> + if (!BGP_AS_IS_PRIVATE(seg->as[i]))
>> + {
>> + public++;
>> + }
>> + }
>> +
>> + // The entire segment is private so skip it
>> + if (!public)
>> + {
>> + seg = seg->next;
>> + continue;
>> + }
>> +
>> + // The entire segment is public so copy it
>> + else if (public == seg->length)
>> + {
>> + new_seg = assegment_dup (seg);
>> + }
>> +
>> + // The segment is a mix of public and private ASNs. Copy as many
>> spots as
>> + // there are public ASNs then come back and fill in only the
>> public ASNs.
>> + else
>> + {
>> + new_seg = assegment_new (seg->type, public);
>> + j = 0;
>> + for (i = 0; i < seg->length; i++)
>> + {
>> + // ASN is public
>> + if (!BGP_AS_IS_PRIVATE(seg->as[i]))
>> + {
>> + new_seg->as[j] = seg->as[i];
>> + j++;
>> + }
>> + }
>> + }
>> +
>> + // This is the first segment so set the aspath segments pointer to
>> this one
>> + if (!last_new_seg)
>> + new->segments = new_seg;
>> + else
>> + last_new_seg->next = new_seg;
>> +
>> + last_new_seg = new_seg;
>> + seg = seg->next;
>> + }
>> +
>> + aspath_str_update(new);
>> + return new;
>> +}
>> +
>>
>
> +/* If this is an EBGP peer with remove-private-AS */
>> +static void
>> +bgp_peer_remove_private_as(struct bgp *bgp, afi_t afi, safi_t safi,
>> + struct peer *peer, struct attr *attr)
>> +{
>> + if (peer->sort == BGP_PEER_EBGP &&
>> + peer_af_flag_check (peer, afi, safi, PEER_FLAG_REMOVE_PRIVATE_AS))
>> + {
>> + // Take action on the entire aspath
>> + if (peer_af_flag_check (peer, afi, safi,
>> PEER_FLAG_REMOVE_PRIVATE_AS_ALL))
>> + {
>> + if (peer_af_flag_check (peer, afi, safi,
>> PEER_FLAG_REMOVE_PRIVATE_AS_REPLACE))
>> + attr->aspath = aspath_replace_private_asns (attr->aspath,
>> bgp->as);
>>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>> +
>> + // The entire aspath consists of private ASNs so create an
>> empty aspath
>> + else if (aspath_private_as_check (attr->aspath))
>> + attr->aspath = aspath_empty_get ();
>>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>> +
>> + // There are some public and some private ASNs, remove the
>> private ASNs
>> + else
>> + attr->aspath = aspath_remove_private_asns (attr->aspath);
>>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>> + }
>> +
>> + // 'all' was not specified so the entire aspath must be private
>> ASNs
>> + // for us to do anything
>> + else if (aspath_private_as_check (attr->aspath))
>> + {
>> + if (peer_af_flag_check (peer, afi, safi,
>> PEER_FLAG_REMOVE_PRIVATE_AS_REPLACE))
>> + attr->aspath = aspath_replace_private_asns (attr->aspath,
>> bgp->as);
>>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>> + else
>> + attr->aspath = aspath_empty_get ();
>>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>> + }
>>
>
> -
>> - /* If this is EBGP peer and remove-private-AS is set. */
>> - if (rsclient->sort == BGP_PEER_EBGP
>> - && peer_af_flag_check (rsclient, afi, safi,
>> PEER_FLAG_REMOVE_PRIVATE_AS)
>> - && aspath_private_as_check (attr->aspath))
>> - attr->aspath = aspath_empty_get ();
>> + bgp_peer_remove_private_as(bgp, afi, safi, rsclient, attr);
>>
>> regards,
> --
> Paul Jakma | [email protected] | @pjakma | Key ID: 0xD86BF79464A2FF6A
> Fortune:
> It is impossible to travel faster than light, and certainly not desirable,
> as one's hat keeps blowing off.
> -- Woody Allen
>
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
