Refactor circuit_predict_and_launch_new

changes/ticket18873

@@ -1,3 +1,5 @@
   o Code simplification and refactoring:
     - Extracted dummy_origin_circuit_new so it can be used by other test
       functions.
+    - Refactor circuit_predict_and_launch_new for readability and testability.
+    - Added unit tests for extracted functions.

src/or/circuitbuild.c

@@ -1508,9 +1508,9 @@ circuit_get_unhandled_ports(time_t now)
  * If we're returning 0, set need_uptime and need_capacity to
  * indicate any requirements that the unhandled ports have.
  */
-int
-circuit_all_predicted_ports_handled(time_t now, int *need_uptime,
-                                    int *need_capacity)
+MOCK_IMPL(int,
+circuit_all_predicted_ports_handled, (time_t now, int *need_uptime,
+                                      int *need_capacity))
 {
   int i, enough;
   uint16_t *port;

src/or/circuitbuild.h

@@ -40,8 +40,9 @@ int onionskin_answer(or_circuit_t *circ,
                      const struct created_cell_t *created_cell,
                      const char *keys,
                      const uint8_t *rend_circ_nonce);
-int circuit_all_predicted_ports_handled(time_t now, int *need_uptime,
-                                        int *need_capacity);
+MOCK_DECL(int, circuit_all_predicted_ports_handled, (time_t now,
+                                                     int *need_uptime,
+                                                     int *need_capacity));
 
 int circuit_append_new_exit(origin_circuit_t *circ, extend_info_t *info);
 int circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *info);

src/or/circuituse.c

