Move last_reachable and testing_since from routerinfo_t to node_t.

changes/bug5529

@@ -0,0 +1,3 @@
+  o Code refactoring:
+    - Move last_reachable and testing_since from routerinfo_t to
+      node_t.  Implements enhancement 5529.

src/or/dirserv.c

@@ -988,7 +988,7 @@ dirserv_set_router_is_running(routerinfo_t *router, time_t now)
     answer = ! we_are_hibernating();
   } else if (router->is_hibernating &&
              (router->cache_info.published_on +
-              HIBERNATION_PUBLICATION_SKEW) > router->last_reachable) {
+              HIBERNATION_PUBLICATION_SKEW) > node->last_reachable) {
     /* A hibernating router is down unless we (somehow) had contact with it
      * since it declared itself to be hibernating. */
     answer = 0;
@@ -998,7 +998,7 @@ dirserv_set_router_is_running(routerinfo_t *router, time_t now)
   } else {
     /* Otherwise, a router counts as up if we found it reachable in the last
        REACHABLE_TIMEOUT seconds. */
-    answer = (now < router->last_reachable + REACHABLE_TIMEOUT);
+    answer = (now < node->last_reachable + REACHABLE_TIMEOUT);
   }
 
   if (!answer && running_long_enough_to_decide_unreachable()) {
@@ -1010,9 +1010,9 @@ dirserv_set_router_is_running(routerinfo_t *router, time_t now)
        it.
      */
     time_t when = now;
-    if (router->last_reachable &&
+    if (node->last_reachable &&
-        router->last_reachable + REACHABILITY_TEST_CYCLE_PERIOD < now)
+        node->last_reachable + REACHABILITY_TEST_CYCLE_PERIOD < now)
-      when = router->last_reachable + REACHABILITY_TEST_CYCLE_PERIOD;
+      when = node->last_reachable + REACHABILITY_TEST_CYCLE_PERIOD;
     rep_hist_note_router_unreachable(router->cache_info.identity_digest, when);
   }
 
@@ -3277,15 +3277,17 @@ dirserv_orconn_tls_done(const char *address,
                         uint16_t or_port,
                         const char *digest_rcvd)
 {
-  routerinfo_t *ri;
+  node_t *node = NULL;
+  routerinfo_t *ri = NULL;
   time_t now = time(NULL);
   tor_assert(address);
   tor_assert(digest_rcvd);
 
-  ri = router_get_mutable_by_digest(digest_rcvd);
+  node = node_get_mutable_by_id(digest_rcvd);
-
+  if (node == NULL)
-  if (ri == NULL)
     return;
+  ri = node->ri;
+  tor_assert(ri);
 
   if (!strcasecmp(address, ri->address) && or_port == ri->or_port) {
     /* Found the right router.  */
@@ -3302,7 +3304,7 @@ dirserv_orconn_tls_done(const char *address,
       else
         log_warn(LD_BUG, "Couldn't parse IP address \"%s\"", ri->address);
       rep_hist_note_router_reachable(digest_rcvd, addrp, or_port, now);
-      ri->last_reachable = now;
+      node->last_reachable = now;
     }
   }
 }
