[quagga-dev 12205] Re: [PATCH 4/7] bgpd: refactor route-map objects modifying integer values

Donald Sharp sharpd at cumulusnetworks.com
Wed Apr 29 14:49:54 BST 2015


As an aside.  Is there a particular reason we have 5 different "set metric"
command handlers in the code base?

sharpd at monster-01:~/quagga-2$ find . -name '*.c' -print | xargs grep "DEFUN
(set_metric"
./bgpd/bgp_routemap.c:DEFUN (set_metric,
./ospf6d/ospf6_asbr.c:DEFUN (set_metric,
./ospfd/ospf_routemap.c:DEFUN (set_metric,
./ripd/rip_routemap.c:DEFUN (set_metric,
./ripngd/ripng_routemap.c:DEFUN (set_metric,

This causes issues with what is actually called due to the way the cli is
created in vtysh/extract.pl since it doesn't have anything to differentiate
what actual function to call.  I added debugs to my version and noticed
that the ospfd version is the one that is called(instead of all of them).
While it's ok or the normal 'set metric' command, bgp depends on the set
metric +/- command and it does not get parsed.

thanks!

donald

On Wed, Apr 29, 2015 at 2:43 AM, Timo Teräs <timo.teras at iki.fi> 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.
>
> Signed-off-by: Timo Teräs <timo.teras at iki.fi>
> ---
>  bgpd/bgp_routemap.c | 303
> +++++++++++++++++++---------------------------------
>  1 file changed, 111 insertions(+), 192 deletions(-)
>
> diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c
> index 416a3e5..4a0dac7 100644
> --- a/bgpd/bgp_routemap.c
> +++ b/bgpd/bgp_routemap.c
> @@ -101,6 +101,86 @@ o Local extensions
>
>  */
>
> + /* generic value manipulation to be shared in multiple rules */
> +
> +#define RMAP_VALUE_SET 0
> +#define RMAP_VALUE_ADD 1
> +#define RMAP_VALUE_SUB 2
> +
> +struct rmap_value
> +{
> +  u_int8_t action;
> +  u_int32_t value;
> +};
> +
> +static int
> +route_value_match (struct rmap_value *rv, u_int32_t value)
> +{
> +  if (value == rv->value)
> +    return RMAP_MATCH;
> +
> +  return RMAP_NOMATCH;
> +}
> +
> +static u_int32_t
> +route_value_adjust (struct rmap_value *rv, u_int32_t current)
> +{
> +  u_int32_t value = rv->value;
> +
> +  switch (rv->action)
> +    {
> +    case RMAP_VALUE_ADD:
> +      if (current > UINT32_MAX-value)
> +        return UINT32_MAX;
> +      return current + value;
> +    case RMAP_VALUE_SUB:
> +      if (current <= value)
> +        return 0;
> +      return current - value;
> +    default:
> +      return value;
> +    }
> +}
> +
> +static void *
> +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 (*arg == 0 || *endptr != 0 || errno || larg > UINT32_MAX)
> +    return NULL;
> +
> +  rv = XMALLOC (MTYPE_ROUTE_MAP_COMPILED, sizeof(struct rmap_value));
> +  if (!rv)
> +    return NULL;
> +
> +  rv->action = action;
> +  rv->value = larg;
> +  return rv;
> +}
> +
> +static void
> +route_value_free (void *rule)
> +{
> +  XFREE (MTYPE_ROUTE_MAP_COMPILED, rule);
> +}
> +
>   /* generic as path object to be shared in multiple rules */
>
>  static void *
> @@ -518,62 +598,25 @@ static route_map_result_t
>  route_match_metric (void *rule, struct prefix *prefix,
>                     route_map_object_t type, void *object)
>  {
> -  u_int32_t *med;
> +  struct rmap_value *rv;
>    struct bgp_info *bgp_info;
>
>    if (type == RMAP_BGP)
>      {
> -      med = rule;
> +      rv = rule;
>        bgp_info = object;
> -
> -      if (bgp_info->attr->med == *med)
> -       return RMAP_MATCH;
> -      else
> -       return RMAP_NOMATCH;
> +      return route_value_match(rv, bgp_info->attr->med);
>      }
>    return RMAP_NOMATCH;
>  }
>
> -/* Route map `match metric' match statement. `arg' is MED value */
> -static void *
> -route_match_metric_compile (const char *arg)
> -{
> -  u_int32_t *med;
> -  char *endptr = NULL;
> -  unsigned long tmpval;
> -
> -  /* Metric value shoud be integer. */
> -  if (! all_digit (arg))
> -    return NULL;
> -
> -  errno = 0;
> -  tmpval = strtoul (arg, &endptr, 10);
> -  if (*endptr != '\0' || errno || tmpval > UINT32_MAX)
> -    return NULL;
> -
> -  med = XMALLOC (MTYPE_ROUTE_MAP_COMPILED, sizeof (u_int32_t));
> -
> -  if (!med)
> -    return med;
> -
> -  *med = tmpval;
> -  return med;
> -}
> -
> -/* Free route map's compiled `match metric' value. */
> -static void
> -route_match_metric_free (void *rule)
> -{
> -  XFREE (MTYPE_ROUTE_MAP_COMPILED, rule);
> -}
> -
>  /* Route map commands for metric matching. */
>  struct route_map_rule_cmd route_match_metric_cmd =
>  {
>    "metric",
>    route_match_metric,
> -  route_match_metric_compile,
> -  route_match_metric_free
> +  route_value_compile,
> +  route_value_free,
>  };
>
>  /* `match as-path ASPATH' */
> @@ -992,64 +1035,34 @@ static route_map_result_t
>  route_set_local_pref (void *rule, struct prefix *prefix,
>                       route_map_object_t type, void *object)
>  {
> -  u_int32_t *local_pref;
> +  struct rmap_value *rv;
>    struct bgp_info *bgp_info;
> +  u_int32_t locpref = 0;
>
>    if (type == RMAP_BGP)
>      {
>        /* Fetch routemap's rule information. */
> -      local_pref = rule;
> +      rv = rule;
>        bgp_info = object;
>
>        /* Set local preference value. */
> +      if (bgp_info->attr->flag & ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF))
> +       locpref = bgp_info->attr->local_pref;
> +
>        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, locpref);
>      }
>
>    return RMAP_OKAY;
>  }
>
> -/* set local preference compilation. */
> -static void *
> -route_set_local_pref_compile (const char *arg)
> -{
> -  unsigned long tmp;
> -  u_int32_t *local_pref;
> -  char *endptr = NULL;
> -
> -  /* Local preference value shoud be integer. */
> -  if (! all_digit (arg))
> -    return NULL;
> -
> -  errno = 0;
> -  tmp = strtoul (arg, &endptr, 10);
> -  if (*endptr != '\0' || errno || tmp > UINT32_MAX)
> -    return NULL;
> -
> -  local_pref = XMALLOC (MTYPE_ROUTE_MAP_COMPILED, sizeof (u_int32_t));
> -
> -  if (!local_pref)
> -    return local_pref;
> -
> -  *local_pref = tmp;
> -
> -  return local_pref;
> -}
> -
> -/* Free route map's local preference value. */
> -static void
> -route_set_local_pref_free (void *rule)
> -{
> -  XFREE (MTYPE_ROUTE_MAP_COMPILED, rule);
> -}
> -
>  /* Set local preference rule structure. */
>  struct route_map_rule_cmd route_set_local_pref_cmd =
>  {
>    "local-preference",
>    route_set_local_pref,
> -  route_set_local_pref_compile,
> -  route_set_local_pref_free,
> +  route_value_compile,
> +  route_value_free,
>  };
>
>  /* `set weight WEIGHT' */
> @@ -1059,18 +1072,20 @@ static route_map_result_t
>  route_set_weight (void *rule, struct prefix *prefix, route_map_object_t
> type,
>                   void *object)
>  {
> -  u_int32_t *weight;
> +  struct rmap_value *rv;
>    struct bgp_info *bgp_info;
> +  u_int32_t weight;
>
>    if (type == RMAP_BGP)
>      {
>        /* Fetch routemap's rule information. */
> -      weight = rule;
> +      rv = rule;
>        bgp_info = object;
>
>        /* Set weight value. */
> -      if (*weight)
> -        (bgp_attr_extra_get (bgp_info->attr))->weight = *weight;
> +      weight = route_value_adjust(rv, 0);
> +      if (weight)
> +        (bgp_attr_extra_get (bgp_info->attr))->weight = weight;
>        else if (bgp_info->attr->extra)
>          bgp_info->attr->extra->weight = 0;
>      }
> @@ -1078,47 +1093,13 @@ route_set_weight (void *rule, struct prefix
> *prefix, route_map_object_t type,
>    return RMAP_OKAY;
>  }
>
> -/* set local preference compilation. */
> -static void *
> -route_set_weight_compile (const char *arg)
> -{
> -  unsigned long tmp;
> -  u_int32_t *weight;
> -  char *endptr = NULL;
> -
> -  /* Local preference value shoud be integer. */
> -  if (! all_digit (arg))
> -    return NULL;
> -
> -  errno = 0;
> -  tmp = strtoul (arg, &endptr, 10);
> -  if (*endptr != '\0' || errno || tmp > UINT32_MAX)
> -    return NULL;
> -
> -  weight = XMALLOC (MTYPE_ROUTE_MAP_COMPILED, sizeof (u_int32_t));
> -
> -  if (weight == NULL)
> -    return weight;
> -
> -  *weight = tmp;
> -
> -  return weight;
> -}
> -
> -/* Free route map's local preference value. */
> -static void
> -route_set_weight_free (void *rule)
> -{
> -  XFREE (MTYPE_ROUTE_MAP_COMPILED, rule);
> -}
> -
>  /* Set local preference rule structure. */
>  struct route_map_rule_cmd route_set_weight_cmd =
>  {
>    "weight",
>    route_set_weight,
> -  route_set_weight_compile,
> -  route_set_weight_free,
> +  route_value_compile,
> +  route_value_free,
>  };
>
>  /* `set metric METRIC' */
> @@ -1128,94 +1109,32 @@ static route_map_result_t
>  route_set_metric (void *rule, struct prefix *prefix,
>                   route_map_object_t type, void *object)
>  {
> -  char *metric;
> -  u_int32_t metric_val;
> +  struct rmap_value *rv;
>    struct bgp_info *bgp_info;
> +  u_int32_t med = 0;
>
>    if (type == RMAP_BGP)
>      {
>        /* Fetch routemap's rule information. */
> -      metric = rule;
> +      rv = rule;
>        bgp_info = object;
>
> -      if (! (bgp_info->attr->flag & ATTR_FLAG_BIT
> (BGP_ATTR_MULTI_EXIT_DISC)))
> -       bgp_info->attr->med = 0;
> -      bgp_info->attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MULTI_EXIT_DISC);
> +      if (bgp_info->attr->flag & ATTR_FLAG_BIT (BGP_ATTR_MULTI_EXIT_DISC))
> +       med = bgp_info->attr->med;
>
> -      if (all_digit (metric))
> -       {
> -         metric_val = strtoul (metric, (char **)NULL, 10);
> -         bgp_info->attr->med = metric_val;
> -       }
> -      else
> -       {
> -         metric_val = strtoul (metric+1, (char **)NULL, 10);
> -
> -         if (strncmp (metric, "+", 1) == 0)
> -           {
> -             if (bgp_info->attr->med/2 + metric_val/2 > BGP_MED_MAX/2)
> -               bgp_info->attr->med = BGP_MED_MAX - 1;
> -             else
> -               bgp_info->attr->med += metric_val;
> -           }
> -         else if (strncmp (metric, "-", 1) == 0)
> -           {
> -             if (bgp_info->attr->med <= metric_val)
> -               bgp_info->attr->med = 0;
> -             else
> -               bgp_info->attr->med -= metric_val;
> -           }
> -       }
> +      bgp_info->attr->med = route_value_adjust(rv, med);
> +      bgp_info->attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MULTI_EXIT_DISC);
>      }
>    return RMAP_OKAY;
>  }
>
> -/* set metric compilation. */
> -static void *
> -route_set_metric_compile (const char *arg)
> -{
> -  unsigned long larg;
> -  char *endptr = NULL;
> -
> -  if (all_digit (arg))
> -    {
> -      /* set metric value check*/
> -      errno = 0;
> -      larg = strtoul (arg, &endptr, 10);
> -      if (*endptr != '\0' || errno || larg > UINT32_MAX)
> -        return NULL;
> -    }
> -  else
> -    {
> -      /* set metric +/-value check */
> -      if ((strncmp (arg, "+", 1) != 0
> -          && strncmp (arg, "-", 1) != 0)
> -          || (! all_digit (arg+1)))
> -       return NULL;
> -
> -      errno = 0;
> -      larg = strtoul (arg+1, &endptr, 10);
> -      if (*endptr != '\0' || errno || larg > UINT32_MAX)
> -       return NULL;
> -    }
> -
> -  return XSTRDUP (MTYPE_ROUTE_MAP_COMPILED, arg);
> -}
> -
> -/* Free route map's compiled `set metric' value. */
> -static void
> -route_set_metric_free (void *rule)
> -{
> -  XFREE (MTYPE_ROUTE_MAP_COMPILED, rule);
> -}
> -
>  /* Set metric rule structure. */
>  struct route_map_rule_cmd route_set_metric_cmd =
>  {
>    "metric",
>    route_set_metric,
> -  route_set_metric_compile,
> -  route_set_metric_free,
> +  route_value_compile,
> +  route_value_free,
>  };
>
>  /* `set as-path prepend ASPATH' */
> --
> 2.3.6
>
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev at lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.quagga.net/pipermail/quagga-dev/attachments/20150429/b7be261d/attachment-0001.html>


More information about the Quagga-dev mailing list