Quagga: Make routemap updates or deletes work for VRFs

Updates to routemaps and delete of the routemap were not working properly
for VRFs. This was because while routemaps are global, the routemap update
processing timer and the processing were at the per-instance level. This
approach was unable to handle processing for multiple instances as the
routemap has no tracking of which instances are still pending processing.
This lead to the processing happening correctly only for the first instance
- which could be the default instance or some other instance. It could also
result in reference to freed memory for an instance.

The fix done is to make the update/delete processing also global and not per
instance. This means that the route-map delay timer will be global and a global
thread will handle the change (or delete) for all instances instead of spawning
a separate thread for each instance. To support this, a global BGP command
"bgp route-map delay-timer <value>" has been implemented. The existing command
per-instance is not deleted but will update the global timer.

Signed-off-by: Vivek Venkatraman <vivek@cumulusnetworks.com>
Reviewed-by:   Donald Sharp <sharpd@cumulusnetworks.com>
Ticket: CM-6970, CM-9918
Reviewed By: CCR-4320
Testing Done: Manual, bgpsmoke

diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c
index 38e5bd7296bd1e1a8fb8e18628dbd3d1aa04d71c..311ebb3635082b952e85503e378fd8d10e76f21f 100644 (file)
--- a/bgpd/bgp_routemap.c
+++ b/bgpd/bgp_routemap.c
@@ -2804,8 +2804,8 @@ bgp_route_map_update_peer_group(const char *rmap_name, struct route_map *map,
  * called for each route-map and within this function we walk the list of peers,
  * network statements, etc looking to see if they use this route-map.
  */
-static int
-bgp_route_map_process_update (void *arg, const char *rmap_name, int route_update)
+static void
+bgp_route_map_process_update (struct bgp *bgp, const char *rmap_name, int route_update)
 {
   int i;
   afi_t afi;
@@ -2813,14 +2813,10 @@ bgp_route_map_process_update (void *arg, const char *rmap_name, int route_update
   struct peer *peer;
   struct bgp_node *bn;
   struct bgp_static *bgp_static;
-  struct bgp *bgp = (struct bgp *)arg;
   struct listnode *node, *nnode;
   struct route_map *map;
   char buf[INET6_ADDRSTRLEN];
 
-  if (!bgp)
-    return (-1);
-
   map = route_map_lookup_by_name (rmap_name);
 
   for (ALL_LIST_ELEMENTS (bgp->peer, node, nnode, peer))
@@ -2918,24 +2914,26 @@ bgp_route_map_process_update (void *arg, const char *rmap_name, int route_update
               }
          }
       }
-
-  return (0);
 }
 
 static int
-bgp_route_map_process_update_cb (void *arg, char *rmap_name)
+bgp_route_map_process_update_cb (char *rmap_name)
 {
-  return bgp_route_map_process_update (arg, rmap_name, 1);
+  struct listnode *node, *nnode;
+  struct bgp *bgp;
+
+  for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp))
+    bgp_route_map_process_update(bgp, rmap_name, 1);
+
+  return 0;
 }
 
 int
 bgp_route_map_update_timer(struct thread *thread)
 {
-  struct bgp *bgp = THREAD_ARG(thread);
+  bm->t_rmap_update = NULL;
 
-  bgp->t_rmap_update = NULL;
-
-  route_map_walk_update_list((void *)bgp, bgp_route_map_process_update_cb);
+  route_map_walk_update_list(bgp_route_map_process_update_cb);
 
   return (0);
 }
