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.

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;
   }
 

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

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,

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);