Move ipv6_preferred from routerinfo_t to node_t.

Move extend_info_from_router() from circuitbuild.c to router.c and
make it static.

Add get_configured_bridge_by_orports_digest() and have
get_configured_bridge_by_routerinfo() and
node_is_a_configured_bridge() use it. We now consider all OR ports of
a bridge when looking for it.

Move node_get_*_orport to nodelist.c.

Fix a cut'n'paste error in header of nodelist.h.

Add node_assert_ok().

Add router_get_all_orports(). It's duplicating code from
node_get_all_orports(). Worth fixing at the cost of complicating the
API slightly?

changes/bug4620

@@ -0,0 +1,3 @@
+  o Code simplifications and refactoring
+    - Move ipv6_preferred from routerinfo_t to node_t.
+      Addresses bug 4620.

src/or/circuitbuild.c

@@ -3825,24 +3825,6 @@ extend_info_alloc(const char *nickname, const char *digest,
   return info;
 }
 
-/** Allocate and return a new extend_info_t that can be used to build
- * a circuit to or through the router <b>r</b>. Use the primary
- * address of the router unless <b>for_direct_connect</b> is true, in
- * which case the preferred address is used instead. */
-extend_info_t *
-extend_info_from_router(const routerinfo_t *r, int for_direct_connect)
-{
-  tor_addr_port_t ap;
-  tor_assert(r);
-
-  if (for_direct_connect)
-    router_get_pref_orport(r, &ap);
-  else
-    router_get_prim_orport(r, &ap);
-  return extend_info_alloc(r->nickname, r->cache_info.identity_digest,
-                           r->onion_pkey, &ap.addr, ap.port);
-}
-
 /** Allocate and return a new extend_info that can be used to build a
  * circuit to or through the node <b>node</b>. Use the primary address
  * of the node unless <b>for_direct_connect</b> is true, in which case
@@ -3854,7 +3836,14 @@ extend_info_t *
 extend_info_from_node(const node_t *node, int for_direct_connect)
 {
   if (node->ri) {
-    return extend_info_from_router(node->ri, for_direct_connect);
+    const routerinfo_t *r = node->ri;
+    tor_addr_port_t ap;
+    if (for_direct_connect)
+      node_get_pref_orport(node, &ap);
+    else
+      node_get_prim_orport(node, &ap);
+    return extend_info_alloc(r->nickname, r->cache_info.identity_digest,
+                             r->onion_pkey, &ap.addr, ap.port);
   } else if (node->rs && node->md) {
     tor_addr_t addr;
     tor_addr_from_ipv4h(&addr, node->rs->addr);
@@ -5137,10 +5126,36 @@ bridge_free(bridge_info_t *bridge)
   tor_free(bridge);
 }
 
+/** Return a bridge pointer if either <b>digest</b>, if possible, or
+ * any of the tor_addr_port_t's in <b>orports</b>, if necessary,
+ * matches any of our known bridges.  Else return NULL. */
+static bridge_info_t *
+get_configured_bridge_by_orports_digest(const char *digest,
+                                        const smartlist_t *orports)
+{
+  if (!bridge_list)
+    return NULL;
+  SMARTLIST_FOREACH_BEGIN(bridge_list, bridge_info_t *, bridge)
+    {
+      if (tor_digest_is_zero(bridge->identity)) {
+        SMARTLIST_FOREACH_BEGIN(orports, tor_addr_port_t *, ap)
+          {
+            if (tor_addr_compare(&bridge->addr, &ap->addr, CMP_EXACT) == 0 &&
+                bridge->port == ap->port)
+              return bridge;
+          }
+        SMARTLIST_FOREACH_END(ap);
+      }
+      if (digest && tor_memeq(bridge->identity, digest, DIGEST_LEN))
+        return bridge;
+    }
+  SMARTLIST_FOREACH_END(bridge);
+  return NULL;
+}
 