@@ -2943,26 +2941,27 @@ bgp_route_map_update_timer(struct thread *thread)
 static void
 bgp_route_map_mark_update (const char *rmap_name)
 {
-  struct listnode *node, *nnode;
-  struct bgp *bgp;
-
-  for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp))
+  if (bm->t_rmap_update == NULL)
     {
+      struct listnode *node, *nnode;
+      struct bgp *bgp;
 
-      if (bgp->t_rmap_update == NULL)
-       {
-         /* rmap_update_timer of 0 means don't do route updates */
-         if (bgp->rmap_update_timer)
-            {
-               bgp->t_rmap_update =
-                 thread_add_timer(bm->master, bgp_route_map_update_timer, bgp,
-                                  bgp->rmap_update_timer);
-              /* Signal the groups that a route-map update event has started */
-              update_group_policy_update(bgp, BGP_POLICY_ROUTE_MAP, rmap_name, 1, 1);
-            }
-         else
-           bgp_route_map_process_update((void *)bgp, rmap_name, 0);
-       }
+      /* rmap_update_timer of 0 means don't do route updates */
+      if (bm->rmap_update_timer)
+        {
+          bm->t_rmap_update =
+            thread_add_timer(bm->master, bgp_route_map_update_timer, NULL,
+                             bm->rmap_update_timer);
+
+          /* Signal the groups that a route-map update event has started */
+          for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp))
+            update_group_policy_update(bgp, BGP_POLICY_ROUTE_MAP, rmap_name, 1, 1);
+        }
+      else
+        {
+          for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp))
+            bgp_route_map_process_update(bgp, rmap_name, 0);
+        }
     }
 }
 

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index a40e8b7bb68f8a1c8e3f5c0e9144e98ef3da240e..bf8f221fa7a55603e87f2e6dc1bb90c493d10a76 100644 (file)
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -4943,21 +4943,19 @@ DEFUN (bgp_set_route_map_delay_timer,
        "0 disables the timer, no route updates happen when route-maps change\n")
 {
   u_int32_t rmap_delay_timer;
-  struct bgp *bgp;
 
-  bgp = vty->index;
   if (argv[0])
     {
       VTY_GET_INTEGER_RANGE ("delay-timer", rmap_delay_timer, argv[0], 0, 600);
-      bgp->rmap_update_timer = rmap_delay_timer;
+      bm->rmap_update_timer = rmap_delay_timer;
 
       /* if the dynamic update handling is being disabled, and a timer is
        * running, stop the timer and act as if the timer has already fired.
        */
-      if (!rmap_delay_timer && bgp->t_rmap_update )
+      if (!rmap_delay_timer && bm->t_rmap_update )
        {
-         BGP_TIMER_OFF(bgp->t_rmap_update);
-         thread_execute (bm->master, bgp_route_map_update_timer, &bgp, 0);
+         BGP_TIMER_OFF(bm->t_rmap_update);
+         thread_execute (bm->master, bgp_route_map_update_timer, NULL, 0);
        }
       return CMD_SUCCESS;
     }
@@ -4972,10 +4970,8 @@ DEFUN (no_bgp_set_route_map_delay_timer,
        "Default BGP route-map delay timer\n"
        "Reset to default time to wait for processing route-map changes\n")
 {
-  struct bgp *bgp;
 
-  bgp = vty->index;
-  bgp->rmap_update_timer = RMAP_DEFAULT_UPDATE_TIMER;
+  bm->rmap_update_timer = RMAP_DEFAULT_UPDATE_TIMER;
 
   return CMD_SUCCESS;
 }
@@ -12340,6 +12336,11 @@ bgp_vty_init (void)
   install_element (CONFIG_NODE, &bgp_config_type_cmd);
   install_element (CONFIG_NODE, &no_bgp_config_type_val_cmd);
 
+  /* bgp route-map delay-timer commands. */
+  install_element (CONFIG_NODE, &bgp_set_route_map_delay_timer_cmd);
+  install_element (CONFIG_NODE, &no_bgp_set_route_map_delay_timer_cmd);
+  install_element (CONFIG_NODE, &no_bgp_set_route_map_delay_timer_val_cmd);
+
   /* Dummy commands (Currently not supported) */
   install_element (BGP_NODE, &no_synchronization_cmd);
   install_element (BGP_NODE, &no_auto_summary_cmd);
@@ -12432,7 +12433,7 @@ bgp_vty_init (void)
   install_element (BGP_NODE, &no_bgp_timers_cmd);
   install_element (BGP_NODE, &no_bgp_timers_arg_cmd);
 
-  /* route-map delay-timer commands */
+  /* route-map delay-timer commands - per instance for backwards compat. */
   install_element (BGP_NODE, &bgp_set_route_map_delay_timer_cmd);
   install_element (BGP_NODE, &no_bgp_set_route_map_delay_timer_cmd);
   install_element (BGP_NODE, &no_bgp_set_route_map_delay_timer_val_cmd);

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 10b9c4dea956d57c939da98418d5fbed97e1a028..aa470b73fd8d5a1e08de6500ab99dc1eb71a88cf 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -2897,8 +2897,6 @@ bgp_get (struct bgp **bgp_val, as_t *as, const char *name,
   *bgp_val = bgp;
 
   bgp->t_rmap_def_originate_eval = NULL;
-  bgp->t_rmap_update = NULL;
-  bgp->rmap_update_timer = RMAP_DEFAULT_UPDATE_TIMER;
 
   /* Create BGP server socket, if first instance.  */
   if (list_isempty(bm->bgp)
@@ -2968,8 +2966,6 @@ bgp_instance_down (struct bgp *bgp)
   struct listnode *next;
 
   /* Stop timers. */
-  if (bgp->t_rmap_update)
-    BGP_TIMER_OFF(bgp->t_rmap_update);
   if (bgp->t_rmap_def_originate_eval)
     {
       BGP_TIMER_OFF(bgp->t_rmap_def_originate_eval);
@@ -3013,8 +3009,6 @@ bgp_delete (struct bgp *bgp)
     }
 
   /* Stop timers. */
-  if (bgp->t_rmap_update)
-    BGP_TIMER_OFF(bgp->t_rmap_update);
   if (bgp->t_rmap_def_originate_eval)
     {
       BGP_TIMER_OFF(bgp->t_rmap_def_originate_eval);
@@ -6927,6 +6921,10 @@ bgp_config_write (struct vty *vty)
       write++;
     }
 
+  if (bm->rmap_update_timer != RMAP_DEFAULT_UPDATE_TIMER)
+    vty_out (vty, "bgp route-map delay-timer %d%s", bm->rmap_update_timer,
+             VTY_NEWLINE);
+
   /* BGP configuration. */
   for (ALL_LIST_ELEMENTS (bm->bgp, mnode, mnnode, bgp))
     {
@@ -7104,10 +7102,6 @@ bgp_config_write (struct vty *vty)
        vty_out (vty, " timers bgp %d %d%s", bgp->default_keepalive, 
                 bgp->default_holdtime, VTY_NEWLINE);
 
-      if (bgp->rmap_update_timer != RMAP_DEFAULT_UPDATE_TIMER)
-       vty_out (vty, " bgp route-map delay-timer %d%s", bgp->rmap_update_timer,
-                VTY_NEWLINE);
-
       /* peer-group */
       for (ALL_LIST_ELEMENTS (bgp->group, node, nnode, group))
        {
@@ -7162,6 +7156,8 @@ bgp_master_init (void)
   bm->port = BGP_PORT_DEFAULT;
   bm->master = thread_master_create ();
   bm->start_time = bgp_clock ();
+  bm->t_rmap_update = NULL;
+  bm->rmap_update_timer = RMAP_DEFAULT_UPDATE_TIMER;
 
   bgp_process_queue_init();
 }
@@ -7284,4 +7280,7 @@ bgp_terminate (void)
       work_queue_free (bm->process_main_queue);
       bm->process_main_queue = NULL;
     }
+
+  if (bm->t_rmap_update)
+    BGP_TIMER_OFF(bm->t_rmap_update);
 }

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 240694bb6d6a90365828c5dc666fe76b1f37a810..e72fab006db85c2052f423dc82ddf288155ec7a2 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -108,6 +108,11 @@ struct bgp_master
 
   u_int64_t updgrp_idspace;
   u_int64_t subgrp_idspace;
+
+  /* timer to dampen route map changes */
+  struct thread *t_rmap_update;   /* Handle route map updates */
+  u_int32_t rmap_update_timer;   /* Route map update timer */
+#define RMAP_DEFAULT_UPDATE_TIMER 5 /* disabled by default */
 };
 
 /* BGP route-map structure.  */
@@ -303,11 +308,6 @@ struct bgp
   /* BGP redistribute configuration. */
   struct list *redist[AFI_MAX][ZEBRA_ROUTE_MAX];
 
-  /* timer to dampen route map changes */
-  struct thread *t_rmap_update;   /* Handle route map updates */
-  u_int32_t rmap_update_timer;   /* Route map update timer */
-#define RMAP_DEFAULT_UPDATE_TIMER 5 /* disabled by default */
-
   /* timer to re-evaluate neighbor default-originate route-maps */
   struct thread *t_rmap_def_originate_eval;
 #define RMAP_DEFAULT_ORIGINATE_EVAL_TIMER 5

diff --git a/lib/routemap.c b/lib/routemap.c
index 53df69dfcd772f6f2d2cc9f181d9f62aff151b41..733e61c99b6bfead57ecddc635ec5ac07d9e757f 100644 (file)
--- a/lib/routemap.c
+++ b/lib/routemap.c
@@ -323,8 +323,7 @@ route_map_get (const char *name)
 }
 
 void
