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

Timo Teras timo.teras at iki.fi
Tue Oct 7 14:43:18 BST 2014


On Tue, 7 Oct 2014 14:30:34 +0100 (BST)
Paul Jakma <paul at jakma.org> wrote:

> 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.

You're free to amend to commit. I'm not sure if there's any guidelines
on this, but on my last commits the links were ok. But agreeable links
get old, so having the core in commit message + .texi is good.

> > 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?

I'm fine either way. This was just the easy way out when writing the
patch.

If you can update the changes it's perfect. I might be also able to
look at it later this week.




More information about the Quagga-dev mailing list