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

David Lamparter equinox at opensourcerouting.org
Sun Jun 29 11:40:34 BST 2014


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

+    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)

[...]
> +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.

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


Other than these, I really like the added functionality.

-David




More information about the Quagga-dev mailing list