Abolish globals in entrynodes.c; relativize guard context to new guard_selection_t structure

src/or/circuitbuild.c

@@ -2213,7 +2213,6 @@ choose_good_entry_server(uint8_t purpose, cpath_build_state_t *state)
    * This is an incomplete fix, but is no worse than the previous behaviour,
    * and only applies to minimal, testing tor networks
    * (so it's no less secure) */
-  /*XXXX++ use the using_as_guard flag to accomplish this.*/
   if (options->UseEntryGuards
       && (!options->TestingTorNetwork ||
          smartlist_len(nodelist_get_list()) > smartlist_len(get_entry_guards())

src/or/entrynodes.h

@@ -16,6 +16,9 @@
 /* XXXX NM I would prefer that all of this stuff be private to
  * entrynodes.c. */
 
+/* Forward declare for guard_selection_t; entrynodes.c has the real struct */
+typedef struct guard_selection_s guard_selection_t;
+
 /** An entry_guard_t represents our information about a chosen long-term
  * first hop, known as a "helper" node in the literature. We can't just
  * use a node_t, since we want to remember these even when we
@@ -70,18 +73,27 @@ typedef struct entry_guard_t {
                                * this guard as first hop. */
 } entry_guard_t;
 
+entry_guard_t *entry_guard_get_by_id_digest_for_guard_selection(
+    guard_selection_t *gs, const char *digest);
 entry_guard_t *entry_guard_get_by_id_digest(const char *digest);
+void entry_guards_changed_for_guard_selection(guard_selection_t *gs);
 void entry_guards_changed(void);
+guard_selection_t * get_guard_selection_info(void);
+const smartlist_t *get_entry_guards_for_guard_selection(
+    guard_selection_t *gs);
 const smartlist_t *get_entry_guards(void);
+int num_live_entry_guards_for_guard_selection(
+    guard_selection_t *gs,
+    int for_directory);
 int num_live_entry_guards(int for_directory);
 
 #endif
 
 #ifdef ENTRYNODES_PRIVATE
-STATIC const node_t *add_an_entry_guard(const node_t *chosen,
+STATIC const node_t *add_an_entry_guard(guard_selection_t *gs,
+                                        const node_t *chosen,
                                         int reset_status, int prepend,
                                         int for_discovery, int for_directory);
-
 STATIC int populate_live_entry_guards(smartlist_t *live_entry_guards,
                                       const smartlist_t *all_entry_guards,
                                       const node_t *chosen_exit,
@@ -90,7 +102,8 @@ STATIC int populate_live_entry_guards(smartlist_t *live_entry_guards,
                                       int need_uptime, int need_capacity);
 STATIC int decide_num_guards(const or_options_t *options, int for_directory);
 
-STATIC void entry_guards_set_from_config(const or_options_t *options);
+STATIC void entry_guards_set_from_config(guard_selection_t *gs,
+                                         const or_options_t *options);
 
 /** Flags to be passed to entry_is_live() to indicate what kind of
  * entry nodes we are looking for. */
@@ -109,20 +122,32 @@ STATIC int entry_is_time_to_retry(const entry_guard_t *e, time_t now);
 
 #endif
 
+void remove_all_entry_guards_for_guard_selection(guard_selection_t *gs);
 void remove_all_entry_guards(void);
 
+void entry_guards_compute_status_for_guard_selection(
+    guard_selection_t *gs, const or_options_t *options, time_t now);
 void entry_guards_compute_status(const or_options_t *options, time_t now);
+int entry_guard_register_connect_status_for_guard_selection(
+    guard_selection_t *gs, const char *digest, int succeeded,
+    int mark_relay_status, time_t now);
 int entry_guard_register_connect_status(const char *digest, int succeeded,
                                         int mark_relay_status, time_t now);
+void entry_nodes_should_be_added_for_guard_selection(guard_selection_t *gs);
 void entry_nodes_should_be_added(void);
 int entry_list_is_constrained(const or_options_t *options);
 const node_t *choose_random_entry(cpath_build_state_t *state);
 const node_t *choose_random_dirguard(dirinfo_type_t t);
+int entry_guards_parse_state_for_guard_selection(
+    guard_selection_t *gs, or_state_t *state, int set, char **msg);
 int entry_guards_parse_state(or_state_t *state, int set, char **msg);
 void entry_guards_update_state(or_state_t *state);
 int getinfo_helper_entry_guards(control_connection_t *conn,
                                 const char *question, char **answer,
                                 const char **errmsg);
+int is_node_used_as_guard_for_guard_selection(guard_selection_t *gs,
+                                              const node_t *node);
+MOCK_DECL(int, is_node_used_as_guard, (const node_t *node));
 
 void mark_bridge_list(void);
 void sweep_bridge_list(void);

src/or/or.h

@@ -2363,9 +2363,6 @@ typedef struct node_t {
   /** Local info: we treat this node as if it rejects everything */
   unsigned int rejects_all:1;
 
-  /** Local info: this node is in our list of guards */
-  unsigned int using_as_guard:1;
-
   /* Local info: derived. */
 
   /** True if the IPv6 OR port is preferred over the IPv4 OR port.

src/or/routerlist.c

@@ -1953,9 +1953,9 @@ router_pick_directory_server_impl(dirinfo_type_t type, int flags,
         !router_supports_extrainfo(node->identity, is_trusted_extrainfo))
       continue;
     /* Don't make the same node a guard twice */
-    if (for_guard && node->using_as_guard) {
-      continue;
-    }
+     if (for_guard && is_node_used_as_guard(node)) {
+       continue;
+     }
     /* Ensure that a directory guard is actually a guard node. */
     if (for_guard && !node->is_possible_guard) {
       continue;

src/test/test_entrynodes.c

@@ -254,7 +254,9 @@ populate_live_entry_guards_test_helper(int num_needed)
 {
   smartlist_t *our_nodelist = NULL;
   smartlist_t *live_entry_guards = smartlist_new();
-  const smartlist_t *all_entry_guards = get_entry_guards();
+  guard_selection_t *gs = get_guard_selection_info();
+  const smartlist_t *all_entry_guards =
+    get_entry_guards_for_guard_selection(gs);
   or_options_t *options = get_options_mutable();
   int retval;
 
@@ -271,7 +273,7 @@ populate_live_entry_guards_test_helper(int num_needed)
 
   SMARTLIST_FOREACH_BEGIN(our_nodelist, const node_t *, node) {
     const node_t *node_tmp;
-    node_tmp = add_an_entry_guard(node, 0, 1, 0, 0);
+    node_tmp = add_an_entry_guard(gs, node, 0, 1, 0, 0);
     tt_assert(node_tmp);
   } SMARTLIST_FOREACH_END(node);
 
@@ -582,7 +584,9 @@ static void
 test_entry_guards_set_from_config(void *arg)
 {
   or_options_t *options = get_options_mutable();
-  const smartlist_t *all_entry_guards = get_entry_guards();
+  guard_selection_t *gs = get_guard_selection_info();
+  const smartlist_t *all_entry_guards =
+    get_entry_guards_for_guard_selection(gs);
   const char *entrynodes_str = "test003r";
   const node_t *chosen_entry = NULL;
   int retval;
@@ -597,7 +601,7 @@ test_entry_guards_set_from_config(void *arg)
   tt_int_op(retval, OP_GE, 0);
 
   /* Read nodes from EntryNodes */
-  entry_guards_set_from_config(options);
+  entry_guards_set_from_config(gs, options);
 
   /* Test that only one guard was added. */
   tt_int_op(smartlist_len(all_entry_guards), OP_EQ, 1);
@@ -689,7 +693,9 @@ static void
 test_entry_is_live(void *arg)
 {
   smartlist_t *our_nodelist = NULL;
-  const smartlist_t *all_entry_guards = get_entry_guards();
+  guard_selection_t *gs = get_guard_selection_info();
+  const smartlist_t *all_entry_guards =
+    get_entry_guards_for_guard_selection(gs);
   const node_t *test_node = NULL;
   const entry_guard_t *test_entry = NULL;
   const char *msg;
@@ -706,7 +712,7 @@ test_entry_is_live(void *arg)
 
   SMARTLIST_FOREACH_BEGIN(our_nodelist, const node_t *, node) {
     const node_t *node_tmp;
-    node_tmp = add_an_entry_guard(node, 0, 1, 0, 0);
+    node_tmp = add_an_entry_guard(gs, node, 0, 1, 0, 0);
     tt_assert(node_tmp);
 
     tt_int_op(node->is_stable, OP_EQ, 0);

src/test/test_routerlist.c

@@ -15,6 +15,7 @@
 #include "container.h"
 #include "directory.h"
 #include "dirvote.h"
+#include "entrynodes.h"
 #include "microdesc.h"
 #include "networkstatus.h"
 #include "nodelist.h"
@@ -203,6 +204,53 @@ mock_usable_consensus_flavor(void)
   return mock_usable_consensus_flavor_value;
 }
 
+static smartlist_t *mock_is_guard_list = NULL;
+
+static int
+mock_is_node_used_as_guard(node_t *n)
+{
+  if (mock_is_guard_list) {
+    SMARTLIST_FOREACH_BEGIN(mock_is_guard_list, node_t *, e) {
+      if (e == n) return 1;
+    } SMARTLIST_FOREACH_END(e);
+  }
+
+  return 0;
+}
+
+static void
+mark_node_used_as_guard(node_t *n)
+{
+  if (!n) return;
+
+  if (!mock_is_guard_list) {
+    mock_is_guard_list = smartlist_new();
+  }
+
+  if (!mock_is_node_used_as_guard(n)) {
+    smartlist_add(mock_is_guard_list, n);
+  }
+}
+
+static void
+mark_node_unused_as_guard(node_t *n)
+{
+  if (!n) return;
+
+  if (!mock_is_guard_list) return;
+
+  smartlist_remove(mock_is_guard_list, n);
+}
+
+static void
+clear_mock_guard_list(void)
+{
+  if (mock_is_guard_list) {
+    smartlist_free(mock_is_guard_list);
+    mock_is_guard_list = NULL;
+  }
+}
+
 static void
 test_router_pick_directory_server_impl(void *arg)
 {
@@ -223,6 +271,7 @@ test_router_pick_directory_server_impl(void *arg)
   (void)arg;
 
   MOCK(usable_consensus_flavor, mock_usable_consensus_flavor);
+  MOCK(is_node_used_as_guard, mock_is_node_used_as_guard);
 
   /* With no consensus, we must be bootstrapping, regardless of time or flavor
    */
@@ -336,28 +385,28 @@ test_router_pick_directory_server_impl(void *arg)
   node_router3->is_valid = 1;
 
   flags |= PDS_FOR_GUARD;
-  node_router1->using_as_guard = 1;
-  node_router2->using_as_guard = 1;
-  node_router3->using_as_guard = 1;
+  mark_node_used_as_guard(node_router1);
+  mark_node_used_as_guard(node_router2);
+  mark_node_used_as_guard(node_router3);
   rs = router_pick_directory_server_impl(V3_DIRINFO, flags, NULL);
   tt_assert(rs == NULL);
-  node_router1->using_as_guard = 0;
+  mark_node_unused_as_guard(node_router1);
   rs = router_pick_directory_server_impl(V3_DIRINFO, flags, NULL);
   tt_assert(rs != NULL);
   tt_assert(tor_memeq(rs->identity_digest, router1_id, DIGEST_LEN));
   rs = NULL;
-  node_router2->using_as_guard = 0;
-  node_router3->using_as_guard = 0;
+  mark_node_unused_as_guard(node_router2);
+  mark_node_unused_as_guard(node_router3);
 
   /* One not valid, one guard. This should leave one remaining */
   node_router1->is_valid = 0;
-  node_router2->using_as_guard = 1;
+  mark_node_used_as_guard(node_router2);
   rs = router_pick_directory_server_impl(V3_DIRINFO, flags, NULL);
   tt_assert(rs != NULL);
   tt_assert(tor_memeq(rs->identity_digest, router3_id, DIGEST_LEN));
   rs = NULL;
   node_router1->is_valid = 1;
-  node_router2->using_as_guard = 0;
+  mark_node_unused_as_guard(node_router2);
 
   /* Manipulate overloaded */
 
@@ -420,6 +469,9 @@ test_router_pick_directory_server_impl(void *arg)
 
  done:
   UNMOCK(usable_consensus_flavor);
+  UNMOCK(is_node_used_as_guard);
+  clear_mock_guard_list();
+
   if (router1_id)
     tor_free(router1_id);
   if (router2_id)