r12687@Kushana: nickm | 2007-04-09 17:05:57 -0400

 Try to fix bug 410: move responsibility for attaching/detaching initial streams from circuits into dns_resolve.  Needs refactoring a little.


svn:r9931

ChangeLog

@@ -66,7 +66,8 @@ Changes in version 0.2.0.1-alpha - 2007-??-??
     - Drop the old code to choke directory connections when the corresponding
       or connections got full: thanks to the cell queue feature, or conns
       don't get full any more.
-
+    - Make dns_resolve handle attaching connections to circuits properly,
+      so the caller doesn't have to.
 
 Changes in version 0.1.2.12-rc - 2007-03-16
   o Major bugfixes:

doc/TODO

@@ -70,8 +70,11 @@ Things we'd like to do in 0.2.0.x:
       o When making a circuit active on a connection with an empty buf,
         we need to "prime" the buffer, so that we can trigger the "I flushed
         some" test.
-      - Change how directory-bridge-choking works: choke when circuit queue
+      X Change how directory-bridge-choking works: choke when circuit queue
         is full, not when the orconn is "too full".
+        [No need to do this: the edge-connection choking will already take
+        care of this a bit, and rewriting the 'bridged connection' code
+        to not use socketpairs will give us even more control.]
       - Do we switch to arena-allocation for cells?
       - Can we stop doing so many memcpys on cells?
       o Also, only package data from exitconns when there is space on the

src/or/connection_edge.c

@@ -2245,12 +2245,7 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
   /* send it off to the gethostbyname farm */
   switch (dns_resolve(n_stream)) {
     case 1: /* resolve worked */
-
-      /* add it into the linked list of n_streams on this circuit */
-      n_stream->next_stream = TO_OR_CIRCUIT(circ)->n_streams;
-      TO_OR_CIRCUIT(circ)->n_streams = n_stream;
       assert_circuit_ok(circ);
-
       log_debug(LD_EXIT,"about to call connection_exit_connect().");
       connection_exit_connect(n_stream);
       return 0;
@@ -2262,8 +2257,6 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
       break;
     case 0: /* resolve added to pending list */
       /* add it into the linked list of resolving_streams on this circuit */
-      n_stream->next_stream = TO_OR_CIRCUIT(circ)->resolving_streams;
-      TO_OR_CIRCUIT(circ)->resolving_streams = n_stream;
       assert_circuit_ok(circ);
       ;
   }
@@ -2310,8 +2303,6 @@ connection_exit_begin_resolve(cell_t *cell, or_circuit_t *circ)
         connection_free(TO_CONN(dummy_conn));
       return 0;
     case 0: /* resolve added to pending list */
-      dummy_conn->next_stream = circ->resolving_streams;
-      circ->resolving_streams = dummy_conn;
       assert_circuit_ok(TO_CIRCUIT(circ));
       break;
   }

src/or/dns.c

@@ -509,14 +509,22 @@ parse_inaddr_arpa_address(const char *address, struct in_addr *in)
  * do that for us.)
  *
  * If we have a cached answer, send the answer back along <b>exitconn</b>'s
- * attached circuit.
+ * circuit.
  *
  * Else, if seen before and pending, add conn to the pending list,
  * and return 0.
  *
  * Else, if not seen before, add conn to pending list, hand to
  * dns farm, and return 0.
+ *
+ * Exitconn's on_circuit field must be set, but exitconn should not
+ * yet be linked onto the n_streams/resolving_streams list of that circuit.
+ * On success, link the connection to n_streams if it's an exit connection.
+ * On "pending", link the connection to resolving streams.  Otherwise,
+ * clear its on_circuit field.
  */
