r17436@tombo: nickm | 2008-07-30 09:03:19 -0400

 Move n_addr, n_port, and n_conn_id_digest fields of circuit_t into a separately allocated extend_info_t.  Saves 22 bytes per connected circuit_t on 32-bit platforms, and makes me more comfortable with using tor_addr_t in place of uint32_t n_addr.


svn:r16257

ChangeLog

@@ -33,6 +33,9 @@ Changes in version 0.2.1.3-alpha - 2008-07-xx
     - Change the implementation of ExcludeNodes and ExcludeExitNodes
       to be more efficient.  Formerly it was quadratic in the number
       of servers; now it should be linear.  Fixes bug 509.
+    - Save 16-22 bytes per open circuit by moving the n_hop, n_port,
+      and n_conn_id_digest fields into a separate structure that's
+      only needed when the circuit has not yet attached to an n_conn.
 
   o Minor bugfixes:
     - Change the contrib/tor.logrotate script so it makes the new

src/or/circuitbuild.c

@@ -356,9 +356,7 @@ circuit_handle_first_hop(origin_circuit_t *circ)
   tor_inet_ntoa(&in, tmpbuf, sizeof(tmpbuf));
   log_debug(LD_CIRC,"Looking for firsthop '%s:%u'",tmpbuf,
             firsthop->extend_info->port);
-  /* imprint the circuit with its future n_conn->id */
-  memcpy(circ->_base.n_conn_id_digest, firsthop->extend_info->identity_digest,
-         DIGEST_LEN);
+
   n_conn = connection_or_get_by_identity_digest(
          firsthop->extend_info->identity_digest);
   /* If we don't have an open conn, or the conn we have is obsolete
@@ -367,8 +365,7 @@ circuit_handle_first_hop(origin_circuit_t *circ)
   if (!n_conn || n_conn->_base.state != OR_CONN_STATE_OPEN ||
       (n_conn->_base.or_is_obsolete)) {
     /* not currently connected */