-/** Return a bridge pointer if <b>ri</b> is one of our known bridges
- * (either by comparing keys if possible, else by comparing addr/port).
- * Else return NULL. */
+/** Return a bridge pointer if <b>digest</b>, if possible, or
+ * <b>addr</b> and <b>port</b>, if necessary, matches any of our known
+ * bridges.  Else return NULL. */
 static bridge_info_t *
 get_configured_bridge_by_addr_port_digest(const tor_addr_t *addr,
                                           uint16_t port,
@@ -5166,11 +5181,13 @@ get_configured_bridge_by_addr_port_digest(const tor_addr_t *addr,
 static bridge_info_t *
 get_configured_bridge_by_routerinfo(const routerinfo_t *ri)
 {
-  tor_addr_port_t ap;
-
-  router_get_pref_orport(ri, &ap);
-  return get_configured_bridge_by_addr_port_digest(&ap.addr, ap.port,
-                                               ri->cache_info.identity_digest);
+  bridge_info_t *bi = NULL;
+  smartlist_t *orports = router_get_all_orports(ri);
+  bi = get_configured_bridge_by_orports_digest(ri->cache_info.identity_digest,
+                                               orports);
+  SMARTLIST_FOREACH(orports, tor_addr_port_t *, p, tor_free(p));
+  smartlist_free(orports);
+  return bi;
 }
 
 /** Return 1 if <b>ri</b> is one of our known bridges, else 0. */
@@ -5184,30 +5201,12 @@ routerinfo_is_a_configured_bridge(const routerinfo_t *ri)
 int
 node_is_a_configured_bridge(const node_t *node)
 {
-  int retval = 0;               /* Negative.  */
-  smartlist_t *orports = NULL;
-
-  if (!node)
-    goto out;
-
-  orports = node_get_all_orports(node);
-  if (orports == NULL)
-    goto out;
-
-  SMARTLIST_FOREACH_BEGIN(orports, tor_addr_port_t *, orport) {
-    if (get_configured_bridge_by_addr_port_digest(&orport->addr, orport->port,
-                                                  node->identity) != NULL) {
-      retval = 1;
-      goto out;
-    }
-  } SMARTLIST_FOREACH_END(orport);
-
- out:
-  if (orports != NULL) {
-    SMARTLIST_FOREACH(orports, tor_addr_port_t *, p, tor_free(p));
-    smartlist_free(orports);
-    orports = NULL;
-  }
+  int retval = 0;
+  smartlist_t *orports = node_get_all_orports(node);
+  retval = get_configured_bridge_by_orports_digest(node->identity,
+                                                   orports) != NULL;
+  SMARTLIST_FOREACH(orports, tor_addr_port_t *, p, tor_free(p));
+  smartlist_free(orports);
   return retval;
 }
 
@@ -5572,18 +5571,18 @@ rewrite_node_address_for_bridge(const bridge_info_t *bridge, node_t *node)
     /* Indicate that we prefer connecting to this bridge over the
        protocol that the bridge address indicates.  Last bridge
        descriptor handled wins.  */
-    ri->ipv6_preferred = tor_addr_family(&bridge->addr) == AF_INET6;
+    node->ipv6_preferred = tor_addr_family(&bridge->addr) == AF_INET6;
 
     /* XXXipv6 we lack support for falling back to another address for
        the same relay, warn the user */
     if (!tor_addr_is_null(&ri->ipv6_addr)) {
       tor_addr_port_t ap;
-      router_get_pref_orport(ri, &ap);
+      node_get_pref_orport(node, &ap);
       log_notice(LD_CONFIG,
                  "Bridge '%s' has both an IPv4 and an IPv6 address.  "
                  "Will prefer using its %s address (%s:%d).",
                  ri->nickname,
-                 ri->ipv6_preferred ? "IPv6" : "IPv4",
+                 node->ipv6_preferred ? "IPv6" : "IPv4",
                  fmt_addr(&ap.addr), ap.port);
     }
   }

src/or/circuitbuild.h

@@ -44,10 +44,7 @@ void onion_append_to_cpath(crypt_path_t **head_ptr, crypt_path_t *new_hop);
 extend_info_t *extend_info_alloc(const char *nickname, const char *digest,
                                  crypto_pk_t *onion_key,
                                  const tor_addr_t *addr, uint16_t port);
-extend_info_t *extend_info_from_router(const routerinfo_t *r,
-                                       int for_direct_connect);
-extend_info_t *extend_info_from_node(const node_t *node,
-                                     int for_direct_connect);
+extend_info_t *extend_info_from_node(const node_t *r, int for_direct_connect);
 extend_info_t *extend_info_dup(extend_info_t *info);
 void extend_info_free(extend_info_t *info);
 const node_t *build_state_get_exit_node(cpath_build_state_t *state);

src/or/nodelist.c

@@ -711,19 +711,6 @@ node_get_all_orports(const node_t *node)
   return sl;
 }
 
