Merge remote-tracking branch 'dgoulet/bug24975_032_01' into maint-0.3.2

changes/bug24975

@@ -2,5 +2,5 @@
     - A logic in the code was preventing the scheduler subystem to properly
       make a decision based on the latest consensus when it arrives. This lead
       to the scheduler failing to notice any consensus parameters that might
-      change from one consensus to another. Fixes bug 24975; bugfix on
+      have changed between consensuses. Fixes bug 24975; bugfix on
       0.3.2.1-alpha.

src/or/networkstatus.c

@@ -1564,17 +1564,20 @@ notify_control_networkstatus_changed(const networkstatus_t *old_c,
   smartlist_free(changed);
 }
 
-/* Called when the consensus has changed from old_c to new_c.
- *
- * IMPORTANT: This is called _after_ the new consensus has been set in the
- * global state so this is safe for anything getting the latest consensus from
- * that state. */
+/* Called before the consensus changes from old_c to new_c. */
 static void
-notify_networkstatus_changed(const networkstatus_t *old_c,
-                             const networkstatus_t *new_c)
+notify_before_networkstatus_changes(const networkstatus_t *old_c,
+                                    const networkstatus_t *new_c)
 {
   notify_control_networkstatus_changed(old_c, new_c);
-  scheduler_notify_networkstatus_changed(old_c, new_c);
+}
+
+/* Called after a new consensus has been put in the global state. It is safe
+ * to use the consensus getters in this function. */
+static void
+notify_after_networkstatus_changes(void)
+{
+  scheduler_notify_networkstatus_changed();
 }
 
 /** Copy all the ancillary information (like router download status and so on)
@@ -1901,6 +1904,12 @@ networkstatus_set_current_consensus(const char *consensus,
 
   const int is_usable_flavor = flav == usable_consensus_flavor();
 
+  /* Before we switch to the new consensus, notify that we are about to change
+   * it using the old consensus and the new one. */
+  if (is_usable_flavor) {
+    notify_before_networkstatus_changes(networkstatus_get_latest_consensus(),
+                                        c);
+  }
   if (flav == FLAV_NS) {
     if (current_ns_consensus) {
       networkstatus_copy_old_consensus_info(c, current_ns_consensus);
@@ -1923,13 +1932,6 @@ networkstatus_set_current_consensus(const char *consensus,
     free_consensus = 0; /* avoid free */
   }
 
-  /* Called _after_ the consensus is set in its global variable so any
-   * functions called from this notification can safely get the latest
-   * consensus being the new one. */
-  if (is_usable_flavor) {
-    notify_networkstatus_changed(networkstatus_get_latest_consensus(), c);
-  }
-
   waiting = &consensus_waiting_for_certs[flav];
   if (waiting->consensus &&
       waiting->consensus->valid_after <= c->valid_after) {
@@ -1949,6 +1951,10 @@ networkstatus_set_current_consensus(const char *consensus,
   }
 
   if (is_usable_flavor) {
+    /* Notify that we just changed the consensus so the current global value
+     * can be looked at. */
+    notify_after_networkstatus_changes();
+
     /* The "current" consensus has just been set and it is a usable flavor so
      * the first thing we need to do is recalculate the voting schedule static
      * object so we can use the timings in there needed by some subsystems

src/or/scheduler.c

@@ -435,15 +435,14 @@ scheduler_conf_changed(void)
  * Whenever we get a new consensus, this function is called.
  */
 void
-scheduler_notify_networkstatus_changed(const networkstatus_t *old_c,
-                                       const networkstatus_t *new_c)
+scheduler_notify_networkstatus_changed(void)
 {
   /* Maybe the consensus param made us change the scheduler. */
   set_scheduler();
 
   /* Then tell the (possibly new) scheduler that we have a new consensus */
   if (the_scheduler->on_new_consensus) {
-    the_scheduler->on_new_consensus(old_c, new_c);
+    the_scheduler->on_new_consensus();
   }
 }
 

src/or/scheduler.h

@@ -80,8 +80,7 @@ typedef struct scheduler_s {
    * (which might be new) will call this so it has the chance to react to the
    * new consensus too. If there's a consensus parameter that your scheduler
    * wants to keep an eye on, this is where you should check for it.  */
-  void (*on_new_consensus)(const networkstatus_t *old_c,
-                           const networkstatus_t *new_c);
+  void (*on_new_consensus)(void);
 
   /* (Optional) To be called when a channel is being freed. Sometimes channels
    * go away (for example: the relay on the other end is shutting down). If
@@ -119,8 +118,7 @@ typedef struct scheduler_s {
 void scheduler_init(void);
 void scheduler_free_all(void);
 void scheduler_conf_changed(void);
-void scheduler_notify_networkstatus_changed(const networkstatus_t *old_c,
-                                            const networkstatus_t *new_c);
+void scheduler_notify_networkstatus_changed(void);
 MOCK_DECL(void, scheduler_release_channel, (channel_t *chan));
 
 /*
@@ -197,7 +195,7 @@ int scheduler_can_use_kist(void);
 void scheduler_kist_set_full_mode(void);
 void scheduler_kist_set_lite_mode(void);
 scheduler_t *get_kist_scheduler(void);
-int kist_scheduler_run_interval(const networkstatus_t *ns);
+int kist_scheduler_run_interval(void);
 
 #ifdef TOR_UNIT_TESTS
 extern int32_t sched_run_interval;

src/or/scheduler_kist.c

@@ -362,10 +362,10 @@ outbuf_table_remove(outbuf_table_t *table, channel_t *chan)
 
 /* Set the scheduler running interval. */
 static void
-set_scheduler_run_interval(const networkstatus_t *ns)
+set_scheduler_run_interval(void)
 {
   int old_sched_run_interval = sched_run_interval;
-  sched_run_interval = kist_scheduler_run_interval(ns);
+  sched_run_interval = kist_scheduler_run_interval();
   if (old_sched_run_interval != sched_run_interval) {
     log_info(LD_SCHED, "Scheduler KIST changing its running interval "
                        "from %" PRId32 " to %" PRId32,
@@ -481,13 +481,9 @@ kist_on_channel_free(const channel_t *chan)
 
 /* Function of the scheduler interface: on_new_consensus() */
 static void
-kist_scheduler_on_new_consensus(const networkstatus_t *old_c,
-                                const networkstatus_t *new_c)
+kist_scheduler_on_new_consensus(void)
 {
-  (void) old_c;
-  (void) new_c;
-
-  set_scheduler_run_interval(new_c);
+  set_scheduler_run_interval();
 }
 
 /* Function of the scheduler interface: on_new_options() */
@@ -497,7 +493,7 @@ kist_scheduler_on_new_options(void)
   sock_buf_size_factor = get_options()->KISTSockBufSizeFactor;
 
   /* Calls kist_scheduler_run_interval which calls get_options(). */
-  set_scheduler_run_interval(NULL);
+  set_scheduler_run_interval();
 }
 
 /* Function of the scheduler interface: init() */
@@ -770,7 +766,7 @@ get_kist_scheduler(void)
  *   - If consensus doesn't say anything, return 10 milliseconds, default.
  */
 int
-kist_scheduler_run_interval(const networkstatus_t *ns)
+kist_scheduler_run_interval(void)
 {
   int run_interval = get_options()->KISTSchedRunInterval;
 
@@ -784,7 +780,7 @@ kist_scheduler_run_interval(const networkstatus_t *ns)
 
   /* Will either be the consensus value or the default. Note that 0 can be
    * returned which means the consensus wants us to NOT use KIST. */
-  return networkstatus_get_param(ns, "KISTSchedRunInterval",
+  return networkstatus_get_param(NULL, "KISTSchedRunInterval",
                                  KIST_SCHED_RUN_INTERVAL_DEFAULT,
                                  KIST_SCHED_RUN_INTERVAL_MIN,
                                  KIST_SCHED_RUN_INTERVAL_MAX);
@@ -823,7 +819,7 @@ scheduler_can_use_kist(void)
 
   /* We do have the support, time to check if we can get the interval that the
    * consensus can be disabling. */
-  int run_interval = kist_scheduler_run_interval(NULL);
+  int run_interval = kist_scheduler_run_interval();
   log_debug(LD_SCHED, "Determined KIST sched_run_interval should be "
                       "%" PRId32 ". Can%s use KIST.",
            run_interval, (run_interval > 0 ? "" : " not"));

src/test/test_scheduler.c

@@ -959,7 +959,7 @@ test_scheduler_can_use_kist(void *arg)
   clear_options();
   mocked_options.KISTSchedRunInterval = 1234;
   res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
+  res_freq = kist_scheduler_run_interval();
 #ifdef HAVE_KIST_SUPPORT
   tt_int_op(res_should, ==, 1);
 #else /* HAVE_KIST_SUPPORT */
@@ -971,7 +971,7 @@ test_scheduler_can_use_kist(void *arg)
   clear_options();
   mocked_options.KISTSchedRunInterval = 0;
   res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
+  res_freq = kist_scheduler_run_interval();
 #ifdef HAVE_KIST_SUPPORT
   tt_int_op(res_should, ==, 1);
 #else /* HAVE_KIST_SUPPORT */
@@ -984,7 +984,7 @@ test_scheduler_can_use_kist(void *arg)
   clear_options();
   mocked_options.KISTSchedRunInterval = 0;
   res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
+  res_freq = kist_scheduler_run_interval();
 #ifdef HAVE_KIST_SUPPORT
   tt_int_op(res_should, ==, 1);
 #else /* HAVE_KIST_SUPPORT */
@@ -998,7 +998,7 @@ test_scheduler_can_use_kist(void *arg)
   clear_options();
   mocked_options.KISTSchedRunInterval = 0;
   res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
+  res_freq = kist_scheduler_run_interval();
   tt_int_op(res_should, ==, 0);
   tt_int_op(res_freq, ==, 0);
   UNMOCK(networkstatus_get_param);
@@ -1032,7 +1032,7 @@ test_scheduler_ns_changed(void *arg)
   /* Change from vanilla to kist via consensus */
   the_scheduler = get_vanilla_scheduler();
   MOCK(networkstatus_get_param, mock_kist_networkstatus_get_param);
-  scheduler_notify_networkstatus_changed(NULL, NULL);
+  scheduler_notify_networkstatus_changed();
   UNMOCK(networkstatus_get_param);
 #ifdef HAVE_KIST_SUPPORT
   tt_ptr_op(the_scheduler, ==, get_kist_scheduler());
@@ -1043,14 +1043,14 @@ test_scheduler_ns_changed(void *arg)
   /* Change from kist to vanilla via consensus */
   the_scheduler = get_kist_scheduler();
   MOCK(networkstatus_get_param, mock_vanilla_networkstatus_get_param);
-  scheduler_notify_networkstatus_changed(NULL, NULL);
+  scheduler_notify_networkstatus_changed();
   UNMOCK(networkstatus_get_param);
   tt_ptr_op(the_scheduler, ==, get_vanilla_scheduler());
 
   /* Doesn't change when using KIST */
   the_scheduler = get_kist_scheduler();
   MOCK(networkstatus_get_param, mock_kist_networkstatus_get_param);
-  scheduler_notify_networkstatus_changed(NULL, NULL);
+  scheduler_notify_networkstatus_changed();
   UNMOCK(networkstatus_get_param);
 #ifdef HAVE_KIST_SUPPORT
   tt_ptr_op(the_scheduler, ==, get_kist_scheduler());
@@ -1061,7 +1061,7 @@ test_scheduler_ns_changed(void *arg)
   /* Doesn't change when using vanilla */
   the_scheduler = get_vanilla_scheduler();
   MOCK(networkstatus_get_param, mock_vanilla_networkstatus_get_param);
-  scheduler_notify_networkstatus_changed(NULL, NULL);
+  scheduler_notify_networkstatus_changed();
   UNMOCK(networkstatus_get_param);
   tt_ptr_op(the_scheduler, ==, get_vanilla_scheduler());