-route_map_walk_update_list (void *arg,
-                           int (*route_map_update_fn) (void *arg, char *name))
+route_map_walk_update_list (int (*route_map_update_fn) (char *name))
 {
   struct route_map *node;
   struct route_map *nnode = NULL;
@@ -334,7 +333,7 @@ route_map_walk_update_list (void *arg,
       if (node->to_be_processed)
        {
          /* DD: Should we add any thread yield code here */
-         route_map_update_fn(arg, node->name);
+         route_map_update_fn(node->name);
          nnode = node->next;
          route_map_clear_updated(node);
        }

diff --git a/lib/routemap.h b/lib/routemap.h
index 5c0626bae4f4c59e5795bae743ac90bb4346ff8f..a90966b383e6a40ad6a18c7680fa1ffe40bdba56 100644 (file)
--- a/lib/routemap.h
+++ b/lib/routemap.h
@@ -217,9 +217,7 @@ extern void route_map_event_hook (void (*func) (route_map_event_t,
                                                const char *));
 extern int route_map_mark_updated (const char *name, int deleted);
 extern int route_map_clear_updated (struct route_map *rmap);
-extern void route_map_walk_update_list (void *arg,
-                                       int (*update_fn) (void *arg,
-                                                         char *name));
+extern void route_map_walk_update_list (int (*update_fn) (char *name));
 extern void route_map_upd8_dependency (route_map_event_t type, const char *arg,
                                       const char *rmap_name);
 extern void route_map_notify_dependencies (const char *affected_name,