[quagga-dev 11377] Re: [PATCH] bgpd: refactor route-map objects modifying integer values

Timo Teras timo.teras at iki.fi
Mon Jun 30 11:19:53 BST 2014


Thanks for the review!

On Sun, 29 Jun 2014 12:40:34 +0200
David Lamparter <david at opensourcerouting.org> wrote:

> On Sun, Jun 29, 2014 at 12:24:07PM +0200, David Lamparter wrote:
> > Use common code to parse, validate and adjust the route-map
> > objects that contain a simple integer value. This also allows
> > compiling the add/sub format metric object.
> [...]
> > +    case RMAP_VALUE_ADD:
> > +      if (current/2 + rv->value/2 > UINT32_MAX/2)
> > +        return UINT32_MAX;
> 
> There are many ways to test overflow, sadly this is not a correct
> one ;) it fails for 0x80000001 + 0x7fffffff, which turns into
> ...          0x40000000 + 0x3fffffff = 0x7fffffff, which is exactly
> UINT32_MAX/2, not greater... but will wrap to 0.
> 
> Since we have all-unsigned operands here, we can do it the simple way
> (since unsigned overflow is not permitted to be "optimized away" by
> overzealous compilers like gcc...):

This was copied from the original code, and I did not pay attention to
the correctness of it.

> +    case RMAP_VALUE_ADD:
> +      /* relies on ISO C unsigned overflow */
> +      if (current + rv->value < current)
> +        return UINT32_MAX;
> 
> (The comment is important so no one accidentally changes the types
> into something signed)

Same here, copied from original code.

> [...]
> > +route_value_compile (const char *arg)
> > +{
> > +  u_int8_t action = RMAP_VALUE_SET;
> > +  unsigned long larg;
> > +  char *endptr = NULL;
> > +  struct rmap_value *rv;
> > +
> > +  if (arg[0] == '+')
> > +    {
> > +      action = RMAP_VALUE_ADD;
> > +      arg++;
> > +    }
> > +  else if (arg[0] == '-')
> > +    {
> > +      action = RMAP_VALUE_SUB;
> > +      arg++;
> > +    }
> > +
> > +  errno = 0;
> > +  larg = strtoul (arg, &endptr, 10);
> > +  if (*endptr != '\0' || errno || larg > UINT32_MAX)
> > +    return NULL;
> 
> strtoul won't indicate an error if the input string is empty,
> therefore this function will accept "+", "-" or "" as 0.  I don't
> think we want that behaviour.

Ok, will add additional check for it.

> [...]
> >  route_set_local_pref (void *rule, struct prefix *prefix,
> >  		      route_map_object_t type, void *object)
> [...]
> >        /* Set local preference value. */ 
> >        bgp_info->attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF);
> > -      bgp_info->attr->local_pref = *local_pref;
> > +      bgp_info->attr->local_pref = route_value_adjust(rv, 0);
> 
> While we can't receive a locpref from a BGP session, we can have a
> value set from previous route-map rules.  This should support
> modifying an existing value.

Oh, ok. That true. Will add that too.

> Other than these, I really like the added functionality.

Great. I'll try to find the time to update my tree sometime this week.

Thanks,
Timo




More information about the Quagga-dev mailing list