-    circ->_base.n_addr = firsthop->extend_info->addr;
-    circ->_base.n_port = firsthop->extend_info->port;
+    circ->_base.n_hop = extend_info_dup(firsthop->extend_info);
 
     if (!n_conn || n_conn->_base.or_is_obsolete) { /* launch the connection */
       if (circ->build_state->onehop_tunnel)
@@ -389,8 +386,7 @@ circuit_handle_first_hop(origin_circuit_t *circ)
      */
     return 0;
   } else { /* it's already open. use it. */
-    circ->_base.n_addr = n_conn->_base.addr;
-    circ->_base.n_port = n_conn->_base.port;
+    tor_assert(!circ->_base.n_hop);
     circ->_base.n_conn = n_conn;
     log_debug(LD_CIRC,"Conn open. Delivering first onion skin.");
     if ((err_reason = circuit_send_next_onion_skin(circ)) < 0) {
@@ -419,25 +415,24 @@ circuit_n_conn_done(or_connection_t *or_conn, int status)
   pending_circs = smartlist_create();
   circuit_get_all_pending_on_or_conn(pending_circs, or_conn);
 
-  SMARTLIST_FOREACH(pending_circs, circuit_t *, circ,
+  SMARTLIST_FOREACH_BEGIN (pending_circs, circuit_t *, circ)
     {
       /* These checks are redundant wrt get_all_pending_on_or_conn, but I'm
        * leaving them in in case it's possible for the status of a circuit to
        * change as we're going down the list. */
-      if (circ->marked_for_close || circ->n_conn ||
+      if (circ->marked_for_close || circ->n_conn || !circ->n_hop ||
           circ->state != CIRCUIT_STATE_OR_WAIT)
         continue;
-      if (tor_digest_is_zero(circ->n_conn_id_digest)) {
+
+      if (tor_digest_is_zero(circ->n_hop->identity_digest)) {
         /* Look at addr/port. This is an unkeyed connection. */
-        if (circ->n_addr != or_conn->_base.addr ||
-            circ->n_port != or_conn->_base.port)
+        if (circ->n_hop->addr != or_conn->_base.addr ||
+            circ->n_hop->port != or_conn->_base.port)
           continue;
-        /* now teach circ the right identity_digest */
-        memcpy(circ->n_conn_id_digest, or_conn->identity_digest, DIGEST_LEN);
       } else {
         /* We expected a key. See if it's the right one. */
         if (memcmp(or_conn->identity_digest,
-                   circ->n_conn_id_digest, DIGEST_LEN))
+                   circ->n_hop->identity_digest, DIGEST_LEN))
           continue;
       }
       if (!status) { /* or_conn failed; close circ */
@@ -450,6 +445,9 @@ circuit_n_conn_done(or_connection_t *or_conn, int status)
        * orconn_circuid_circuit_map, so we don't need to call
        * set_circid_orconn here. */
       circ->n_conn = or_conn;
+      extend_info_free(circ->n_hop);
+      circ->n_hop = NULL;
+
       if (CIRCUIT_IS_ORIGIN(circ)) {
         if ((err_reason =
              circuit_send_next_onion_skin(TO_ORIGIN_CIRCUIT(circ))) < 0) {
@@ -471,7 +469,8 @@ circuit_n_conn_done(or_connection_t *or_conn, int status)
         tor_free(circ->n_conn_onionskin);
         circuit_set_state(circ, CIRCUIT_STATE_OPEN);
       }
-    });
+    }
+  SMARTLIST_FOREACH_END(circ);
 
   smartlist_free(pending_circs);
 }
@@ -723,6 +722,8 @@ circuit_extend(cell_t *cell, circuit_t *circ)
   relay_header_t rh;
   char *onionskin;
   char *id_digest=NULL;
+  uint32_t n_addr;
+  uint16_t n_port;
 
   if (circ->n_conn) {
     log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
@@ -745,11 +746,11 @@ circuit_extend(cell_t *cell, circuit_t *circ)
     return -1;
   }
 
-  circ->n_addr = ntohl(get_uint32(cell->payload+RELAY_HEADER_SIZE));
-  circ->n_port = ntohs(get_uint16(cell->payload+RELAY_HEADER_SIZE+4));
-
+  n_addr = ntohl(get_uint32(cell->payload+RELAY_HEADER_SIZE));
+  n_port = ntohs(get_uint16(cell->payload+RELAY_HEADER_SIZE+4));
   onionskin = cell->payload+RELAY_HEADER_SIZE+4+2;
   id_digest = cell->payload+RELAY_HEADER_SIZE+4+2+ONIONSKIN_CHALLENGE_LEN;
+
   n_conn = connection_or_get_by_identity_digest(id_digest);
 
   /* If we don't have an open conn, or the conn we have is obsolete
@@ -759,24 +760,23 @@ circuit_extend(cell_t *cell, circuit_t *circ)
       n_conn->_base.or_is_obsolete) {
     struct in_addr in;
     char tmpbuf[INET_NTOA_BUF_LEN];
-    in.s_addr = htonl(circ->n_addr);
+    in.s_addr = htonl(n_addr);
     tor_inet_ntoa(&in,tmpbuf,sizeof(tmpbuf));
     log_debug(LD_CIRC|LD_OR,"Next router (%s:%d) not connected. Connecting.",
-              tmpbuf, circ->n_port);
+              tmpbuf, (int)n_port);
+
+    circ->n_hop = extend_info_alloc(NULL /*nickname*/,
+                                    id_digest,
+                                    NULL /*onion_key*/,
+                                    n_addr, n_port);
 
     circ->n_conn_onionskin = tor_malloc(ONIONSKIN_CHALLENGE_LEN);
     memcpy(circ->n_conn_onionskin, onionskin, ONIONSKIN_CHALLENGE_LEN);
     circuit_set_state(circ, CIRCUIT_STATE_OR_WAIT);
 
-    /* imprint the circuit with its future n_conn->id */
-    memcpy(circ->n_conn_id_digest, id_digest, DIGEST_LEN);
-
-    if (n_conn && !n_conn->_base.or_is_obsolete) {
-      circ->n_addr = n_conn->_base.addr;
-      circ->n_port = n_conn->_base.port;
-    } else {
-     /* we should try to open a connection */
-      n_conn = connection_or_connect(circ->n_addr, circ->n_port, id_digest);
+    if (! (n_conn && !n_conn->_base.or_is_obsolete)) {
+      /* we should try to open a connection */
+      n_conn = connection_or_connect(n_addr, n_port, id_digest);
       if (!n_conn) {
         log_info(LD_CIRC,"Launching n_conn failed. Closing circuit.");
         circuit_mark_for_close(circ, END_CIRC_REASON_CONNECTFAILED);
@@ -791,12 +791,8 @@ circuit_extend(cell_t *cell, circuit_t *circ)
     return 0;
   }
 
-  /* these may be different if the router connected to us from elsewhere */
-  circ->n_addr = n_conn->_base.addr;
-  circ->n_port = n_conn->_base.port;
-
+  tor_assert(circ->n_hop);
   circ->n_conn = n_conn;
-  memcpy(circ->n_conn_id_digest, n_conn->identity_digest, DIGEST_LEN);
   log_debug(LD_CIRC,"n_conn is %s:%u",
             n_conn->_base.address,n_conn->_base.port);
 

src/or/circuitlist.c

@@ -235,16 +235,18 @@ circuit_get_all_pending_on_or_conn(smartlist_t *out, or_connection_t *or_conn)
   {
     if (circ->marked_for_close)
       continue;
+    if (!circ->n_hop)
+      continue;
     tor_assert(circ->state == CIRCUIT_STATE_OR_WAIT);
-    if (tor_digest_is_zero(circ->n_conn_id_digest)) {
+    if (tor_digest_is_zero(circ->n_hop->identity_digest)) {
       /* Look at addr/port. This is an unkeyed connection. */
-      if (circ->n_addr != or_conn->_base.addr ||
-          circ->n_port != or_conn->_base.port)
+      if (circ->n_hop->addr != or_conn->_base.addr ||
+          circ->n_hop->port != or_conn->_base.port)
         continue;
     } else {
       /* We expected a key. See if it's the right one. */
       if (memcmp(or_conn->identity_digest,
-                 circ->n_conn_id_digest, DIGEST_LEN))
+                 circ->n_hop->identity_digest, DIGEST_LEN))
         continue;
     }
     smartlist_add(out, circ);
@@ -430,6 +432,8 @@ circuit_free(circuit_t *circ)
     cell_queue_clear(&ocirc->p_conn_cells);
   }
 
+  if (circ->n_hop)
+    extend_info_free(circ->n_hop);
   tor_free(circ->n_conn_onionskin);
 
   /* Remove from map. */
@@ -568,11 +572,11 @@ circuit_dump_by_conn(connection_t *conn, int severity)
         }
       }
     }
