Browse Source

Prop210: Refactor connection_get_* to produce lists and counts

teor (Tim Wilson-Brown) 8 years ago
parent
commit
df0c135d62
6 changed files with 940 additions and 79 deletions
  1. 153 77
      src/or/connection.c
  2. 58 1
      src/or/connection.h
  3. 2 1
      src/test/Makefile.nmake
  4. 1 0
      src/test/include.am
  5. 2 0
      src/test/test.c
  6. 724 0
      src/test/test_connection.c

+ 153 - 77
src/or/connection.c

@@ -1618,13 +1618,18 @@ connection_init_accepted_conn(connection_t *conn,
   return 0;
 }
 
-static int
-connection_connect_sockaddr(connection_t *conn,
+/** Take conn, make a nonblocking socket; try to connect to
+ * sa, binding to bindaddr if sa is not localhost. If fail, return -1 and if
+ * applicable put your best guess about errno into *<b>socket_error</b>.
+ * If connected return 1, if EAGAIN return 0.
+ */
+MOCK_IMPL(STATIC int,
+connection_connect_sockaddr,(connection_t *conn,
                             const struct sockaddr *sa,
                             socklen_t sa_len,
                             const struct sockaddr *bindaddr,
                             socklen_t bindaddr_len,
-                            int *socket_error)
+                            int *socket_error))
 {
   tor_socket_t s;
   int inprogress = 0;
@@ -4222,6 +4227,19 @@ connection_write_to_buf_impl_,(const char *string, size_t len,
   }
 }
 
+/** Return a connection_t * from get_connection_array() that satisfies test on
+ * var, and that is not marked for close. */
+#define CONN_GET_TEMPLATE(var, test)               \
+  STMT_BEGIN                                       \
+    smartlist_t *conns = get_connection_array();   \
+    SMARTLIST_FOREACH(conns, connection_t *, var,  \
+    {                                              \
+      if (var && (test) && !var->marked_for_close) \
+        return var;                                \
+    });                                            \
+    return NULL;                                   \
+  STMT_END
+
 /** Return a connection with given type, address, port, and purpose;
  * or NULL if no such connection exists (or if all such connections are marked
  * for close). */
@@ -4230,17 +4248,11 @@ connection_get_by_type_addr_port_purpose(int type,
                                          const tor_addr_t *addr, uint16_t port,
                                          int purpose)
 {
-  smartlist_t *conns = get_connection_array();
-  SMARTLIST_FOREACH(conns, connection_t *, conn,
-  {
-    if (conn->type == type &&
+  CONN_GET_TEMPLATE(conn,
+       (conn->type == type &&
         tor_addr_eq(&conn->addr, addr) &&
         conn->port == port &&
-        conn->purpose == purpose &&
-        !conn->marked_for_close)
-      return conn;
-  });
-  return NULL;
+        conn->purpose == purpose));
 }
 
 /** Return the stream with id <b>id</b> if it is not already marked for
@@ -4249,13 +4261,7 @@ connection_get_by_type_addr_port_purpose(int type,
 connection_t *
 connection_get_by_global_id(uint64_t id)
 {
-  smartlist_t *conns = get_connection_array();
-  SMARTLIST_FOREACH(conns, connection_t *, conn,
-  {
-    if (conn->global_identifier == id)
-      return conn;
-  });
-  return NULL;
+  CONN_GET_TEMPLATE(conn, conn->global_identifier == id);
 }
 
 /** Return a connection of type <b>type</b> that is not marked for close.
@@ -4263,13 +4269,7 @@ connection_get_by_global_id(uint64_t id)
 connection_t *
 connection_get_by_type(int type)
 {
-  smartlist_t *conns = get_connection_array();
-  SMARTLIST_FOREACH(conns, connection_t *, conn,
-  {
-    if (conn->type == type && !conn->marked_for_close)
-      return conn;
-  });
-  return NULL;
+  CONN_GET_TEMPLATE(conn, conn->type == type);
 }
 
 /** Return a connection of type <b>type</b> that is in state <b>state</b>,
@@ -4278,13 +4278,7 @@ connection_get_by_type(int type)
 connection_t *
 connection_get_by_type_state(int type, int state)
 {
-  smartlist_t *conns = get_connection_array();
-  SMARTLIST_FOREACH(conns, connection_t *, conn,
-  {
-    if (conn->type == type && conn->state == state && !conn->marked_for_close)
-      return conn;
-  });
-  return NULL;
+  CONN_GET_TEMPLATE(conn, conn->type == type && conn->state == state);
 }
 
 /** Return a connection of type <b>type</b> that has rendquery equal
@@ -4295,55 +4289,142 @@ connection_t *
 connection_get_by_type_state_rendquery(int type, int state,
                                        const char *rendquery)
 {
-  smartlist_t *conns = get_connection_array();
-
   tor_assert(type == CONN_TYPE_DIR ||
              type == CONN_TYPE_AP || type == CONN_TYPE_EXIT);
   tor_assert(rendquery);
 
-  SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn) {
-    if (conn->type == type &&
-        !conn->marked_for_close &&
-        (!state || state == conn->state)) {
-      if (type == CONN_TYPE_DIR &&
+  CONN_GET_TEMPLATE(conn,
+       (conn->type == type &&
+        (!state || state == conn->state)) &&
+        (
+         (type == CONN_TYPE_DIR &&
           TO_DIR_CONN(conn)->rend_data &&
           !rend_cmp_service_ids(rendquery,
                                 TO_DIR_CONN(conn)->rend_data->onion_address))
-        return conn;
-      else if (CONN_IS_EDGE(conn) &&
+         ||
+              (CONN_IS_EDGE(conn) &&
                TO_EDGE_CONN(conn)->rend_data &&
                !rend_cmp_service_ids(rendquery,
                             TO_EDGE_CONN(conn)->rend_data->onion_address))
-        return conn;
-    }
-  } SMARTLIST_FOREACH_END(conn);
-  return NULL;
+         ));
 }
 
+#define CONN_FIRST_AND_FREE_TEMPLATE(sl)       \
+  STMT_BEGIN                                   \
+    if (smartlist_len(sl) > 0) {               \
+      void *first_item = smartlist_get(sl, 0); \
+      smartlist_free(sl);                      \
+      return first_item;                       \
+    } else {                                   \
+      smartlist_free(sl);                      \
+      return NULL;                             \
+    }                                          \
+  STMT_END
+
+
 /** Return a directory connection (if any one exists) that is fetching
- * the item described by <b>state</b>/<b>resource</b> */
+ * the item described by <b>purpose</b>/<b>resource</b>, otherwise return NULL.
+ */
 dir_connection_t *