@@ -1022,8 +1022,95 @@ circuit_stream_is_being_handled(entry_connection_t *conn,
 /** Don't keep more than this many unused open circuits around. */
 #define MAX_UNUSED_OPEN_CIRCUITS 14
 
-/** Figure out how many circuits we have open that are clean. Make
- * sure it's enough for all the upcoming behaviors we predict we'll have.
+/* Return true if a circuit is available for use, meaning that it is open,
+ * clean, usable for new multi-hop connections, and a general purpose origin
+ * circuit.
+ * Accept any kind of circuit, return false if the above conditions are not
+ * met. */
+STATIC int
+circuit_is_available_for_use(const circuit_t *circ)
+{
+  const origin_circuit_t *origin_circ;
+  cpath_build_state_t *build_state;
+
+  if (!CIRCUIT_IS_ORIGIN(circ))
+    return 0;
+  if (circ->marked_for_close)
+    return 0; /* Don't mess with marked circs */
+  if (circ->timestamp_dirty)
+    return 0; /* Only count clean circs */
+  if (circ->purpose != CIRCUIT_PURPOSE_C_GENERAL)
+    return 0;/* Only pay attention to general
+                purpose circs */
+
+  origin_circ = CONST_TO_ORIGIN_CIRCUIT(circ);
+  if (origin_circ->unusable_for_new_conns)
+    return 0;
+
+  build_state = origin_circ->build_state;
+  if (build_state->onehop_tunnel)
+    return 0;
+
+  return 1;
+}
+
+/* Return true if we need any more exit circuits.
+ * needs_uptime and needs_capacity are set only if we need more exit circuits.
+ * Check if we know of a port that's been requested recently and no circuit
+ * is currently available that can handle it. */
+STATIC int
+needs_exit_circuits(time_t now, int *needs_uptime, int *needs_capacity)
+{
+  return (!circuit_all_predicted_ports_handled(now, needs_uptime,
+                                               needs_capacity) &&
+          router_have_consensus_path() == CONSENSUS_PATH_EXIT);
+}
+
+/* Return true if we need any more hidden service server circuits.
+ * HS servers only need an internal circuit. */
+STATIC int
+needs_hs_server_circuits(int num_uptime_internal)
+{
+  return (num_rend_services() && num_uptime_internal < 3 &&
+          router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN);
+}
+
+/* Return true if we need any more hidden service client circuits.
+ * HS clients only need an internal circuit. */
+STATIC int
+needs_hs_client_circuits(time_t now, int *needs_uptime, int *needs_capacity,
+    int num_internal, int num_uptime_internal)
+{
+  int used_internal_recently = rep_hist_get_predicted_internal(now,
+                                                               needs_uptime,
+                                                               needs_capacity);
+  return (used_internal_recently &&
+         ((num_uptime_internal<2 && needs_uptime) || num_internal<3) &&
+          router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN);
+}
+
+/* Check to see if we still need more circuits to learn
+ * a good build timeout. But if we're close to our max number we
+ * want, don't do another -- we want to leave a few slots open so
+ * we can still build circuits preemptively as needed.
+ * XXXX make the assumption that build timeout streams should be
+ * created whenever we can build internal circuits. */
+STATIC int
+needs_circuits_for_build(int num)
+{
+  if (router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) {
+    if (num < MAX_UNUSED_OPEN_CIRCUITS-2 &&
+        ! circuit_build_times_disabled() &&
+        circuit_build_times_needs_circuits_now(get_circuit_build_times()))
+    {
+      return 1;
+    }
+  }
+  return 0;
+}
+
+/** Determine how many circuits we have open that are clean,
+ * Make sure it's enough for all the upcoming behaviors we predict we'll have.
  * But put an upper bound on the total number of circuits.
  */
 static void
@@ -1035,25 +1122,14 @@ circuit_predict_and_launch_new(void)
   time_t now = time(NULL);
   int flags = 0;
 
-  /* First, count how many of each type of circuit we have already. */
+  /* Count how many of each type of circuit we currently have. */
   SMARTLIST_FOREACH_BEGIN(circuit_get_global_list(), circuit_t *, circ) {
-    cpath_build_state_t *build_state;
-    origin_circuit_t *origin_circ;
-    if (!CIRCUIT_IS_ORIGIN(circ))
-      continue;
-    if (circ->marked_for_close)
-      continue; /* don't mess with marked circs */
-    if (circ->timestamp_dirty)
-      continue; /* only count clean circs */
-    if (circ->purpose != CIRCUIT_PURPOSE_C_GENERAL)
-      continue; /* only pay attention to general-purpose circs */
-    origin_circ = TO_ORIGIN_CIRCUIT(circ);
-    if (origin_circ->unusable_for_new_conns)
-      continue;
-    build_state = origin_circ->build_state;
-    if (build_state->onehop_tunnel)
+    if (!circuit_is_available_for_use(circ))
       continue;
+
     num++;
+
+    cpath_build_state_t *build_state = TO_ORIGIN_CIRCUIT(circ)->build_state;
     if (build_state->is_internal)
       num_internal++;
     if (build_state->need_uptime && build_state->is_internal)
@@ -1063,19 +1139,14 @@ circuit_predict_and_launch_new(void)
 
   /* If that's enough, then stop now. */
   if (num >= MAX_UNUSED_OPEN_CIRCUITS)
-    return; /* we already have many, making more probably will hurt */
+    return;
 
-  /* Second, see if we need any more exit circuits. */
-  /* check if we know of a port that's been requested recently
-   * and no circuit is currently available that can handle it.
-   * Exits (obviously) require an exit circuit. */
-  if (!circuit_all_predicted_ports_handled(now, &port_needs_uptime,
-                                           &port_needs_capacity)
-      && router_have_consensus_path() == CONSENSUS_PATH_EXIT) {
+  if (needs_exit_circuits(now, &port_needs_uptime, &port_needs_capacity)) {
     if (port_needs_uptime)
       flags |= CIRCLAUNCH_NEED_UPTIME;
     if (port_needs_capacity)
       flags |= CIRCLAUNCH_NEED_CAPACITY;
+
     log_info(LD_CIRC,
              "Have %d clean circs (%d internal), need another exit circ.",
              num, num_internal);
@@ -1083,12 +1154,10 @@ circuit_predict_and_launch_new(void)
     return;
   }
 
-  /* Third, see if we need any more hidden service (server) circuits.
-   * HS servers only need an internal circuit. */
-  if (num_rend_services() && num_uptime_internal < 3
-      && router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) {
+  if (needs_hs_server_circuits(num_uptime_internal)) {
     flags = (CIRCLAUNCH_NEED_CAPACITY | CIRCLAUNCH_NEED_UPTIME |
              CIRCLAUNCH_IS_INTERNAL);
+
     log_info(LD_CIRC,
              "Have %d clean circs (%d internal), need another internal "
              "circ for my hidden service.",
@@ -1097,18 +1166,16 @@ circuit_predict_and_launch_new(void)
     return;
   }
 
-  /* Fourth, see if we need any more hidden service (client) circuits.
-   * HS clients only need an internal circuit. */
-  if (rep_hist_get_predicted_internal(now, &hidserv_needs_uptime,
-                                      &hidserv_needs_capacity) &&
-      ((num_uptime_internal<2 && hidserv_needs_uptime) ||
-        num_internal<3)
-        && router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) {
+  if (needs_hs_client_circuits(now, &hidserv_needs_uptime,
+                               &hidserv_needs_capacity,
+                               num_internal, num_uptime_internal))
+  {
     if (hidserv_needs_uptime)
       flags |= CIRCLAUNCH_NEED_UPTIME;
     if (hidserv_needs_capacity)
       flags |= CIRCLAUNCH_NEED_CAPACITY;
     flags |= CIRCLAUNCH_IS_INTERNAL;
+
     log_info(LD_CIRC,
              "Have %d clean circs (%d uptime-internal, %d internal), need"
              " another hidden service circ.",
@@ -1117,26 +1184,17 @@ circuit_predict_and_launch_new(void)
     return;
   }
 
-  /* Finally, check to see if we still need more circuits to learn
-   * a good build timeout. But if we're close to our max number we
-   * want, don't do another -- we want to leave a few slots open so
-   * we can still build circuits preemptively as needed.
-   * XXXX make the assumption that build timeout streams should be
-   * created whenever we can build internal circuits. */
-  if (router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) {
-    if (num < MAX_UNUSED_OPEN_CIRCUITS-2 &&
-        ! circuit_build_times_disabled() &&
-        circuit_build_times_needs_circuits_now(get_circuit_build_times())) {
-      flags = CIRCLAUNCH_NEED_CAPACITY;
-      /* if there are no exits in the consensus, make timeout
-       * circuits internal */
-      if (router_have_consensus_path() == CONSENSUS_PATH_INTERNAL)
-        flags |= CIRCLAUNCH_IS_INTERNAL;
+  if (needs_circuits_for_build(num)) {
+    flags = CIRCLAUNCH_NEED_CAPACITY;
+    /* if there are no exits in the consensus, make timeout
+     * circuits internal */
+    if (router_have_consensus_path() == CONSENSUS_PATH_INTERNAL)
+      flags |= CIRCLAUNCH_IS_INTERNAL;
+
       log_info(LD_CIRC,
                "Have %d clean circs need another buildtime test circ.", num);
       circuit_launch(CIRCUIT_PURPOSE_C_GENERAL, flags);
       return;
-    }
   }
 }
 

src/or/circuituse.h

@@ -59,5 +59,25 @@ int hostname_in_track_host_exits(const or_options_t *options,
                                  const char *address);
 void mark_circuit_unusable_for_new_conns(origin_circuit_t *circ);
 
+#ifdef TOR_UNIT_TESTS
+/* Used only by circuituse.c and test_circuituse.c */
+
+STATIC int circuit_is_available_for_use(const circuit_t *circ);
+
+STATIC int needs_exit_circuits(time_t now,
+                               int *port_needs_uptime,
+                               int *port_needs_capacity);
+STATIC int needs_hs_server_circuits(int num_uptime_internal);
+
+STATIC int needs_hs_client_circuits(time_t now,
+                                    int *needs_uptime,
+                                    int *needs_capacity,
+                                    int num_internal,
+                                    int num_uptime_internal);
+
+STATIC int needs_circuits_for_build(int num);
+
+#endif
+
 #endif
 

src/or/nodelist.c

@@ -1569,8 +1569,8 @@ router_have_minimum_dir_info(void)
  * this can cause router_have_consensus_path() to be set to
  * CONSENSUS_PATH_EXIT, even if there are no nodes with accept exit policies.
  */
-consensus_path_type_t
-router_have_consensus_path(void)
+MOCK_IMPL(consensus_path_type_t,
+router_have_consensus_path, (void))
 {
   return have_consensus_path;
 }

src/or/nodelist.h

@@ -118,7 +118,8 @@ typedef enum {
    * create exit and internal paths, circuits, streams, ... */
   CONSENSUS_PATH_EXIT = 1
 } consensus_path_type_t;
-consensus_path_type_t router_have_consensus_path(void);
+
+MOCK_DECL(consensus_path_type_t, router_have_consensus_path, (void));
 
 void router_dir_info_changed(void);
 const char *get_dir_info_status_string(void);

src/test/include.am

@@ -80,6 +80,7 @@ src_test_test_SOURCES = \
 	src/test/test_checkdir.c \
 	src/test/test_circuitlist.c \
 	src/test/test_circuitmux.c \
+	src/test/test_circuituse.c \
 	src/test/test_compat_libevent.c \
 	src/test/test_config.c \
 	src/test/test_connection.c \

src/test/test.c

@@ -1190,6 +1190,7 @@ struct testgroup_t testgroups[] = {
   { "checkdir/", checkdir_tests },
   { "circuitlist/", circuitlist_tests },
   { "circuitmux/", circuitmux_tests },
+  { "circuituse/", circuituse_tests },
   { "compat/libevent/", compat_libevent_tests },
   { "config/", config_tests },
   { "connection/", connection_tests },

src/test/test.h

@@ -185,6 +185,7 @@ extern struct testcase_t channeltls_tests[];
 extern struct testcase_t checkdir_tests[];
 extern struct testcase_t circuitlist_tests[];
 extern struct testcase_t circuitmux_tests[];
+extern struct testcase_t circuituse_tests[];
 extern struct testcase_t compat_libevent_tests[];
 extern struct testcase_t config_tests[];
 extern struct testcase_t connection_tests[];

src/test/test_circuituse.c

@@ -0,0 +1,284 @@
+/* Copyright (c) 2001-2004, Roger Dingledine.
+ * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2016, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+#include "or.h"
+#include "test.h"
+#include "test_helpers.h"
+#include "config.h"
+#include "circuitlist.h"
+#include "circuituse.h"
+#include "circuitbuild.h"
+#include "nodelist.h"
+
+static void
+test_circuit_is_available_for_use_ret_false_when_marked_for_close(void *arg)
+{
+  (void)arg;
+
+  circuit_t *circ = tor_malloc(sizeof(circuit_t));
+  circ->marked_for_close = 1;
+
+  tt_int_op(0, ==, circuit_is_available_for_use(circ));
+
+  done:
+    tor_free(circ);
+}
+
+static void
+test_circuit_is_available_for_use_ret_false_when_timestamp_dirty(void *arg)
+{
+  (void)arg;
+
+  circuit_t *circ = tor_malloc(sizeof(circuit_t));
+  circ->timestamp_dirty = 1;
+
+  tt_int_op(0, ==, circuit_is_available_for_use(circ));
+
+  done:
+    tor_free(circ);
+}
+
+static void
+test_circuit_is_available_for_use_ret_false_for_non_general_purpose(void *arg)
+{
+  (void)arg;
+
+  circuit_t *circ = tor_malloc(sizeof(circuit_t));
+  circ->purpose = CIRCUIT_PURPOSE_OR;
+
+  tt_int_op(0, ==, circuit_is_available_for_use(circ));
+
+  done:
+    tor_free(circ);
+}
+
+static void
+test_circuit_is_available_for_use_ret_false_for_non_origin_purpose(void *arg)
+{
+  (void)arg;
+
+  circuit_t *circ = tor_malloc(sizeof(circuit_t));
+  circ->purpose = CIRCUIT_PURPOSE_OR;
+
+  tt_int_op(0, ==, circuit_is_available_for_use(circ));
+
+  done:
+    tor_free(circ);
+}
+
+static void
+test_circuit_is_available_for_use_ret_false_unusable_for_new_conns(void *arg)
+{
+  (void)arg;
+
+  circuit_t *circ = dummy_origin_circuit_new(30);
+  mark_circuit_unusable_for_new_conns(TO_ORIGIN_CIRCUIT(circ));
+
+  tt_int_op(0, ==, circuit_is_available_for_use(circ));
+
+  done:
+    tor_free(circ);
+}
+
+static void
+test_circuit_is_available_for_use_returns_false_for_onehop_tunnel(void *arg)
+{
+  (void)arg;
+
+  circuit_t *circ = dummy_origin_circuit_new(30);
+  origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ);
+  oc->build_state = tor_malloc(sizeof(cpath_build_state_t));
+  oc->build_state->onehop_tunnel = 1;
+
+  tt_int_op(0, ==, circuit_is_available_for_use(circ));
+
+  done:
+    tor_free(circ);
+}
+
+static void
+test_circuit_is_available_for_use_returns_true_for_clean_circuit(void *arg)
+{
+  (void)arg;
+
+  circuit_t *circ = dummy_origin_circuit_new(30);
+  origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ);
+  oc->build_state = tor_malloc(sizeof(cpath_build_state_t));
+  oc->build_state->onehop_tunnel = 0;
+
+  tt_int_op(1, ==, circuit_is_available_for_use(circ));
+
+  done:
+    tor_free(circ);
+}
+
+static int
+mock_circuit_all_predicted_ports_handled(time_t now,
+                                         int *need_uptime,
+                                         int *need_capacity)
+{
+  (void)now;
+
+  if (need_uptime && need_capacity)
+    return 0;
+  return 1;
+}
+
+static consensus_path_type_t
+mock_router_have_unknown_consensus_path(void)
+{
+  return CONSENSUS_PATH_UNKNOWN;
+}
+
+static consensus_path_type_t
+mock_router_have_exit_consensus_path(void)
+{
+  return CONSENSUS_PATH_EXIT;
+}
+
+static void
+test_needs_exit_circuits_ret_false_for_predicted_ports_and_path(void *arg)
+{
+  (void)arg;
+
+  MOCK(circuit_all_predicted_ports_handled,
+       mock_circuit_all_predicted_ports_handled);
+  int needs_uptime = 1;
+  int needs_capacity = 0;
+
+  time_t now = time(NULL);
+  tt_int_op(0, ==, needs_exit_circuits(now, &needs_uptime, &needs_capacity));
+
+  done:
+    UNMOCK(circuit_all_predicted_ports_handled);
+}
+
+static void
+test_needs_exit_circuits_ret_false_for_non_exit_consensus_path(void *arg)
+{
+  (void)arg;
+
+  MOCK(circuit_all_predicted_ports_handled,
+       mock_circuit_all_predicted_ports_handled);
+  int needs_uptime = 1;
+  int needs_capacity = 1;
+  MOCK(router_have_consensus_path, mock_router_have_unknown_consensus_path);
+
+  time_t now = time(NULL);
+  tt_int_op(0, ==, needs_exit_circuits(now, &needs_uptime, &needs_capacity));
+
+  done:
+    UNMOCK(circuit_all_predicted_ports_handled);
+    UNMOCK(router_have_consensus_path);
+}
+
+static void
+test_needs_exit_circuits_ret_true_for_predicted_ports_and_path(void *arg)
+{
+  (void)arg;
+
+  MOCK(circuit_all_predicted_ports_handled,
+       mock_circuit_all_predicted_ports_handled);
+  int needs_uptime = 1;
+  int needs_capacity = 1;
+  MOCK(router_have_consensus_path, mock_router_have_exit_consensus_path);
+
+  time_t now = time(NULL);
+  tt_int_op(1, ==, needs_exit_circuits(now, &needs_uptime, &needs_capacity));
+
+  done:
+    UNMOCK(circuit_all_predicted_ports_handled);
+    UNMOCK(router_have_consensus_path);
+}
+
+static void
+test_needs_circuits_for_build_ret_false_consensus_path_unknown(void *arg)
+{
+  (void)arg;
+  MOCK(router_have_consensus_path, mock_router_have_unknown_consensus_path);
+  tt_int_op(0, ==, needs_circuits_for_build(0));
+  done: ;
+}
+
+static void
+test_needs_circuits_for_build_ret_false_if_num_less_than_max(void *arg)
+{
+  (void)arg;
+  MOCK(router_have_consensus_path, mock_router_have_exit_consensus_path);
+  tt_int_op(0, ==, needs_circuits_for_build(13));
+  done:
+    UNMOCK(router_have_consensus_path);
+}
+
+static void
+test_needs_circuits_for_build_returns_true_when_more_are_needed(void *arg)
+{
+  (void)arg;
+  MOCK(router_have_consensus_path, mock_router_have_exit_consensus_path);
+  tt_int_op(1, ==, needs_circuits_for_build(0));
+  done:
+    UNMOCK(router_have_consensus_path);
+}
+
+struct testcase_t circuituse_tests[] = {
+ { "marked",
+   test_circuit_is_available_for_use_ret_false_when_marked_for_close,
+   TT_FORK, NULL, NULL
+ },
+ { "timestamp",
+   test_circuit_is_available_for_use_ret_false_when_timestamp_dirty,
+   TT_FORK, NULL, NULL
+ },
+ { "non_general",
+   test_circuit_is_available_for_use_ret_false_for_non_general_purpose,
+   TT_FORK, NULL, NULL
+ },
+ { "origin",
+   test_circuit_is_available_for_use_ret_false_for_non_origin_purpose,
+   TT_FORK, NULL, NULL
+ },
+ { "clean",
+   test_circuit_is_available_for_use_ret_false_unusable_for_new_conns,
+   TT_FORK, NULL, NULL
+ },
+ { "onehop",
+   test_circuit_is_available_for_use_returns_false_for_onehop_tunnel,
+   TT_FORK, NULL, NULL
+ },
+ { "clean_circ",
+   test_circuit_is_available_for_use_returns_true_for_clean_circuit,
+   TT_FORK, NULL, NULL
+ },
+ { "exit_f",
+   test_needs_exit_circuits_ret_false_for_predicted_ports_and_path,
+   TT_FORK, NULL, NULL
+ },
+ { "exit_t",
+   test_needs_exit_circuits_ret_true_for_predicted_ports_and_path,
+   TT_FORK, NULL, NULL
+ },
+ { "non_exit",
+   test_needs_exit_circuits_ret_false_for_non_exit_consensus_path,
+   TT_FORK, NULL, NULL
+ },
+ { "true",
+   test_needs_exit_circuits_ret_true_for_predicted_ports_and_path,
+   TT_FORK, NULL, NULL
+ },
+ { "consensus_path_unknown",
+   test_needs_circuits_for_build_ret_false_consensus_path_unknown,
+   TT_FORK, NULL, NULL
+ },
+ { "less_than_max",
+   test_needs_circuits_for_build_ret_false_if_num_less_than_max,
+   TT_FORK, NULL, NULL
+ },
+ { "more_needed",
+   test_needs_circuits_for_build_returns_true_when_more_are_needed,
+   TT_FORK, NULL, NULL
+ },
+  END_OF_TESTCASES
+};
+