[tor-commits] [tor/master] nodelist: Stop recursing in router_choose_random_node()

[nickm at torproject.org]

commit 3f7f976d4812a92bdbf5f14e25db0d276f123cef
Author: teor <teor at riseup.net>
Date:   Mon May 11 17:58:28 2020 +1000

    nodelist: Stop recursing in router_choose_random_node()
    
    Instead, call out to a helper function, repeating the call if needed.
    
    Avoids duplicating exclusions for:
      * the current relay's family, and
      * any exclusions specified by the caller.
    
    Part of 34200.
---
 src/feature/nodelist/node_select.c | 69 ++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 22 deletions(-)

diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c
index 11abc84d4..85ef5ad56 100644
--- a/src/feature/nodelist/node_select.c
+++ b/src/feature/nodelist/node_select.c
@@ -930,6 +930,42 @@ nodelist_subtract(smartlist_t *sl, const smartlist_t *excluded)
   bitarray_free(excluded_idx);
 }
 
+/* Node selection helper for router_choose_random_node().
+ *
+ * Populates a node list based on <b>flags</b>, ignoring nodes in
+ * <b>excludednodes</b> and <b>excludedset</b>. Chooses the node based on
+ * <b>rule</b>. */
+static const node_t *
+router_choose_random_node_helper(smartlist_t *excludednodes,
+                                 routerset_t *excludedset,
+                                 router_crn_flags_t flags,
+                                 bandwidth_weight_rule_t rule)
+{
+  smartlist_t *sl=smartlist_new();
+  const node_t *choice = NULL;
+
+  router_add_running_nodes_to_smartlist(sl, flags);
+  log_debug(LD_CIRC,
+           "We found %d running nodes.",
+            smartlist_len(sl));
+
+  nodelist_subtract(sl, excludednodes);
+
+  if (excludedset) {
+    routerset_subtract_nodes(sl,excludedset);
+    log_debug(LD_CIRC,
+              "We removed excludedset, leaving %d nodes.",
+              smartlist_len(sl));
+  }
+
+  // Always weight by bandwidth
+  choice = node_sl_choose_by_bandwidth(sl, rule);
+
+  smartlist_free(sl);
+
+  return choice;
+}
+
 /** Return a random running node from the nodelist. Never pick a node that is
  * in <b>excludedsmartlist</b>, or which matches <b>excludedset</b>, even if
  * they are the only nodes available.
@@ -942,15 +978,14 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
                           routerset_t *excludedset,
                           router_crn_flags_t flags)
 {
-  /* A limited set of flags, used for weighting and fallback node selection.
+  /* A limited set of flags, used for fallback node selection.
    */
   const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
   const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
   const int need_guard = (flags & CRN_NEED_GUARD) != 0;
   const int pref_addr = (flags & CRN_PREF_ADDR) != 0;
 
-  smartlist_t *sl=smartlist_new(),
-    *excludednodes=smartlist_new();
+  smartlist_t *excludednodes=smartlist_new();
   const node_t *choice = NULL;
   const routerinfo_t *r;
   bandwidth_weight_rule_t rule;
@@ -963,29 +998,17 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   if ((r = router_get_my_routerinfo()))
     routerlist_add_node_and_family(excludednodes, r);
 
-  router_add_running_nodes_to_smartlist(sl, flags);
-  log_debug(LD_CIRC,
-           "We found %d running nodes.",
-            smartlist_len(sl));
-
   if (excludedsmartlist) {
     smartlist_add_all(excludednodes, excludedsmartlist);
   }
-  nodelist_subtract(sl, excludednodes);
 
-  if (excludedset) {
-    routerset_subtract_nodes(sl,excludedset);
-    log_debug(LD_CIRC,
-              "We removed excludedset, leaving %d nodes.",
-              smartlist_len(sl));
-  }
+  choice = router_choose_random_node_helper(excludednodes,
+                                            excludedset,
+                                            flags,
+                                            rule);
 
-  // Always weight by bandwidth
-  choice = node_sl_choose_by_bandwidth(sl, rule);
-
-  smartlist_free(sl);
   if (!choice && (need_uptime || need_capacity || need_guard || pref_addr)) {
-    /* try once more -- recurse but with fewer restrictions. */
+    /* try once more, with fewer restrictions. */
     log_info(LD_CIRC,
              "We couldn't find any live%s%s%s%s routers; falling back "
              "to list of all routers.",
@@ -995,8 +1018,10 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
              pref_addr?", preferred address":"");
     flags &= ~ (CRN_NEED_UPTIME|CRN_NEED_CAPACITY|CRN_NEED_GUARD|
                 CRN_PREF_ADDR);
-    choice = router_choose_random_node(
-                     excludedsmartlist, excludedset, flags);
+    choice = router_choose_random_node_helper(excludednodes,
+                                              excludedset,
+                                              flags,
+                                              rule);
   }
   smartlist_free(excludednodes);
   if (!choice) {