-connection_dir_get_by_purpose_and_resource(int purpose,
+connection_dir_get_by_purpose_and_resource(
+                                           int purpose,
                                            const char *resource)
 {
-  smartlist_t *conns = get_connection_array();
+  smartlist_t *conns = connection_dir_list_by_purpose_and_resource(
+                                                          purpose,
+                                                          resource);
+  CONN_FIRST_AND_FREE_TEMPLATE(conns);
+}
 
-  SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn) {
-    dir_connection_t *dirconn;
-    if (conn->type != CONN_TYPE_DIR || conn->marked_for_close ||
-        conn->purpose != purpose)
-      continue;
-    dirconn = TO_DIR_CONN(conn);
-    if (dirconn->requested_resource == NULL) {
-      if (resource == NULL)
-        return dirconn;
-    } else if (resource) {
-      if (0 == strcmp(resource, dirconn->requested_resource))
-        return dirconn;
-    }
-  } SMARTLIST_FOREACH_END(conn);
+/** Return a new smartlist of dir_connection_t * from get_connection_array()
+ * that satisfy conn_test on connection_t *conn_var, and dirconn_test on
+ * dir_connection_t *dirconn_var. conn_var must be of CONN_TYPE_DIR and not
+ * marked for close to be included in the list. */
+#define DIR_CONN_LIST_TEMPLATE(conn_var, conn_test,             \
+                               dirconn_var, dirconn_test)       \
+  STMT_BEGIN                                                    \
+    smartlist_t *conns = get_connection_array();                \
+    smartlist_t *dir_conns = smartlist_new();                   \
+    SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn_var) {  \
+      if (conn_var && (conn_test)                               \
+          && conn_var->type == CONN_TYPE_DIR                    \
+          && !conn_var->marked_for_close) {                     \
+        dir_connection_t *dirconn_var = TO_DIR_CONN(conn_var);  \
+        if (dirconn_var && (dirconn_test)) {                    \
+          smartlist_add(dir_conns, dirconn_var);                \
+        }                                                       \
+      }                                                         \
+    } SMARTLIST_FOREACH_END(conn_var);                          \
+    return dir_conns;                                           \
+  STMT_END
+
+/** Return a list of directory connections that are fetching the item
+ * described by <b>purpose</b>/<b>resource</b>. If there are none,
+ * return an empty list. This list must be freed using smartlist_free,
+ * but the pointers in it must not be freed.
+ * Note that this list should not be cached, as the pointers in it can be
+ * freed if their connections close. */
+smartlist_t *
+connection_dir_list_by_purpose_and_resource(
+                                            int purpose,
+                                            const char *resource)
+{
+  DIR_CONN_LIST_TEMPLATE(conn,
+                         conn->purpose == purpose,
+                         dirconn,
+                         0 == strcmp_opt(resource,
+                                         dirconn->requested_resource));
+}
 
-  return NULL;
+/** Return a directory connection (if any one exists) that is fetching
+ * the item described by <b>purpose</b>/<b>resource</b>/<b>state</b>,
+ * otherwise return NULL. */
+dir_connection_t *
+connection_dir_get_by_purpose_resource_and_state(
+                                                 int purpose,
+                                                 const char *resource,
+                                                 int state)
+{
+  smartlist_t *conns =
+    connection_dir_list_by_purpose_resource_and_state(
+                                                      purpose,
+                                                      resource,
+                                                      state);
+  CONN_FIRST_AND_FREE_TEMPLATE(conns);
+}
+
+#undef CONN_FIRST_AND_FREE_TEMPLATE
+
+/** Return a list of directory connections that are fetching the item
+ * described by <b>purpose</b>/<b>resource</b>/<b>state</b>. If there are
+ * none, return an empty list. This list must be freed using smartlist_free,
+ * but the pointers in it must not be freed.
+ * Note that this list should not be cached, as the pointers in it can be
+ * freed if their connections close. */
+smartlist_t *
+connection_dir_list_by_purpose_resource_and_state(
+                                                  int purpose,
+                                                  const char *resource,
+                                                  int state)
+{
+  DIR_CONN_LIST_TEMPLATE(conn,
+                         conn->purpose == purpose && conn->state == state,
+                         dirconn,
+                         0 == strcmp_opt(resource,
+                                         dirconn->requested_resource));
+}
+
+#undef DIR_CONN_LIST_TEMPLATE
+
+/** Return an arbitrary active OR connection that isn't <b>this_conn</b>.
+ *
+ * We use this to guess if we should tell the controller that we
+ * didn't manage to connect to any of our bridges. */
+static connection_t *
+connection_get_another_active_or_conn(const or_connection_t *this_conn)
+{
+  CONN_GET_TEMPLATE(conn,
+                    conn != TO_CONN(this_conn) && conn->type == CONN_TYPE_OR);
 }
 
 /** Return 1 if there are any active OR connections apart from
@@ -4354,23 +4435,18 @@ connection_dir_get_by_purpose_and_resource(int purpose,
 int
 any_other_active_or_conns(const or_connection_t *this_conn)
 {
-  smartlist_t *conns = get_connection_array();
-  SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn) {
-    if (conn == TO_CONN(this_conn)) { /* don't consider this conn */
-      continue;
-    }
-
-    if (conn->type == CONN_TYPE_OR &&
-        !conn->marked_for_close) {
-      log_debug(LD_DIR, "%s: Found an OR connection: %s",
-                __func__, conn->address);
-      return 1;
-    }
-  } SMARTLIST_FOREACH_END(conn);
+  connection_t *conn = connection_get_another_active_or_conn(this_conn);
+  if (conn != NULL) {
+    log_debug(LD_DIR, "%s: Found an OR connection: %s",
+              __func__, conn->address);
+    return 1;
+  }
 
   return 0;
 }
 