-/** Copy the primary (IPv4) OR port (IP address and TCP port) for
- * <b>node</b> into *<b>ap_out</b>.  */
-void
-node_get_prim_orport(const node_t *node, tor_addr_port_t *ap_out)
-{
-  if (node->ri) {
-    router_get_prim_orport(node->ri, ap_out);
-  } else if (node->rs) {
-    tor_addr_from_ipv4h(&ap_out->addr, node->rs->addr);
-    ap_out->port = node->rs->or_port;
-  }
-}
-
 /** Wrapper around node_get_prim_orport for backward
     compatibility.  */
 void
@@ -747,36 +734,6 @@ node_get_prim_addr_ipv4h(const node_t *node)
   return 0;
 }
 
-/** Copy the preferred OR port (IP address and TCP port) for
- * <b>node</b> into <b>ap_out</b>.  */
-void
-node_get_pref_orport(const node_t *node, tor_addr_port_t *ap_out)
-{
-  if (node->ri) {
-    router_get_pref_orport(node->ri, ap_out);
-  } else if (node->rs) {
-    /* No IPv6 in routerstatus_t yet.  XXXprop186 ok for private
-       bridges but needs fixing */
-    tor_addr_from_ipv4h(&ap_out->addr, node->rs->addr);
-    ap_out->port = node->rs->or_port;
-  }
-}
-
-/** Copy the preferred IPv6 OR port (address and TCP port) for
- * <b>node</b> into *<b>ap_out</b>. */
-void
-node_get_pref_ipv6_orport(const node_t *node, tor_addr_port_t *ap_out)
-{
-  if (node->ri) {
-    router_get_pref_ipv6_orport(node->ri, ap_out);
-  } else if (node->rs) {
-    /* No IPv6 in routerstatus_t yet.  XXXprop186 ok for private
-       bridges but needs fixing */
-    tor_addr_make_unspec(&ap_out->addr);
-    ap_out->port = 0;
-  }
-}
-
 /** Copy a string representation of an IP address for <b>node</b> into
  * the <b>len</b>-byte buffer at <b>buf</b>.  */
 void
@@ -847,3 +804,69 @@ node_get_declared_family(const node_t *node)
     return NULL;
 }
 