-    if (!circ->n_conn && circ->n_addr && circ->n_port &&
-        circ->n_addr == conn->addr &&
-        circ->n_port == conn->port &&
+    if (!circ->n_conn && circ->n_hop &&
+        circ->n_hop->addr == conn->addr &&
+        circ->n_hop->port == conn->port &&
         conn->type == CONN_TYPE_OR &&
-        !memcmp(TO_OR_CONN(conn)->identity_digest, circ->n_conn_id_digest,
+        !memcmp(TO_OR_CONN(conn)->identity_digest, circ->n_hop->identity_digest,
                 DIGEST_LEN)) {
       circuit_dump_details(severity, circ, conn->conn_array_index,
                            (circ->state == CIRCUIT_STATE_OPEN &&
@@ -1152,8 +1156,8 @@ assert_circuit_ok(const circuit_t *c)
   }
 
   if (c->n_conn) {
-    tor_assert(!memcmp(c->n_conn->identity_digest, c->n_conn_id_digest,
-                       DIGEST_LEN));
+    tor_assert(!c->n_hop);
+
     if (c->n_circ_id)
       tor_assert(c == circuit_get_by_circid_orconn(c->n_circ_id, c->n_conn));
   }

src/or/circuituse.c

@@ -289,7 +289,7 @@ circuit_expire_building(time_t now)
 
     if (victim->n_conn)
       log_info(LD_CIRC,"Abandoning circ %s:%d:%d (state %d:%s, purpose %d)",
-               victim->n_conn->_base.address, victim->n_port,
+               victim->n_conn->_base.address, victim->n_conn->_base.port,
                victim->n_circ_id,
                victim->state, circuit_state_to_string(victim->state),
                victim->purpose);
@@ -739,12 +739,15 @@ circuit_build_failed(origin_circuit_t *circ)
     /* We failed at the first hop. If there's an OR connection
        to blame, blame it. */
     or_connection_t *n_conn = NULL;
+    const char *n_conn_id = NULL;
     if (circ->_base.n_conn) {
       n_conn = circ->_base.n_conn;
-    } else if (circ->_base.state == CIRCUIT_STATE_OR_WAIT) {
+      if (n_conn) n_conn_id = n_conn->identity_digest;
+    } else if (circ->_base.state == CIRCUIT_STATE_OR_WAIT &&
+               circ->_base.n_hop) {
       /* we have to hunt for it */
-      n_conn = connection_or_get_by_identity_digest(
-                                               circ->_base.n_conn_id_digest);
+      n_conn_id = circ->_base.n_hop->identity_digest;
+      n_conn = connection_or_get_by_identity_digest(n_conn_id);
     }
     if (n_conn) {
       log_info(LD_OR,
@@ -760,7 +763,8 @@ circuit_build_failed(origin_circuit_t *circ)
     }
     /* if there are any one-hop streams waiting on this circuit, fail
      * them now so they can retry elsewhere. */
-    connection_ap_fail_onehop(circ->_base.n_conn_id_digest, circ->build_state);
+    if (n_conn_id)
+      connection_ap_fail_onehop(n_conn_id, circ->build_state);
   }
 
   switch (circ->_base.purpose) {

src/or/or.h

@@ -1780,14 +1780,12 @@ typedef struct circuit_t {
   cell_queue_t n_conn_cells;
   /** The OR connection that is next in this circuit. */
   or_connection_t *n_conn;
-  /** The identity hash of n_conn. */
-  char n_conn_id_digest[DIGEST_LEN];
   /** The circuit_id used in the next (forward) hop of this circuit. */
   circid_t n_circ_id;
-  /** The port for the OR that is next in this circuit. */
-  uint16_t n_port;
-  /** The IPv4 address of the OR that is next in this circuit. */
-  uint32_t n_addr;
+
+  /** The hop to which we want to extend this ciruit.  Should be NULL if
+   * the */
+  extend_info_t *n_hop;
 
   /** True iff we are waiting for n_conn_cells to become less full before
    * allowing p_streams to add any more cells. (Origin circuit only.) */