+#undef CONN_GET_TEMPLATE
+
 /** Return 1 if <b>conn</b> is a listener conn, else return 0. */
 int
 connection_is_listener(connection_t *conn)

+ 58 - 1
src/or/connection.h

@@ -193,7 +193,57 @@ connection_t *connection_get_by_type_state(int type, int state);
 connection_t *connection_get_by_type_state_rendquery(int type, int state,
                                                      const char *rendquery);
 dir_connection_t *connection_dir_get_by_purpose_and_resource(
-                                           int state, const char *resource);
+                                                  int purpose,
+                                                  const char *resource);
+dir_connection_t *connection_dir_get_by_purpose_resource_and_state(
+                                                  int purpose,
+                                                  const char *resource,
+                                                  int state);
+smartlist_t *connection_dir_list_by_purpose_and_resource(
+                                                  int purpose,
+                                                  const char *resource);
+smartlist_t *connection_dir_list_by_purpose_resource_and_state(
+                                                  int purpose,
+                                                  const char *resource,
+                                                  int state);
+
+#define CONN_LEN_AND_FREE_TEMPLATE(sl) \
+  STMT_BEGIN                           \
+    int len = smartlist_len(sl);       \
+    smartlist_free(sl);                \
+    return len;                        \
+  STMT_END
+
+/** Return a count of directory connections that are fetching the item
+ * described by <b>purpose</b>/<b>resource</b>. */
+static INLINE int
+connection_dir_count_by_purpose_and_resource(
+                                             int purpose,
+                                             const char *resource)
+{
+  smartlist_t *conns = connection_dir_list_by_purpose_and_resource(
+                                                                   purpose,
+                                                                   resource);
+  CONN_LEN_AND_FREE_TEMPLATE(conns);
+}
+
+/** Return a count of directory connections that are fetching the item
+ * described by <b>purpose</b>/<b>resource</b>/<b>state</b>. */
+static INLINE int
+connection_dir_count_by_purpose_resource_and_state(
+                                                   int purpose,
+                                                   const char *resource,
+                                                   int state)
+{
+  smartlist_t *conns =
+    connection_dir_list_by_purpose_resource_and_state(
+                                                      purpose,
+                                                      resource,
+                                                      state);
+  CONN_LEN_AND_FREE_TEMPLATE(conns);
+}
+
+#undef CONN_LEN_AND_FREE_TEMPLATE
 
 int any_other_active_or_conns(const or_connection_t *this_conn);
 
@@ -239,6 +289,13 @@ void connection_buckets_note_empty_ts(uint32_t *timestamp_var,
                                       int tokens_before,
                                       size_t tokens_removed,
                                       const struct timeval *tvnow);
+MOCK_DECL(STATIC int,connection_connect_sockaddr,
+                                            (connection_t *conn,
+                                             const struct sockaddr *sa,
+                                             socklen_t sa_len,
+                                             const struct sockaddr *bindaddr,
+                                             socklen_t bindaddr_len,
+                                             int *socket_error));
 #endif
 
 #endif

+ 2 - 1
src/test/Makefile.nmake

@@ -14,7 +14,8 @@ LIBS = ..\..\..\build-alpha\lib\libevent.lib \
 TEST_OBJECTS = test.obj test_addr.obj test_channel.obj test_channeltls.obj \
         test_containers.obj \
 	test_controller_events.obj test_crypto.obj test_data.obj test_dir.obj \
-	test_checkdir.obj test_microdesc.obj test_pt.obj test_util.obj test_config.obj \
+	test_checkdir.obj test_microdesc.obj test_pt.obj test_util.obj \
+        test_config.obj test_connection.obj \
 	test_cell_formats.obj test_relay.obj test_replay.obj \
 	test_scheduler.obj test_introduce.obj test_hs.obj tinytest.obj
 

+ 1 - 0
src/test/include.am

@@ -65,6 +65,7 @@ src_test_test_SOURCES = \
 	src/test/test_circuitmux.c \
 	src/test/test_compat_libevent.c \
 	src/test/test_config.c \
+	src/test/test_connection.c \
 	src/test/test_containers.c \
 	src/test/test_controller.c \
 	src/test/test_controller_events.c \