+/** Return 1 if we prefer the IPv6 address and OR TCP port of
+ * <b>node</b>, else 0.
+ *
+ *  We prefer the IPv6 address if the router has an IPv6 address and
+ *  i) the node_t says that we do prefer IPv6
+ *  or
+ *  ii) the router has no IPv4 address.  */
+int
+node_ipv6_preferred(const node_t *node)
+{
+  node_assert_ok(node);
+  if (node->ri)
+    return (!tor_addr_is_null(&node->ri->ipv6_addr)
+            && (node->ipv6_preferred || node->ri->addr == 0));
+  if (node->rs)
+    return (!tor_addr_is_null(&node->rs->ipv6_addr)
+            && (node->ipv6_preferred || node->rs->addr == 0));
+  return 0;
+}
+
+/** Copy the primary (IPv4) OR port (IP address and TCP port) for
+ * <b>node</b> into *<b>ap_out</b>. */
+void
+node_get_prim_orport(const node_t *node, tor_addr_port_t *ap_out)
+{
+  node_assert_ok(node);
+  tor_assert(ap_out);
+
+  if (node->ri) {
+    tor_addr_from_ipv4h(&ap_out->addr, node->ri->addr);
+    ap_out->port = node->ri->or_port;
+  } else if (node->rs) {
+    tor_addr_from_ipv4h(&ap_out->addr, node->rs->addr);
+    ap_out->port = node->rs->or_port;
+  }
+}
+
+/** Copy the preferred OR port (IP address and TCP port) for
+ * <b>node</b> into *<b>ap_out</b>.  */
+void
+node_get_pref_orport(const node_t *node, tor_addr_port_t *ap_out)
+{
+  tor_assert(ap_out);
+
+  if (node_ipv6_preferred(node))
+    node_get_pref_ipv6_orport(node, ap_out);
+  else
+    node_get_prim_orport(node, ap_out);
+}
+
+/** Copy the preferred IPv6 OR port (IP address and TCP port) for
+ * <b>node</b> into *<b>ap_out</b>. */
+void
+node_get_pref_ipv6_orport(const node_t *node, tor_addr_port_t *ap_out)
+{
+  node_assert_ok(node);
+  tor_assert(ap_out);
+
+  if (node->ri) {
+    tor_addr_copy(&ap_out->addr, &node->ri->ipv6_addr);
+    ap_out->port = node->ri->ipv6_orport;
+  } else if (node->rs) {
+    tor_addr_copy(&ap_out->addr, &node->rs->ipv6_addr);
+    ap_out->port = node->rs->ipv6_orport;
+  }
+}

src/or/nodelist.h

@@ -5,13 +5,23 @@
 /* See LICENSE for licensing information */
 
 /**
- * \file microdesc.h
- * \brief Header file for microdesc.c.
+ * \file nodelist.h
+ * \brief Header file for nodelist.c.
  **/
 
 #ifndef _TOR_NODELIST_H
 #define _TOR_NODELIST_H
 
+/* XXX duplicating code from tor_assert(). */
+#define node_assert_ok(n) STMT_BEGIN                            \
+  if (PREDICT_UNLIKELY((n)->ri == NULL && (n)->rs == NULL)) {   \
+    log_err(LD_BUG, "%s:%d: %s: Node is invalid; aborting.",    \
+            _SHORT_FILE_, __LINE__, __func__);                  \
+    fprintf(stderr, "%s:%d: %s: Node is invalid; aborting.\n",  \
+            _SHORT_FILE_, __LINE__, __func__);                  \
+    abort();                                                    \
+  } STMT_END
+
 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);
@@ -38,10 +48,9 @@ int node_get_purpose(const node_t *node);
 int node_is_me(const node_t *node);
 int node_exit_policy_rejects_all(const node_t *node);
 smartlist_t *node_get_all_orports(const node_t *node);
-void node_get_prim_orport(const node_t *node, tor_addr_port_t *addr_port_out);
-void node_get_pref_orport(const node_t *node, tor_addr_port_t *addr_port_out);
-void node_get_pref_ipv6_orport(const node_t *node,
-                               tor_addr_port_t *addr_port_out);
+void node_get_prim_orport(const node_t *node, tor_addr_port_t *ap_out);
+void node_get_pref_orport(const node_t *node, tor_addr_port_t *ap_out);
+void node_get_pref_ipv6_orport(const node_t *node, tor_addr_port_t *ap_out);
 uint32_t node_get_prim_addr_ipv4h(const node_t *node);
 int node_allows_single_hop_exits(const node_t *node);
 const char *node_get_nickname(const node_t *node);