+/* XXXX020 Split this into a helper that checks whether there is an answer,
+ * and a caller that takes appropriate action based on what happened. */
 int
 dns_resolve(edge_connection_t *exitconn)
 {
@@ -530,6 +538,7 @@ dns_resolve(edge_connection_t *exitconn)
   assert_connection_ok(TO_CONN(exitconn), 0);
   tor_assert(exitconn->_base.s == -1);
   assert_cache_ok();
+  tor_assert(oncirc);
 
   is_resolve = exitconn->_base.purpose == EXIT_PURPOSE_RESOLVE;
 
@@ -538,8 +547,12 @@ dns_resolve(edge_connection_t *exitconn)
   if (tor_inet_aton(exitconn->_base.address, &in) != 0) {
     exitconn->_base.addr = ntohl(in.s_addr);
     exitconn->address_ttl = DEFAULT_DNS_TTL;
-    if (is_resolve)
+    if (is_resolve) {
       send_resolved_cell(exitconn, RESOLVED_TYPE_IPV4);
+    } else {
+      exitconn->next_stream = oncirc->n_streams;
+      oncirc->n_streams = exitconn;
+    }
     return 1;
   }
   if (address_is_invalid_destination(exitconn->_base.address, 0)) {
@@ -548,7 +561,8 @@ dns_resolve(edge_connection_t *exitconn)
         escaped_safe_str(exitconn->_base.address));
     if (is_resolve)
       send_resolved_cell(exitconn, RESOLVED_TYPE_ERROR);
-    circuit_detach_stream(TO_CIRCUIT(oncirc), exitconn);
+    //circuit_detach_stream(TO_CIRCUIT(oncirc), exitconn);
+    exitconn->on_circuit = NULL;
     if (!exitconn->_base.marked_for_close)
       connection_free(TO_CONN(exitconn));
     return -1;
@@ -581,7 +595,8 @@ dns_resolve(edge_connection_t *exitconn)
 
       if (exitconn->_base.purpose == EXIT_PURPOSE_RESOLVE)
         send_resolved_cell(exitconn, RESOLVED_TYPE_ERROR);
-      circuit_detach_stream(TO_CIRCUIT(oncirc), exitconn);
+      //circuit_detach_stream(TO_CIRCUIT(oncirc), exitconn);
+      exitconn->on_circuit = NULL;
       if (!exitconn->_base.marked_for_close)
         connection_free(TO_CONN(exitconn));
       return -1;
@@ -606,6 +621,9 @@ dns_resolve(edge_connection_t *exitconn)
                   "resolve of %s", exitconn->_base.s,
                   escaped_safe_str(exitconn->_base.address));
         exitconn->_base.state = EXIT_CONN_STATE_RESOLVING;
+
+        exitconn->next_stream = oncirc->resolving_streams;
+        oncirc->resolving_streams = exitconn;
         return 0;
       case CACHE_STATE_CACHED_VALID:
         log_debug(LD_EXIT,"Connection (fd %d) found cached answer for %s",
@@ -621,6 +639,12 @@ dns_resolve(edge_connection_t *exitconn)
           if (is_resolve)
             send_resolved_cell(exitconn, RESOLVED_TYPE_IPV4);
         }
+        if (!is_resolve) {
+          /* It's a connect; add it into the linked list of n_streams on this
+             circuit */
+          exitconn->next_stream = oncirc->n_streams;
+          oncirc->n_streams = exitconn;
+        }
         return 1;
       case CACHE_STATE_CACHED_FAILED:
         log_debug(LD_EXIT,"Connection (fd %d) found cached error for %s",
@@ -628,7 +652,8 @@ dns_resolve(edge_connection_t *exitconn)
                   escaped_safe_str(exitconn->_base.address));
         if (is_resolve)
           send_resolved_cell(exitconn, RESOLVED_TYPE_ERROR);
-        circuit_detach_stream(TO_CIRCUIT(oncirc), exitconn);
+        // circuit_detach_stream(TO_CIRCUIT(oncirc), exitconn);
+        exitconn->on_circuit = NULL;
         if (!exitconn->_base.marked_for_close)
           connection_free(TO_CONN(exitconn));
         return -1;
@@ -658,7 +683,18 @@ dns_resolve(edge_connection_t *exitconn)
   log_debug(LD_EXIT,"Launching %s.",
             escaped_safe_str(exitconn->_base.address));
   assert_cache_ok();
-  return launch_resolve(exitconn);
+
+  r = launch_resolve(exitconn);
+  if (r == 0) {
+    exitconn->next_stream = oncirc->resolving_streams;
+    oncirc->resolving_streams = exitconn;
+  } else {
+    tor_assert(r<0);
+    exitconn->on_circuit = NULL;
+    if (!exitconn->_base.marked_for_close)
+      connection_free(TO_CONN(exitconn));
+  }
+  return r;
 }
 
 /** Log an error and abort if conn is waiting for a DNS resolve.
@@ -1160,7 +1196,7 @@ evdns_callback(int result, char type, int count, int ttl, void *addresses,
 }
 
 /** For eventdns: start resolving as necessary to find the target for
- * <b>exitconn</b> */
+ * <b>exitconn</b>.  Returns -1 on error, 0 on "resolve launched." */
 static int
 launch_resolve(edge_connection_t *exitconn)
 {