@@ -3338,12 +3340,17 @@ dirserv_should_launch_reachability_test(const routerinfo_t *ri,
 void
 dirserv_single_reachability_test(time_t now, routerinfo_t *router)
 {
+  node_t *node = NULL;
   tor_addr_t router_addr;
+
+  tor_assert(router);
+  node = node_get_mutable_by_id(router->cache_info.identity_digest);
+  tor_assert(node);
   log_debug(LD_OR,"Testing reachability of %s at %s:%u.",
             router->nickname, router->address, router->or_port);
   /* Remember when we started trying to determine reachability */
-  if (!router->testing_since)
+  if (!node->testing_since)
-    router->testing_since = now;
+    node->testing_since = now;
   tor_addr_from_ipv4h(&router_addr, router->addr);
   connection_or_connect(&router_addr, router->or_port,
                         router->cache_info.identity_digest);

src/or/nodelist.c

@@ -115,13 +115,58 @@ node_get_or_create(const char *identity_digest)
   return node;
 }
 
-/** Add <b>ri</b> to the nodelist. */
+/** Replace <b>old</b> router with <b>new</b> in nodelist.  If
+ *  <b>old</b> and <b>new</b> in fact are the same relays (having the
+ *  same identity_digest) the node_t of <b>old</b> is used for
+ *  <b>new</b>.  Otherwise the node_t of <b>old</b> is dropped and
+ *  <b>new</b> gets a new one (which might be a recycled node_t in
+ *  case we already have one matching its identity).
+ */
 node_t *
-nodelist_add_routerinfo(routerinfo_t *ri)
+nodelist_replace_routerinfo(routerinfo_t *old, routerinfo_t *new)
 {
-  node_t *node;
+  node_t *node = NULL;
+  tor_assert(old);
+  tor_assert(new);
+
+  if (tor_memeq(old->cache_info.identity_digest,
+                new->cache_info.identity_digest, DIGEST_LEN)) {
+    /* NEW == OLD, reuse node_t.  */
+    node = node_get_mutable_by_id(old->cache_info.identity_digest);
+    if (node) {
+      tor_assert(node->ri == old);
+      /* XXXX prop186 we may have more than one address.  */
+      if (!routers_have_same_or_addr(old, new)) {
+        /* These mustn't carry over when the address and orport
+           change. */
+        node->last_reachable = 0;
+        node->testing_since = 0;
+      }
+    }
+  } else {
+    /* NEW != OLD, get a new node_t.  */
+    nodelist_remove_routerinfo(old);
+  }
+  node = nodelist_add_routerinfo(node, new);
+
+  return node;
+}
+
+
+/** Add <b>ri</b> to the nodelist.  If <b>node_in</b> is not NULL, use
+    that node rather than creating a new.  */
+node_t *
+nodelist_add_routerinfo(node_t *node_in, routerinfo_t *ri)
+{
+  node_t *node = NULL;
+
+  if (node_in) {
+    node = node_in;
+  } else {
+    tor_assert(ri);
     init_nodelist();
     node = node_get_or_create(ri->cache_info.identity_digest);
+  }
   node->ri = ri;
 
   if (node->country == -1)

src/or/nodelist.h

@@ -15,7 +15,8 @@
 node_t *node_get_mutable_by_id(const char *identity_digest);
 const node_t *node_get_by_id(const char *identity_digest);
 const node_t *node_get_by_hex_id(const char *identity_digest);
-node_t *nodelist_add_routerinfo(routerinfo_t *ri);
+node_t *nodelist_replace_routerinfo(routerinfo_t *old, routerinfo_t *new);
+node_t *nodelist_add_routerinfo(node_t *node, routerinfo_t *ri);
 node_t *nodelist_add_microdesc(microdesc_t *md);
 void nodelist_set_consensus(networkstatus_t *ns);
 

src/or/or.h

@@ -1793,15 +1793,6 @@ typedef struct {
    * things; see notes on ROUTER_PURPOSE_* macros above.
    */
   uint8_t purpose;
-
-  /* The below items are used only by authdirservers for
-   * reachability testing. */
-
-  /** When was the last time we could reach this OR? */
-  time_t last_reachable;
-  /** When did we start testing reachability for this OR? */
-  time_t testing_since;
-
 } routerinfo_t;
 
 /** Information needed to keep and cache a signed extra-info document. */
@@ -2037,6 +2028,14 @@ typedef struct node_t {
 
   /** According to the geoip db what country is this router in? */
   country_t country;
+
+  /* The below items are used only by authdirservers for
+   * reachability testing. */
+
+  /** When was the last time we could reach this OR? */
+  time_t last_reachable;        /* IPv4 */
+  /** When did we start testing reachability for this OR? */
+  time_t testing_since;         /* IPv4 */
 } node_t;
 
 /** How many times will we try to download a router's descriptor before giving

src/or/routerlist.c

@@ -2875,7 +2875,7 @@ routerlist_insert(routerlist_t *rl, routerinfo_t *ri)
               &ri->cache_info);
   smartlist_add(rl->routers, ri);
   ri->cache_info.routerlist_index = smartlist_len(rl->routers) - 1;
-  nodelist_add_routerinfo(ri);
+  nodelist_add_routerinfo(NULL, ri);
   router_dir_info_changed();
 #ifdef DEBUG_ROUTERLIST
   routerlist_assert_ok(rl);
@@ -3104,8 +3104,7 @@ routerlist_replace(routerlist_t *rl, routerinfo_t *ri_old,
   tor_assert(0 <= idx && idx < smartlist_len(rl->routers));
   tor_assert(smartlist_get(rl->routers, idx) == ri_old);
 
-  nodelist_remove_routerinfo(ri_old);
+  nodelist_replace_routerinfo(ri_old, ri_new);
-  nodelist_add_routerinfo(ri_new);
 
   router_dir_info_changed();
   if (idx >= 0) {
@@ -3442,11 +3441,6 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
       /* Same key, and either new, or listed in the consensus. */
       log_debug(LD_DIR, "Replacing entry for router %s",
                 router_describe(router));
-      if (routers_have_same_or_addr(router, old_router)) {
-        /* these carry over when the address and orport are unchanged. */
-        router->last_reachable = old_router->last_reachable;
-        router->testing_since = old_router->testing_since;
-      }
       routerlist_replace(routerlist, old_router, router);
       if (!from_cache) {
         signed_desc_append_to_journal(&router->cache_info,