@@ -50,6 +59,7 @@ void node_get_address_string(const node_t *node, char *cp, size_t len);
 long node_get_declared_uptime(const node_t *node);
 time_t node_get_published_on(const node_t *node);
 const smartlist_t *node_get_declared_family(const node_t *node);
+int node_ipv6_preferred(const node_t *node);
 
 smartlist_t *nodelist_get_list(void);
 

src/or/or.h

@@ -1770,8 +1770,6 @@ typedef struct {
   /** True if, after we have added this router, we should re-launch
    * tests for it. */
   unsigned int needs_retest_if_added:1;
-  /** True if ipv6_addr:ipv6_orport is preferred.  */
-  unsigned int ipv6_preferred:1;
 
 /** Tor can use this router for general positions in circuits; we got it
  * from a directory server as usual, or we're an authority and a server
@@ -2029,6 +2027,9 @@ typedef struct node_t {
 
   /* Local info: derived. */
 
+  /** True if the IPv6 OR port is preferred over the IPv4 OR port.  */
+  unsigned int ipv6_preferred:1;
+
   /** According to the geoip db what country is this router in? */
   /* XXXprop186 what is this suppose to mean with multiple OR ports? */
   country_t country;

src/or/router.c

@@ -880,6 +880,21 @@ decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port)
   return advertising ? dir_port : 0;
 }
 
+/** Allocate and return a new extend_info_t that can be used to build
+ * a circuit to or through the router <b>r</b>. Use the primary
+ * address of the router unless <b>for_direct_connect</b> is true, in
+ * which case the preferred address is used instead. */
+static extend_info_t *
+extend_info_from_router(const routerinfo_t *r)
+{
+  tor_addr_port_t ap;
+  tor_assert(r);
+
+  router_get_prim_orport(r, &ap);
+  return extend_info_alloc(r->nickname, r->cache_info.identity_digest,
+                           r->onion_pkey, &ap.addr, ap.port);
+}
+
 /** Some time has passed, or we just got new directory information.
  * See if we currently believe our ORPort or DirPort to be
  * unreachable. If so, launch a new test for it.
@@ -921,12 +936,11 @@ consider_testing_reachability(int test_or, int test_dir)
   }
 
   if (test_or && (!orport_reachable || !circuit_enough_testing_circs())) {
-    extend_info_t *ei;
+    extend_info_t *ei = extend_info_from_router(me);
+    /* XXX IPv6 self testing */
     log_info(LD_CIRC, "Testing %s of my ORPort: %s:%d.",
              !orport_reachable ? "reachability" : "bandwidth",
              me->address, me->or_port);
-    /* XXX IPv6 self testing IPv6 orports will need pref_addr */
-    ei = extend_info_from_router(me, 0);
     circuit_launch_by_extend_info(CIRCUIT_PURPOSE_TESTING, ei,
                             CIRCLAUNCH_NEED_CAPACITY|CIRCLAUNCH_IS_INTERNAL);
     extend_info_free(ei);
@@ -2195,42 +2209,6 @@ router_get_prim_orport(const routerinfo_t *router, tor_addr_port_t *ap_out)
   ap_out->port = router->or_port;
 }
 
