Rename and tweak nodelist_add_node_family() to add node

It's very easy for nodelist_add_node_family(sl,node) to accidentally
add 'node', and kind of hard to make sure that it omits it.  Instead
of taking pains to leave 'node' out, let's instead make sure that we
always include it.

I also rename the function to nodelist_add_node_and_family, and
audit its users so that they don't add the node itself any longer,
since the function will take care of that for them.

Resolves bug 2616, which was not actually a bug.

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 90d92802ea75e7e8b81612d9bbbe5a3088559011..df335571af5937cc8871ab6c9610c245d40d21ac 100644 (file)
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -3102,13 +3102,11 @@ choose_good_middle_server(uint8_t purpose,
   log_debug(LD_CIRC, "Contemplating intermediate hop: random choice.");
   excluded = smartlist_create();
   if ((r = build_state_get_exit_node(state))) {
-    smartlist_add(excluded, (void*) r);
-    nodelist_add_node_family(excluded, r);
+    nodelist_add_node_and_family(excluded, r);
   }
   for (i = 0, cpath = head; i < cur_len; ++i, cpath=cpath->next) {
     if ((r = node_get_by_id(cpath->extend_info->identity_digest))) {
-      smartlist_add(excluded, (void*)r);
-      nodelist_add_node_family(excluded, r);
+      nodelist_add_node_and_family(excluded, r);
     }
   }
 
@@ -3152,8 +3150,7 @@ choose_good_entry_server(uint8_t purpose, cpath_build_state_t *state)
   if (state && (node = build_state_get_exit_node(state))) {
     /* Exclude the exit node from the state, if we have one.  Also exclude its
      * family. */
-    smartlist_add(excluded, (void*)node);
-    nodelist_add_node_family(excluded, node);
+    nodelist_add_node_and_family(excluded, node);
   }
   if (firewall_is_fascist_or()) {
     /* Exclude all ORs that we can't reach through our firewall */
@@ -3168,8 +3165,7 @@ choose_good_entry_server(uint8_t purpose, cpath_build_state_t *state)
     SMARTLIST_FOREACH(entry_guards, entry_guard_t *, entry,
       {
         if ((node = node_get_by_id(entry->identity))) {
-          smartlist_add(excluded, (void*)node);
-          nodelist_add_node_family(excluded, node);
+          nodelist_add_node_and_family(excluded, node);
         }
       });
   }
@@ -4094,7 +4090,7 @@ choose_random_entry(cpath_build_state_t *state)
   int preferred_min, consider_exit_family = 0;
 
   if (chosen_exit) {
-    nodelist_add_node_family(exit_family, chosen_exit);
+    nodelist_add_node_and_family(exit_family, chosen_exit);
     consider_exit_family = 1;
   }
 

diff --git a/src/or/nodelist.h b/src/or/nodelist.h
index 08f11da59132ef240873375e39cb3fcad1e0594a..bd2e63953c3d7c98d6c3ea3db58309547ba31665 100644 (file)
--- a/src/or/nodelist.h
+++ b/src/or/nodelist.h
@@ -53,7 +53,7 @@ smartlist_t *nodelist_get_list(void);
 /* XXXX These need to move out of routerlist.c */
 void nodelist_refresh_countries(void);
 void node_set_country(node_t *node);
-void nodelist_add_node_family(smartlist_t *nodes, const node_t *node);
+void nodelist_add_node_and_family(smartlist_t *nodes, const node_t *node);
 int nodes_in_same_family(const node_t *node1, const node_t *node2);
 
 #endif

diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index 5aaa4a9614ce010610805f445b35fb681b142e9c..c0a233ceede671cc3d3295c03c7adc9397be9680 100644 (file)
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -1357,12 +1357,17 @@ addrs_in_same_network_family(const tor_addr_t *a1,
   return 0 == tor_addr_compare_masked(a1, a2, 16, CMP_SEMANTIC);
 }
 
-/** Add all the family of <b>router</b> to the smartlist <b>sl</b>.
- * This is used to make sure we don't pick siblings in a single path,
- * or pick more than one relay from a family for our entry guard list.
+/**
+ * Add all the family of <b>node</b>, including <b>node</b> itself, to
+ * the smartlist <b>sl</b>.
+ *
+ * This is used to make sure we don't pick siblings in a single path, or
+ * pick more than one relay from a family for our entry guard list.
+ * Note that a node may be added to <b>sl</b> more than once if it is
+ * part of <b>node</b>'s family for more than one reason.
  */
 void
-nodelist_add_node_family(smartlist_t *sl, const node_t *node)
+nodelist_add_node_and_family(smartlist_t *sl, const node_t *node)
 {
   /* XXXX MOVE */
   const smartlist_t *all_nodes = nodelist_get_list();
@@ -1373,6 +1378,13 @@ nodelist_add_node_family(smartlist_t *sl, const node_t *node)
 
   declared_family = node_get_declared_family(node);
 
+  /* Let's make sure that we have the node itself, if it's a real node. */
+  {
+    const node_t *real_node = node_get_by_id(node->identity);
+    if (real_node)
+      smartlist_add(sl, (node_t*)real_node);
+  }
+
   /* First, add any nodes with similar network addresses. */
   if (options->EnforceDistinctSubnets) {
     tor_addr_t node_addr;
@@ -1417,13 +1429,14 @@ nodelist_add_node_family(smartlist_t *sl, const node_t *node)
   }
 }
 
-/** Given a <b>router</b>, add every node_t in its family to <b>sl</b>.
+/** Given a <b>router</b>, add every node_t in its family (including the
+ * node itself</b>) to <b>sl</b>.
  *
  * Note the type mismatch: This function takes a routerinfo, but adds nodes
  * to the smartlist!
  */
 static void
-routerlist_add_nodes_in_family(smartlist_t *sl, const routerinfo_t *router)
+routerlist_add_node_and_family(smartlist_t *sl, const routerinfo_t *router)
 {
   /* XXXX MOVE ? */
   node_t fake_node;
@@ -1434,7 +1447,7 @@ routerlist_add_nodes_in_family(smartlist_t *sl, const routerinfo_t *router)
     memcpy(fake_node.identity, router->cache_info.identity_digest, DIGEST_LEN);
     node = &fake_node;
   }
-  nodelist_add_node_family(sl, node);
+  nodelist_add_node_and_family(sl, node);
 }
 
 /** Return true iff <b>node</b> is named by some nickname in <b>lst</b>. */