+ 2 - 0
src/test/test.c

@@ -1141,6 +1141,7 @@ extern struct testcase_t circuitlist_tests[];
 extern struct testcase_t circuitmux_tests[];
 extern struct testcase_t compat_libevent_tests[];
 extern struct testcase_t config_tests[];
+extern struct testcase_t connection_tests[];
 extern struct testcase_t container_tests[];
 extern struct testcase_t controller_tests[];
 extern struct testcase_t controller_event_tests[];
@@ -1196,6 +1197,7 @@ struct testgroup_t testgroups[] = {
   { "circuitmux/", circuitmux_tests },
   { "compat/libevent/", compat_libevent_tests },
   { "config/", config_tests },
+  { "connection/", connection_tests },
   { "container/", container_tests },
   { "control/", controller_tests },
   { "control/event/", controller_event_tests },

+ 724 - 0
src/test/test_connection.c

@@ -0,0 +1,724 @@
+/* Copyright (c) 2015, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+#include "orconfig.h"
+
+#define CONNECTION_PRIVATE
+#define MAIN_PRIVATE
+
+#include "or.h"
+#include "test.h"
+
+#include "connection.h"
+#include "main.h"
+#include "networkstatus.h"
+#include "rendcache.h"
+#include "directory.h"
+
+static void test_conn_lookup_addr_helper(const char *address,
+                                         int family,
+                                         tor_addr_t *addr);
+
+static void * test_conn_get_basic_setup(const struct testcase_t *tc);
+static int test_conn_get_basic_teardown(const struct testcase_t *tc,
+                                        void *arg);
+
+static void * test_conn_get_rend_setup(const struct testcase_t *tc);
+static int test_conn_get_rend_teardown(const struct testcase_t *tc,
+                                        void *arg);
+
+static void * test_conn_get_rsrc_setup(const struct testcase_t *tc);
+static int test_conn_get_rsrc_teardown(const struct testcase_t *tc,
+                                        void *arg);
+
+/* Arbitrary choice - IPv4 Directory Connection to localhost */
+#define TEST_CONN_TYPE          (CONN_TYPE_DIR)
+/* We assume every machine has IPv4 localhost, is that ok? */
+#define TEST_CONN_ADDRESS       "127.0.0.1"
+#define TEST_CONN_PORT          (12345)
+#define TEST_CONN_ADDRESS_PORT  "127.0.0.1:12345"
+#define TEST_CONN_FAMILY        (AF_INET)
+#define TEST_CONN_STATE         (DIR_CONN_STATE_MIN_)
+#define TEST_CONN_ADDRESS_2     "127.0.0.2"
+
+#define TEST_CONN_BASIC_PURPOSE (DIR_PURPOSE_MIN_)
+
+#define TEST_CONN_REND_ADDR     "cfs3rltphxxvabci"
+#define TEST_CONN_REND_PURPOSE  (DIR_PURPOSE_FETCH_RENDDESC_V2)
+#define TEST_CONN_REND_PURPOSE_SUCCESSFUL (DIR_PURPOSE_HAS_FETCHED_RENDDESC_V2)
+#define TEST_CONN_REND_TYPE_2   (CONN_TYPE_AP)
+#define TEST_CONN_REND_ADDR_2   "icbavxxhptlr3sfc"
+
+#define TEST_CONN_RSRC          (networkstatus_get_flavor_name(FLAV_MICRODESC))
+#define TEST_CONN_RSRC_PURPOSE  (DIR_PURPOSE_FETCH_CONSENSUS)
+#define TEST_CONN_RSRC_STATE_SUCCESSFUL (DIR_CONN_STATE_CLIENT_FINISHED)
+#define TEST_CONN_RSRC_2        (networkstatus_get_flavor_name(FLAV_NS))
+
+#define TEST_CONN_DL_STATE      (DIR_CONN_STATE_CLIENT_SENDING)
+
+#define TEST_CONN_FD_INIT 50
+static int mock_connection_connect_sockaddr_called = 0;
+static int fake_socket_number = TEST_CONN_FD_INIT;
+
+static int
+mock_connection_connect_sockaddr(connection_t *conn,
+                                 const struct sockaddr *sa,
+                                 socklen_t sa_len,
+                                 const struct sockaddr *bindaddr,
+                                 socklen_t bindaddr_len,
+                                 int *socket_error)
+{
+  (void)sa_len;
+  (void)bindaddr;
+  (void)bindaddr_len;
+
+  tor_assert(conn);
+  tor_assert(sa);
+  tor_assert(socket_error);
+
+  mock_connection_connect_sockaddr_called++;
+
+  conn->s = fake_socket_number++;
+  tt_assert(SOCKET_OK(conn->s));
+  /* We really should call tor_libevent_initialize() here. Because we don't,
+   * we are relying on other parts of the code not checking if the_event_base
+   * (and therefore event->ev_base) is NULL.  */
+  tt_assert(connection_add_connecting(conn) == 0);
+
+ done:
+  /* Fake "connected" status */
+  return 1;
+}
+
+static void
+test_conn_lookup_addr_helper(const char *address, int family, tor_addr_t *addr)
+{
+  int rv = 0;
+
+  tt_assert(addr);
+
+  rv = tor_addr_lookup(address, family, addr);
+  /* XXXX - should we retry on transient failure? */
+  tt_assert(rv == 0);
+  tt_assert(tor_addr_is_loopback(addr));
+  tt_assert(tor_addr_is_v4(addr));
+
+  return;
+
+ done:
+  tor_addr_make_null(addr, TEST_CONN_FAMILY);
+}
+
+static void *
+test_conn_get_basic_setup(const struct testcase_t *tc)
+{
+  connection_t *conn = NULL;
+  tor_addr_t addr;
+  int socket_err = 0;
+  int in_progress = 0;
+  (void)tc;
+
+  MOCK(connection_connect_sockaddr,
+       mock_connection_connect_sockaddr);
+
+  init_connection_lists();
+
+  conn = connection_new(TEST_CONN_TYPE, TEST_CONN_FAMILY);
+  tt_assert(conn);
+
+  test_conn_lookup_addr_helper(TEST_CONN_ADDRESS, TEST_CONN_FAMILY, &addr);
+  tt_assert(!tor_addr_is_null(&addr));
+
+  /* XXXX - connection_connect doesn't set these, should it? */
+  tor_addr_copy_tight(&conn->addr, &addr);
+  conn->port = TEST_CONN_PORT;
+  mock_connection_connect_sockaddr_called = 0;
+  in_progress = connection_connect(conn, TEST_CONN_ADDRESS_PORT, &addr,
+                                   TEST_CONN_PORT, &socket_err);
+  tt_assert(mock_connection_connect_sockaddr_called == 1);
+  tt_assert(!socket_err);
+  tt_assert(in_progress == 0 || in_progress == 1);
+
+  /* fake some of the attributes so the connection looks OK */
+  conn->state = TEST_CONN_STATE;
+  conn->purpose = TEST_CONN_BASIC_PURPOSE;
+  assert_connection_ok(conn, time(NULL));
+
+  UNMOCK(connection_connect_sockaddr);
+
+  return conn;
+
+  /* On failure */
+ done:
+  UNMOCK(connection_connect_sockaddr);
+  test_conn_get_basic_teardown(tc, conn);
+
+  /* Returning NULL causes the unit test to fail */
+  return NULL;
+}
+
+static int
+test_conn_get_basic_teardown(const struct testcase_t *tc, void *arg)
+{
+  (void)tc;
+  connection_t *conn = arg;
+
+  tt_assert(conn);
+  assert_connection_ok(conn, time(NULL));
+
+  /* teardown the connection as fast as possible */
+  if (conn->linked_conn) {
+    assert_connection_ok(conn->linked_conn, time(NULL));
+
+    /* We didn't call tor_libevent_initialize(), so event_base was NULL,
+     * so we can't rely on connection_unregister_events() use of event_del().
+     */
+    if (conn->linked_conn->read_event) {
+      tor_free(conn->linked_conn->read_event);
+      conn->linked_conn->read_event = NULL;
+    }
+    if (conn->linked_conn->write_event) {
+      tor_free(conn->linked_conn->write_event);
+      conn->linked_conn->write_event = NULL;
+    }
+
+    connection_free(conn->linked_conn);
+    conn->linked_conn = NULL;
+
+    conn->linked_conn->linked_conn = NULL;
+    if (!conn->linked_conn->marked_for_close) {
+      connection_close_immediate(conn->linked_conn);
+      connection_mark_for_close(conn->linked_conn);
+    }
+  }
+
+  /* We didn't set the events up properly, so we can't use event_del() in
+   * close_closeable_connections() > connection_free()
+   * > connection_unregister_events() */
+  if (conn->read_event) {
+    tor_free(conn->read_event);
+    conn->read_event = NULL;
+  }
+  if (conn->write_event) {
+    tor_free(conn->write_event);
+    conn->write_event = NULL;
+  }
+
+  if (!conn->marked_for_close) {
+    connection_close_immediate(conn);
+    connection_mark_for_close(conn);
+  }
+
+  close_closeable_connections();
+
+  /* The unit test will fail if we return 0 */
+  return 1;
+
+  /* When conn == NULL, we can't cleanup anything */
+ done:
+  return 0;
+}
+
+static void *
+test_conn_get_rend_setup(const struct testcase_t *tc)
+{
+  dir_connection_t *conn = DOWNCAST(dir_connection_t,
+                                    test_conn_get_basic_setup(tc));
+  tt_assert(conn);
+  assert_connection_ok(&conn->base_, time(NULL));
+
+  rend_cache_init();
+
+  /* TODO: use directory_initiate_command_rend() to do this - maybe? */
+  conn->rend_data = tor_malloc_zero(sizeof(rend_data_t));
+  memcpy(conn->rend_data->onion_address,
+         TEST_CONN_REND_ADDR,
+         REND_SERVICE_ADDRESS_LEN+1);
+  conn->rend_data->hsdirs_fp = smartlist_new();
+  conn->base_.purpose = TEST_CONN_REND_PURPOSE;
+
+  assert_connection_ok(&conn->base_, time(NULL));
+  return conn;
+
+  /* On failure */
+ done:
+  test_conn_get_rend_teardown(tc, conn);
+  /* Returning NULL causes the unit test to fail */
+  return NULL;
+}
+
+static int
+test_conn_get_rend_teardown(const struct testcase_t *tc, void *arg)
+{
+  dir_connection_t *conn = DOWNCAST(dir_connection_t, arg);
+  int rv = 0;
+
+  tt_assert(conn);
+  assert_connection_ok(&conn->base_, time(NULL));
+
+  /* avoid a last-ditch attempt to refetch the descriptor */
+  conn->base_.purpose = TEST_CONN_REND_PURPOSE_SUCCESSFUL;
+
+  /* connection_free_() cleans up rend_data */
+  rv = test_conn_get_basic_teardown(tc, arg);
+ done:
+  rend_cache_free_all();
+  return rv;
+}
+
+static void *
+test_conn_get_rsrc_setup(const struct testcase_t *tc)
+{
+  dir_connection_t *conn = DOWNCAST(dir_connection_t,
+                                    test_conn_get_basic_setup(tc));
+  tt_assert(conn);
+  assert_connection_ok(&conn->base_, time(NULL));
+
+  /* TODO: use the canonical function to do this - maybe? */
+  conn->requested_resource = tor_strdup(TEST_CONN_RSRC);
+  conn->base_.purpose = TEST_CONN_RSRC_PURPOSE;
+
+  assert_connection_ok(&conn->base_, time(NULL));
+  return conn;
+
+  /* On failure */
+ done:
+  test_conn_get_rend_teardown(tc, conn);
+  /* Returning NULL causes the unit test to fail */
+  return NULL;
+}
+
+static int
+test_conn_get_rsrc_teardown(const struct testcase_t *tc, void *arg)
+{
+  dir_connection_t *conn = DOWNCAST(dir_connection_t, arg);
+  int rv = 0;
+
+  tt_assert(conn);
+  assert_connection_ok(&conn->base_, time(NULL));
+
+  /* avoid a last-ditch attempt to refetch the consensus */
+  conn->base_.state = TEST_CONN_RSRC_STATE_SUCCESSFUL;
+
+  /* connection_free_() cleans up requested_resource */
+  rv = test_conn_get_basic_teardown(tc, arg);
+ done:
+  return rv;
+}
+
+static void *
+test_conn_download_status_setup(const struct testcase_t *tc)
+{
+  (void)tc;
+
+  /* Don't return NULL, that causes the test to fail */
+  return "ok";
+}
+
+static int
+test_conn_download_status_teardown(const struct testcase_t *tc, void *arg)
+{
+  (void)arg;
+  int rv = 0;
+
+  /* Ignore arg, and just loop through the connection array */
+  SMARTLIST_FOREACH_BEGIN(get_connection_array(), connection_t *, conn) {
+    if (conn) {
+      assert_connection_ok(conn, time(NULL));
+
+      /* connection_free_() cleans up requested_resource */
+      rv = test_conn_get_rsrc_teardown(tc, conn);
+      tt_assert(rv == 1);
+    }
+  } SMARTLIST_FOREACH_END(conn);
+
+ done:
+  return rv;
+}
+
+static dir_connection_t *
+test_conn_download_status_add_a_connection(void)
+{
+  dir_connection_t *conn = DOWNCAST(dir_connection_t,
+                                    test_conn_get_rsrc_setup(NULL));
+
+  tt_assert(conn);
+  assert_connection_ok(&conn->base_, time(NULL));
+
+  return conn;
+
+ done:
+  test_conn_download_status_teardown(NULL, NULL);
+  return NULL;
+}
+
+static struct testcase_setup_t test_conn_get_basic_st = {
+  test_conn_get_basic_setup, test_conn_get_basic_teardown
+};
+
+static struct testcase_setup_t test_conn_get_rend_st = {
+  test_conn_get_rend_setup, test_conn_get_rend_teardown
+};
+
+static struct testcase_setup_t test_conn_get_rsrc_st = {
+  test_conn_get_rsrc_setup, test_conn_get_rsrc_teardown
+};
+
+static struct testcase_setup_t test_conn_download_status_st = {
+  test_conn_download_status_setup, test_conn_download_status_teardown
+};
+
+static void
+test_conn_get_basic(void *arg)
+{
+  connection_t *conn = (connection_t*)arg;
+  tor_addr_t addr, addr2;
+
+  tt_assert(conn);
+  assert_connection_ok(conn, time(NULL));
+
+  test_conn_lookup_addr_helper(TEST_CONN_ADDRESS, TEST_CONN_FAMILY, &addr);
+  tt_assert(!tor_addr_is_null(&addr));
+  test_conn_lookup_addr_helper(TEST_CONN_ADDRESS_2, TEST_CONN_FAMILY, &addr2);
+  tt_assert(!tor_addr_is_null(&addr2));
+
+  /* Check that we get this connection back when we search for it by
+   * its attributes, but get NULL when we supply a different value. */
+
+  tt_assert(connection_get_by_global_id(conn->global_identifier) == conn);
+  tt_assert(connection_get_by_global_id(!conn->global_identifier) == NULL);
+
+  tt_assert(connection_get_by_type(conn->type) == conn);
+  tt_assert(connection_get_by_type(TEST_CONN_TYPE) == conn);
+  tt_assert(connection_get_by_type(!conn->type) == NULL);
+  tt_assert(connection_get_by_type(!TEST_CONN_TYPE) == NULL);
+
+  tt_assert(connection_get_by_type_state(conn->type, conn->state)
+            == conn);
+  tt_assert(connection_get_by_type_state(TEST_CONN_TYPE, TEST_CONN_STATE)
+            == conn);
+  tt_assert(connection_get_by_type_state(!conn->type, !conn->state)
+            == NULL);
+  tt_assert(connection_get_by_type_state(!TEST_CONN_TYPE, !TEST_CONN_STATE)
+            == NULL);
+
+  /* Match on the connection fields themselves */
+  tt_assert(connection_get_by_type_addr_port_purpose(conn->type,
+                                                     &conn->addr,
+                                                     conn->port,
+                                                     conn->purpose)
+            == conn);
+  /* Match on the original inputs to the connection */
+  tt_assert(connection_get_by_type_addr_port_purpose(TEST_CONN_TYPE,
+                                                     &conn->addr,
+                                                     conn->port,
+                                                     conn->purpose)
+            == conn);
+  tt_assert(connection_get_by_type_addr_port_purpose(conn->type,
+                                                     &addr,
+                                                     conn->port,
+                                                     conn->purpose)
+            == conn);
+  tt_assert(connection_get_by_type_addr_port_purpose(conn->type,
+                                                     &conn->addr,
+                                                     TEST_CONN_PORT,
+                                                     conn->purpose)
+            == conn);
+  tt_assert(connection_get_by_type_addr_port_purpose(conn->type,
+                                                     &conn->addr,
+                                                     conn->port,
+                                                     TEST_CONN_BASIC_PURPOSE)
+            == conn);
+  tt_assert(connection_get_by_type_addr_port_purpose(TEST_CONN_TYPE,
+                                                     &addr,
+                                                     TEST_CONN_PORT,
+                                                     TEST_CONN_BASIC_PURPOSE)
+            == conn);
+  /* Then try each of the not-matching combinations */
+  tt_assert(connection_get_by_type_addr_port_purpose(!conn->type,
+                                                     &conn->addr,
+                                                     conn->port,
+                                                     conn->purpose)
+            == NULL);
+  tt_assert(connection_get_by_type_addr_port_purpose(conn->type,
+                                                     &addr2,
+                                                     conn->port,
+                                                     conn->purpose)
+            == NULL);
+  tt_assert(connection_get_by_type_addr_port_purpose(conn->type,
+                                                     &conn->addr,
+                                                     !conn->port,
+                                                     conn->purpose)
+            == NULL);
+  tt_assert(connection_get_by_type_addr_port_purpose(conn->type,
+                                                     &conn->addr,
+                                                     conn->port,
+                                                     !conn->purpose)
+            == NULL);
+  /* Then try everything not-matching */
+  tt_assert(connection_get_by_type_addr_port_purpose(!conn->type,
+                                                     &addr2,
+                                                     !conn->port,
+                                                     !conn->purpose)
+            == NULL);
+  tt_assert(connection_get_by_type_addr_port_purpose(!TEST_CONN_TYPE,
+                                                     &addr2,
+                                                     !TEST_CONN_PORT,
+                                                     !TEST_CONN_BASIC_PURPOSE)
+            == NULL);
+
+ done:
+  ;
+}
+
+static void
+test_conn_get_rend(void *arg)
+{
+  dir_connection_t *conn = DOWNCAST(dir_connection_t, arg);
+  tt_assert(conn);
+  assert_connection_ok(&conn->base_, time(NULL));
+
+  tt_assert(connection_get_by_type_state_rendquery(
+                                            conn->base_.type,
+                                            conn->base_.state,
+                                            conn->rend_data->onion_address)
+            == TO_CONN(conn));
+  tt_assert(connection_get_by_type_state_rendquery(
+                                            TEST_CONN_TYPE,
+                                            TEST_CONN_STATE,
+                                            TEST_CONN_REND_ADDR)
+            == TO_CONN(conn));
+  tt_assert(connection_get_by_type_state_rendquery(TEST_CONN_REND_TYPE_2,
+                                                   !conn->base_.state,
+                                                   "")
+            == NULL);
+  tt_assert(connection_get_by_type_state_rendquery(TEST_CONN_REND_TYPE_2,
+                                                   !TEST_CONN_STATE,
+                                                   TEST_CONN_REND_ADDR_2)
+            == NULL);
+
+ done:
+  ;
+}
+
+#define sl_is_conn_assert(sl, conn) \
+  do { \
+    tt_assert(smartlist_len((sl)) == 1); \
+    tt_assert(smartlist_get((sl), 0) == (conn)); \
+  } while (0)
+
+#define sl_no_conn_assert(sl) \
+  do { \
+    tt_assert(smartlist_len((sl)) == 0); \
+  } while (0)
+
+static void
+test_conn_get_rsrc(void *arg)
+{
+  dir_connection_t *conn = DOWNCAST(dir_connection_t, arg);
+  tt_assert(conn);
+  assert_connection_ok(&conn->base_, time(NULL));
+
+  tt_assert(connection_dir_get_by_purpose_and_resource(
+                                                    conn->base_.purpose,
+                                                    conn->requested_resource)
+            == conn);
+  tt_assert(connection_dir_get_by_purpose_and_resource(
+                                                    TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC)
+            == conn);
+  tt_assert(connection_dir_get_by_purpose_and_resource(
+                                                    !conn->base_.purpose,
+                                                    "")
+            == NULL);
+  tt_assert(connection_dir_get_by_purpose_and_resource(
+                                                    !TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC_2)
+            == NULL);
+
+  tt_assert(connection_dir_get_by_purpose_resource_and_state(
+                                                    conn->base_.purpose,
+                                                    conn->requested_resource,
+                                                    conn->base_.state)
+            == conn);
+  tt_assert(connection_dir_get_by_purpose_resource_and_state(
+                                                    TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC,
+                                                    TEST_CONN_STATE)
+            == conn);
+  tt_assert(connection_dir_get_by_purpose_resource_and_state(
+                                                    !conn->base_.purpose,
+                                                    "",
+                                                    !conn->base_.state)
+            == NULL);
+  tt_assert(connection_dir_get_by_purpose_resource_and_state(
+                                                    !TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC_2,
+                                                    !TEST_CONN_STATE)
+            == NULL);
+
+  sl_is_conn_assert(connection_dir_list_by_purpose_and_resource(
+                                                    conn->base_.purpose,
+                                                    conn->requested_resource),
+                    conn);
+  sl_is_conn_assert(connection_dir_list_by_purpose_and_resource(
+                                                    TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC),
+                    conn);
+  sl_no_conn_assert(connection_dir_list_by_purpose_and_resource(
+                                                    !conn->base_.purpose,
+                                                    ""));
+  sl_no_conn_assert(connection_dir_list_by_purpose_and_resource(
+                                                    !TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC_2));
+
+  sl_is_conn_assert(connection_dir_list_by_purpose_resource_and_state(
+                                                    conn->base_.purpose,
+                                                    conn->requested_resource,
+                                                    conn->base_.state),
+                    conn);
+  sl_is_conn_assert(connection_dir_list_by_purpose_resource_and_state(
+                                                    TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC,
+                                                    TEST_CONN_STATE),
+                    conn);
+  sl_no_conn_assert(connection_dir_list_by_purpose_resource_and_state(
+                                                    !conn->base_.purpose,
+                                                    "",
+                                                    !conn->base_.state));
+  sl_no_conn_assert(connection_dir_list_by_purpose_resource_and_state(
+                                                    !TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC_2,
+                                                    !TEST_CONN_STATE));
+
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                    conn->base_.purpose,
+                                                    conn->requested_resource)
+            == 1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                    TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC)
+            == 1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                    !conn->base_.purpose,
+                                                    "")
+            == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                    !TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC_2)
+            == 0);
+
+  tt_assert(connection_dir_count_by_purpose_resource_and_state(
+                                                    conn->base_.purpose,
+                                                    conn->requested_resource,
+                                                    conn->base_.state)
+            == 1);
+  tt_assert(connection_dir_count_by_purpose_resource_and_state(
+                                                    TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC,
+                                                    TEST_CONN_STATE)
+            == 1);
+  tt_assert(connection_dir_count_by_purpose_resource_and_state(
+                                                    !conn->base_.purpose,
+                                                    "",
+                                                    !conn->base_.state)
+            == 0);
+  tt_assert(connection_dir_count_by_purpose_resource_and_state(
+                                                    !TEST_CONN_RSRC_PURPOSE,
+                                                    TEST_CONN_RSRC_2,
+                                                    !TEST_CONN_STATE)
+            == 0);
+
+ done:
+  ;
+}
+
+static void
+test_conn_download_status(void *arg)
+{
+  (void)arg;
+  dir_connection_t *conn = NULL;
+  dir_connection_t *conn2 = NULL;
+  dir_connection_t *conn3 = NULL;
+
+  /* no connections, no excess, not downloading */
+  tt_assert(networkstatus_consensus_has_excess_connections() == 0);
+  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0);
+
+  /* one connection, no excess, not downloading */
+  conn = test_conn_download_status_add_a_connection();
+  tt_assert(networkstatus_consensus_has_excess_connections() == 0);
+  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0);
+
+  /* one connection, no excess, but downloading */
+  conn->base_.state = TEST_CONN_DL_STATE;
+  tt_assert(networkstatus_consensus_has_excess_connections() == 0);
+  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1);
+  conn->base_.state = TEST_CONN_STATE;
+
+  /* two connections, excess, but not downloading */
+  conn2 = test_conn_download_status_add_a_connection();
+  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
+  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0);
+
+  /* two connections, excess, downloading */
+  conn2->base_.state = TEST_CONN_DL_STATE;
+  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
+  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1);
+  conn2->base_.state = TEST_CONN_STATE;
+
+  /* more connections, excess, but not downloading */
+  conn3 = test_conn_download_status_add_a_connection();
+  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
+  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0);
+
+  /* more connections, excess, downloading */
+  conn3->base_.state = TEST_CONN_DL_STATE;
+  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
+  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1);
+
+  /* more connections, more downloading */
+  conn2->base_.state = TEST_CONN_DL_STATE;
+  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
+  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1);
+
+  /* now try closing the one that isn't downloading:
+   * these tests won't work unless tor thinks it is bootstrapping */
+  tt_assert(networkstatus_consensus_is_boostrapping(time(NULL)));
+
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        TEST_CONN_RSRC) == 3);
+  tt_assert(connection_dir_close_consensus_conn_if_extra(conn) == -1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        TEST_CONN_RSRC) == 2);
+
+  /* now try closing one that is downloading - it stays open */
+  tt_assert(connection_dir_close_consensus_conn_if_extra(conn) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        TEST_CONN_RSRC) == 2);
+
+  /* now try closing all excess connections */
+  connection_dir_close_extra_consensus_conns();
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        TEST_CONN_RSRC) == 1);
+
+ done:
+  /* the teardown function removes all the connections */;
+}
+
+#define CONNECTION_TESTCASE(name, fork, setup)                           \
+  { #name, test_conn_##name, fork, &setup, NULL }
+
+struct testcase_t connection_tests[] = {
+  CONNECTION_TESTCASE(get_basic, TT_FORK, test_conn_get_basic_st),
+  CONNECTION_TESTCASE(get_rend,  TT_FORK, test_conn_get_rend_st),
+  CONNECTION_TESTCASE(get_rsrc,  TT_FORK, test_conn_get_rsrc_st),
+  CONNECTION_TESTCASE(download_status,  TT_FORK, test_conn_download_status_st),
+//CONNECTION_TESTCASE(func_suffix, TT_FORK, setup_func_pair),
+  END_OF_TESTCASES
+};
+