-/** Return 1 if we prefer the IPv6 address and OR TCP port of
- * <b>router</b>, else 0.
- *
- *  We prefer the IPv6 address if the router has one and
- *  i) the routerinfo_t says so
- *  or
- *  ii) the router has no IPv4 address.  */
-int
-router_ipv6_preferred(const routerinfo_t *router)
-{
-  return (!tor_addr_is_null(&router->ipv6_addr)
-          && (router->ipv6_preferred || router->addr == 0));
-}
-
-/** Copy the preferred OR port (IP address and TCP port) for
- * <b>router</b> into *<b>addr_out</b>.  */
-void
-router_get_pref_orport(const routerinfo_t *router, tor_addr_port_t *ap_out)
-{
-  if (router_ipv6_preferred(router))
-    router_get_pref_ipv6_orport(router, ap_out);
-  else
-    router_get_prim_orport(router, ap_out);
-}
-
-/** Copy the preferred IPv6 OR port (IP address and TCP port) for
- * <b>router</b> into *<b>ap_out</b>. */
-void
-router_get_pref_ipv6_orport(const routerinfo_t *router,
-                            tor_addr_port_t *ap_out)
-{
-  tor_assert(ap_out != NULL);
-  tor_addr_copy(&ap_out->addr, &router->ipv6_addr);
-  ap_out->port = router->ipv6_orport;
-}
-
 /** Return 1 if any of <b>router</b>'s addresses are <b>addr</b>.
  *   Otherwise return 0. */
 int
@@ -2782,3 +2760,29 @@ router_free_all(void)
   }
 }
 
+/** Return a smartlist of tor_addr_port_t's with all the OR ports of
+    <b>ri</b>. Note that freeing of the items in the list as well as
+    the smartlist itself is the callers responsibility.
+
+    XXX duplicating code from node_get_all_orports(). */
+smartlist_t *
+router_get_all_orports(const routerinfo_t *ri)
+{
+  smartlist_t *sl = smartlist_new();
+  tor_assert(ri);
+
+  if (ri->addr != 0) {
+    tor_addr_port_t *ap = tor_malloc(sizeof(tor_addr_port_t));
+    tor_addr_from_ipv4h(&ap->addr, ri->addr);
+    ap->port = ri->or_port;
+    smartlist_add(sl, ap);
+  }
+  if (!tor_addr_is_null(&ri->ipv6_addr)) {
+    tor_addr_port_t *ap = tor_malloc(sizeof(tor_addr_port_t));
+    tor_addr_copy(&ap->addr, &ri->ipv6_addr);
+    ap->port = ri->or_port;
+    smartlist_add(sl, ap);
+  }
+
+  return sl;
+}

src/or/router.h

@@ -86,17 +86,13 @@ int router_pick_published_address(const or_options_t *options, uint32_t *addr);
 int router_rebuild_descriptor(int force);
 int router_dump_router_to_string(char *s, size_t maxlen, routerinfo_t *router,
                                  crypto_pk_t *ident_key);
-void router_get_prim_orport(const routerinfo_t *router,
-                            tor_addr_port_t *addr_port_out);
-void router_get_pref_orport(const routerinfo_t *router,
-                            tor_addr_port_t *addr_port_out);
-void router_get_pref_ipv6_orport(const routerinfo_t *router,
-                                 tor_addr_port_t *addr_port_out);
-int router_ipv6_preferred(const routerinfo_t *router);
-int router_has_addr(const routerinfo_t *router, const tor_addr_t *addr);
-int router_has_orport(const routerinfo_t *router, const tor_addr_port_t *orport);
 int extrainfo_dump_to_string(char **s, extrainfo_t *extrainfo,
                              crypto_pk_t *ident_key);
+void router_get_prim_orport(const routerinfo_t *router,
+                            tor_addr_port_t *ap_out);
+int router_has_addr(const routerinfo_t *router, const tor_addr_t *addr);
+int router_has_orport(const routerinfo_t *router,
+                      const tor_addr_port_t *orport);
 int is_legal_nickname(const char *s);
 int is_legal_nickname_or_hexdigest(const char *s);
 int is_legal_hexdigest(const char *s);
@@ -134,6 +130,8 @@ void router_free_all(void);
 const char *router_purpose_to_string(uint8_t p);
 uint8_t router_purpose_from_string(const char *s);
 
+smartlist_t *router_get_all_orports(const routerinfo_t *ri);
+
 #ifdef ROUTER_PRIVATE
 /* Used only by router.c and test.c */
 void get_platform_str(char *platform, size_t len);