[quagga-dev 11549] Re: [PATCH 1/1] bgpd: implement route-map set as-path prepend last-as

Paul Jakma paul at jakma.org
Tue Oct 7 14:30:34 BST 2014


Hi Timo,

Thanks for this. Just wondering if you might be able to take 2 small 
comments on this? See inline below.

On Tue, 30 Sep 2014, Timo Teräs wrote:

> It picks up the AS to add from the aspath, or uses the peers
> AS number. Useful mostly in iBGP setups.
>
> Cisco compatible syntax per:
> http://www.cisco.com/web/techdoc/dc/reference/cli/nxos/commands/bgp/set_as-path.html

Instead of referring to 3rd parties, could you just add a brief 
description to the bgpd.texi? Or you can just leave this out from the 
commit message and I'll extend bgpd.texi.


> diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c
> index c498f58..b0077e1 100644
> --- a/bgpd/bgp_routemap.c
> +++ b/bgpd/bgp_routemap.c
> @@ -1233,7 +1233,6 @@ route_set_aspath_prepend (void *rule, struct prefix *prefix, route_map_object_t
>
>   if (type == RMAP_BGP)
>     {
> -      aspath = rule;
>       binfo = object;
>
>       if (binfo->attr->aspath->refcnt)
> @@ -1241,20 +1240,50 @@ route_set_aspath_prepend (void *rule, struct prefix *prefix, route_map_object_t
>       else
> 	new = binfo->attr->aspath;
> 
> -      aspath_prepend (aspath, new);
> +      if ((uintptr_t)rule > 10)
> +      {
> +	aspath = rule;
> +	aspath_prepend (aspath, new);
> +      }
> +      else
> +      {
> +	as_t as = aspath_leftmost(new);
> +	if (!as) as = binfo->peer->as;
> +	new = aspath_add_seq_n (new, as, (uintptr_t) rule);
> +      }
> +
>       binfo->attr->aspath = new;
>     }
>
>   return RMAP_OKAY;
> }
> 
> +static void *
> +route_set_aspath_prepend_compile (const char *arg)
> +{
> +  unsigned int num;
> +
> +  if (sscanf(arg, "last-as %u", &num) == 1 && num > 0 && num < 10)
> +    return (void*)(uintptr_t)num;
> +
> +  return route_aspath_compile(arg);
> +}
> +
> +static void
> +route_set_aspath_prepend_free (void *rule)
> +{
> +  if ((uintptr_t)rule > 10)
> +    route_aspath_free(rule);
> +}

Not quite sure how I feel about the overloading of (void *)rule here. On 
the one hand, this does seems convenient. On the other hand, I wonder if 
it might just look a bit better to just go ahead and create a struct for 
this and dynamically allocate storage to hold a flag and potentially a 
short number?

regards,
-- 
Paul Jakma	paul at jakma.org	@pjakma	Key ID: 64A2FF6A
Fortune:
Seeing is believing.  You wouldn't have seen it if you hadn't believed it.


More information about the Quagga-dev mailing list