@@ -2171,12 +2184,8 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
       });
   }
 
-  if ((r = routerlist_find_my_routerinfo())) {
-    const node_t *me = node_get_by_id(r->cache_info.identity_digest);
-    if (me)
-      smartlist_add(excludednodes, (void *)me);
-    routerlist_add_nodes_in_family(excludednodes, r);
-  }
+  if ((r = routerlist_find_my_routerinfo()))
+    routerlist_add_node_and_family(excludednodes, r);
 
   router_add_running_nodes_to_smartlist(sl, allow_invalid,
                                         need_uptime, need_capacity,

diff --git a/src/or/routerlist.h b/src/or/routerlist.h
index 3a8af6fd9d62aae05fbed2d196af6bb3f153f3f1..11290468d8a21c2b584c795a26b46c22ab38afa1 100644 (file)
--- a/src/or/routerlist.h
+++ b/src/or/routerlist.h
@@ -36,7 +36,6 @@ const routerstatus_t *router_pick_trusteddirserver(dirinfo_type_t type,
 int router_get_my_share_of_directory_requests(double *v2_share_out,
                                               double *v3_share_out);
 void router_reset_status_download_failures(void);
-void routerlist_add_family(smartlist_t *sl, const routerinfo_t *router);
 int routers_have_same_or_addr(const routerinfo_t *r1, const routerinfo_t *r2);
 int router_nickname_is_in_list(const routerinfo_t *router, const char *list);
 const routerinfo_t *routerlist_find_my_routerinfo(void);