Refactor mark_for_close, connection_edge_end and friends. Now, everybody

who wants to shut down a connection calls connection_mark_for_close instead
of setting marked_for_close to 1.  This automatically removes the connection
from the DNS cache if needed, sends a RELAY END cell if appropriate, and can
be changed to do whatever else is needed.

Still to do:
  - The same for circuits, maybe.
  - Add some kind of hold_connection_open_until_flushed flag, maybe.
  - Change stuff that closes connections with return -1 to use mark_for_close,
    maybe.


svn:r1145

src/or/circuit.c

@@ -64,6 +64,8 @@ circuit_t *circuit_new(uint16_t p_circ_id, connection_t *p_conn) {
 
   circ->timestamp_created = time(NULL);
 
+  circ->marked_for_close = 0;
+
   circ->p_circ_id = p_circ_id;
   circ->p_conn = p_conn;
 
@@ -739,9 +741,8 @@ void circuit_about_to_close_connection(connection_t *conn) {
         return;
 
       if(!conn->has_sent_end) {
-        log_fn(LOG_INFO,"Edge connection hasn't sent end yet? Bug.");
-        if(connection_edge_end(conn, END_STREAM_REASON_MISC, conn->cpath_layer) < 0)
-          log_fn(LOG_WARN,"1: I called connection_edge_end redundantly.");
+        log_fn(LOG_WARN,"Edge connection hasn't sent end yet? Bug.");
+        connection_mark_for_close(conn, END_STREAM_REASON_MISC);
       }
 
       circuit_detach_stream(circ, conn);
@@ -1160,8 +1161,8 @@ int circuit_truncated(circuit_t *circ, crypt_path_t *layer) {
         /* no need to send 'end' relay cells,
          * because the other side's already dead
          */
-        stream->marked_for_close = 1;
         stream->has_sent_end = 1;
+        connection_mark_for_close(stream,0);
       }
     }
 

src/or/connection.c

@@ -81,7 +81,6 @@ connection_t *connection_new(int type) {
   conn->magic = CONNECTION_MAGIC;
   conn->s = -1; /* give it a default of 'not used' */
 
-
   conn->type = type;
   if(!connection_is_listener(conn)) { /* listeners never use their buf */
     conn->inbuf = buf_new();
@@ -142,6 +141,51 @@ void connection_free_all(void) {
     connection_free(carray[i]);
 }
 
+int
+_connection_mark_for_close(connection_t *conn, char reason)
+{
+  assert_connection_ok(conn,0);
+
+  if (conn->marked_for_close) {
+    log(LOG_WARN, "Double mark-for-close on connection.");
+    return -1;
+  }
+
+  switch (conn->type)
+    {
+    case CONN_TYPE_OR_LISTENER:
+    case CONN_TYPE_AP_LISTENER:
+    case CONN_TYPE_DIR_LISTENER:
+    case CONN_TYPE_CPUWORKER:
+    case CONN_TYPE_DIR:
+      /* No special processing needed. */
+      break;
+    case CONN_TYPE_OR:
+      /* No special processing needed, I think. */
+      break;
+    case CONN_TYPE_EXIT:
+    case CONN_TYPE_AP:
+      if (conn->state == EXIT_CONN_STATE_RESOLVING)
+        dns_cancel_pending_resolve(conn->address, conn);
+      if (reason < _MIN_END_STREAM_REASON || reason > _MAX_END_STREAM_REASON)
+        reason = END_STREAM_REASON_MISC;
+      if (!conn->has_sent_end &&
+          connection_edge_end(conn, reason, conn->cpath_layer) < 0)
+        return -1;
+      break;
+    case CONN_TYPE_DNSWORKER:
+      if (conn->state == DNSWORKER_STATE_BUSY) {
+        dns_cancel_pending_resolve(conn->address, NULL);
+      }
+      break;
+    default:
+      log(LOG_ERR, "Unknown connection type %d", conn->type);
+      ;
+    }
+  conn->marked_for_close = 1;
+  return 0;
+}
+
 int connection_create_listener(char *bindaddress, uint16_t bindport, int type) {
   struct sockaddr_in bindaddr; /* where to bind */
   struct hostent *rent;
@@ -239,7 +283,7 @@ static int connection_handle_listener_read(connection_t *conn, int new_type) {
   }
 
   if(connection_init_accepted_conn(newconn) < 0) {
-    newconn->marked_for_close = 1;
+    connection_mark_for_close(newconn,0);
     return 0;
   }
   return 0;
@@ -306,11 +350,14 @@ int connection_connect(connection_t *conn, char *address, uint32_t addr, uint16_
 }
 
 static void listener_close_if_present(int type) {
+  assert(type == CONN_TYPE_OR_LISTENER ||
+         type == CONN_TYPE_AP_LISTENER ||
+         type == CONN_TYPE_DIR_LISTENER);
   connection_t *conn = connection_get_by_type(type);
   if (conn) {
     close(conn->s);
     conn->s = -1;
-    conn->marked_for_close = 1;
+    connection_mark_for_close(conn,0);
   }
 }
 
@@ -523,7 +570,7 @@ void connection_write_to_buf(const char *string, int len, connection_t *conn) {
 
   if(write_to_buf(string, len, conn->outbuf) < 0) {
     log_fn(LOG_WARN,"write_to_buf failed. Closing connection (fd %d).", conn->s);
-    conn->marked_for_close = 1;
+    connection_mark_for_close(conn,0);
     return;
   }
 
@@ -539,8 +586,8 @@ void connection_write_to_buf(const char *string, int len, connection_t *conn) {
     len -= (MIN_TLS_FLUSHLEN - conn->outbuf_flushlen);
     conn->outbuf_flushlen = MIN_TLS_FLUSHLEN;
     if(connection_handle_write(conn) < 0) {
-      conn->marked_for_close = 1;
       log_fn(LOG_WARN,"flushing failed.");
+      connection_mark_for_close(conn,0);
     }
   }
   if(len > 0) { /* if there's any left over */
@@ -693,9 +740,7 @@ int connection_send_destroy(uint16_t circ_id, connection_t *conn) {
        log_fn(LOG_INFO,"...and informing resolver we don't want the answer anymore.");
        dns_cancel_pending_resolve(conn->address, conn);
      }
-     if(connection_edge_end(conn, END_STREAM_REASON_DESTROY, conn->cpath_layer) < 0)
-       log_fn(LOG_WARN,"1: I called connection_edge_end redundantly.");
-     /* if they already sent a destroy, they know. XXX can just close? */
+     connection_mark_for_close(conn, END_STREAM_REASON_DESTROY);
      return 0;
   }
 
@@ -865,4 +910,3 @@ void assert_connection_ok(connection_t *conn, time_t now)
   c-basic-offset:2
   End:
 */
-

src/or/connection_edge.c

@@ -48,8 +48,7 @@ int connection_edge_process_inbuf(connection_t *conn) {
     conn->done_receiving = 1;
     shutdown(conn->s, 0); /* XXX check return, refactor NM */
     if (conn->done_sending) {
-      if(connection_edge_end(conn, END_STREAM_REASON_DONE, conn->cpath_layer) < 0)
-        log_fn(LOG_WARN,"1: I called connection_edge_end redundantly.");
+      connection_mark_for_close(conn, END_STREAM_REASON_DONE);
     } else {
       connection_edge_send_command(conn, circuit_get_by_conn(conn), RELAY_COMMAND_END,
                                    NULL, 0, conn->cpath_layer);
@@ -58,8 +57,7 @@ int connection_edge_process_inbuf(connection_t *conn) {
 #else
     /* eof reached, kill it. */
     log_fn(LOG_INFO,"conn (fd %d) reached eof. Closing.", conn->s);
-    if(connection_edge_end(conn, END_STREAM_REASON_DONE, conn->cpath_layer) < 0)
-      log_fn(LOG_WARN,"2: I called connection_edge_end redundantly.");
+    connection_mark_for_close(conn, END_STREAM_REASON_DONE);
     return -1;
 #endif
   }
@@ -67,8 +65,7 @@ int connection_edge_process_inbuf(connection_t *conn) {
   switch(conn->state) {
     case AP_CONN_STATE_SOCKS_WAIT:
       if(connection_ap_handshake_process_socks(conn) < 0) {
-        if(connection_edge_end(conn, END_STREAM_REASON_MISC, conn->cpath_layer) < 0)
-          log_fn(LOG_WARN,"3: I called connection_edge_end redundantly.");
+        connection_mark_for_close(conn, END_STREAM_REASON_MISC);
         return -1;
       }
       return 0;
@@ -79,8 +76,7 @@ int connection_edge_process_inbuf(connection_t *conn) {
         return 0;
       }
       if(connection_edge_package_raw_inbuf(conn) < 0) {
-        if(connection_edge_end(conn, END_STREAM_REASON_MISC, conn->cpath_layer) < 0)
-          log_fn(LOG_WARN,"4: I called connection_edge_end redundantly.");
+        connection_mark_for_close(conn, END_STREAM_REASON_MISC);
         return -1;
       }
       return 0;
@@ -130,13 +126,14 @@ int connection_edge_end(connection_t *conn, char reason, crypt_path_t *cpath_lay
   }
 
   circ = circuit_get_by_conn(conn);
-  if(circ) {
+  if(circ && !circ->marked_for_close) {
     log_fn(LOG_DEBUG,"Marking conn (fd %d) and sending end.",conn->s);
     connection_edge_send_command(conn, circ, RELAY_COMMAND_END,
                                  payload, payload_len, cpath_layer);
+  } else {
+    log_fn(LOG_DEBUG,"Marking conn (fd %d); no circ to send end.",conn->s);
   }
 
-  conn->marked_for_close = 1;
   conn->has_sent_end = 1;
   return 0;
 }
@@ -150,7 +147,7 @@ int connection_edge_send_command(connection_t *fromconn, circuit_t *circ, int re
   if(!circ) {
     log_fn(LOG_WARN,"no circ. Closing.");
     assert(fromconn);
-    fromconn->marked_for_close = 1;
+    connection_mark_for_close(fromconn,0);
     return -1;
   }
 
@@ -207,12 +204,8 @@ int connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, connection
     if(rh.command == RELAY_COMMAND_END) {
       log_fn(LOG_INFO,"Exit got end (%s) before we're connected. Marking for close.",
         connection_edge_end_reason(cell->payload+RELAY_HEADER_SIZE, rh.length));
-      if(conn->state == EXIT_CONN_STATE_RESOLVING) {
-        log_fn(LOG_INFO,"...and informing resolver we don't want the answer anymore.");
-        dns_cancel_pending_resolve(conn->address, conn);
-      }
-      conn->marked_for_close = 1;
-      conn->has_sent_end = 1;
+      conn->has_sent_end = 1; /* So we don't send an end cell. */
+      connection_mark_for_close(conn, 0);
       return 0;
     }
     if(conn->type == CONN_TYPE_AP && rh.command == RELAY_COMMAND_CONNECTED) {
@@ -231,15 +224,13 @@ int connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, connection
       circuit_log_path(LOG_WARN,circ);
       if(connection_ap_handshake_socks_reply(conn, NULL, 0, 1) < 0) {
         log_fn(LOG_INFO,"Writing to socks-speaking application failed. Closing.");
-        if(connection_edge_end(conn, END_STREAM_REASON_MISC, conn->cpath_layer) < 0)
-          log_fn(LOG_WARN,"3: I called connection_edge_end redundantly.");
+        connection_mark_for_close(conn, END_STREAM_REASON_MISC);
       }
       return 0;
     } else {
       log_fn(LOG_WARN,"Got an unexpected relay command %d, in state %d (%s). Closing.",
              rh.command, conn->state, conn_state_to_string[conn->type][conn->state]);
-      if(connection_edge_end(conn, END_STREAM_REASON_MISC, conn->cpath_layer) < 0)
-        log_fn(LOG_WARN,"1: I called connection_edge_end redundantly.");
+      connection_mark_for_close(conn, END_STREAM_REASON_MISC);
       return -1;
     }
   }
@@ -264,8 +255,7 @@ int connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, connection
       if((edge_type == EDGE_AP && --layer_hint->deliver_window < 0) ||
          (edge_type == EDGE_EXIT && --circ->deliver_window < 0)) {
         log_fn(LOG_WARN,"(relay data) circ deliver_window below 0. Killing.");
-        if(connection_edge_end(conn, END_STREAM_REASON_MISC, conn->cpath_layer) < 0)
-          log_fn(LOG_WARN,"2: I called connection_edge_end redundantly.");
+        connection_mark_for_close(conn, END_STREAM_REASON_MISC);
         return -1;
       }
       log_fn(LOG_DEBUG,"circ deliver_window now %d.", edge_type == EDGE_AP ?
@@ -316,12 +306,12 @@ int connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, connection
       conn->done_sending = 1;
       shutdown(conn->s, 1); /* XXX check return; refactor NM */
       if (conn->done_receiving) {
-        conn->marked_for_close = 1;
         conn->has_sent_end = 1; /* no need to send end, we just got one! */
+        connection_mark_for_close(conn, END_STREAM_REASON_DONE);
       }
 #else
-      conn->marked_for_close = 1;
       conn->has_sent_end = 1; /* no need to send end, we just got one! */
+      connection_mark_for_close(conn, 0);
 #endif
       return 0;
     case RELAY_COMMAND_EXTEND:
@@ -544,8 +534,8 @@ void connection_ap_expire_beginning(void) {
       conn->timestamp_lastread += 15;
       if(connection_ap_handshake_attach_circuit(conn)<0) {
         /* it will never work */
-        conn->marked_for_close = 1;
-        conn->has_sent_end = 1;
+        conn->has_sent_end = 1; /* Don't need to send end -- why? */
+        connection_mark_for_close(conn, 0);
       }
     }
   }
@@ -567,8 +557,8 @@ void connection_ap_attach_pending(void)
       continue;
     if(connection_ap_handshake_attach_circuit(conn) < 0) {
       /* it will never work */
-      conn->marked_for_close = 1;
-      conn->has_sent_end = 1;
+      conn->has_sent_end = 1; /* why? */
+      connection_mark_for_close(conn,0);
     }
   }
 }
@@ -733,7 +723,7 @@ static void connection_ap_handshake_send_begin(connection_t *ap_conn, circuit_t
 
   ap_conn->stream_id = get_unique_stream_id_by_circ(circ);
   if (ap_conn->stream_id==0) {
-    ap_conn->marked_for_close = 1;
+    connection_mark_for_close(ap_conn, 0);
     return;
   }
 
@@ -851,8 +841,7 @@ static int connection_exit_begin_conn(cell_t *cell, circuit_t *circ) {
       return 0;
     case -1: /* resolve failed */
       log_fn(LOG_INFO,"Resolve failed (%s).", n_stream->address);
-      if(connection_edge_end(n_stream, END_STREAM_REASON_RESOLVEFAILED, NULL) < 0)
-        log_fn(LOG_WARN,"1: I called connection_edge_end redundantly.");
+      connection_mark_for_close(n_stream, END_STREAM_REASON_RESOLVEFAILED);
     /* case 0, resolve added to pending list */
   }
   return 0;
@@ -863,15 +852,13 @@ void connection_exit_connect(connection_t *conn) {
 
   if(router_compare_to_my_exit_policy(conn) == ADDR_POLICY_REJECTED) {
     log_fn(LOG_INFO,"%s:%d failed exit policy. Closing.", conn->address, conn->port);
-    if(connection_edge_end(conn, END_STREAM_REASON_EXITPOLICY, NULL) < 0)
-      log_fn(LOG_WARN,"1: I called connection_edge_end redundantly.");
+    connection_mark_for_close(conn, END_STREAM_REASON_EXITPOLICY);
     return;
   }
 
   switch(connection_connect(conn, conn->address, conn->addr, conn->port)) {
     case -1:
-      if(connection_edge_end(conn, END_STREAM_REASON_CONNECTFAILED, NULL) < 0)
-        log_fn(LOG_WARN,"2: I called connection_edge_end redundantly.");
+      connection_mark_for_close(conn, END_STREAM_REASON_CONNECTFAILED);
       return;
     case 0:
       connection_set_poll_socket(conn);

src/or/dns.c

@@ -236,8 +236,7 @@ void dns_cancel_pending_resolve(char *address, connection_t *onlyconn) {
            address);
     while(resolve->pending_connections) {
       pend = resolve->pending_connections;
-      if(connection_edge_end(pend->conn, END_STREAM_REASON_MISC, NULL) < 0)
-        log_fn(LOG_WARN,"1: I called connection_edge_end redundantly.");
+      connection_mark_for_close(pend->conn, END_STREAM_REASON_MISC);
       resolve->pending_connections = pend->next;
       free(pend);
     }
@@ -302,8 +301,7 @@ static void dns_found_answer(char *address, uint32_t addr) {
     assert_connection_ok(pend->conn,0);
     pend->conn->addr = resolve->addr;
     if(resolve->state == CACHE_STATE_FAILED) {
-      if(connection_edge_end(pend->conn, END_STREAM_REASON_RESOLVEFAILED, NULL) < 0)
-        log_fn(LOG_WARN,"1: I called connection_edge_end redundantly.");
+      connection_mark_for_close(pend->conn, END_STREAM_REASON_RESOLVEFAILED);
     } else {
       assert_connection_ok(pend->conn, time(NULL));
       connection_exit_connect(pend->conn);
@@ -457,10 +455,8 @@ static void spawn_enough_dnsworkers(void) {
 
     log_fn(LOG_WARN, "%d DNS workers are spawned; all are busy. Killing one.",
            MAX_DNSWORKERS);
-    /* tell the exit connection that it's failed */
-    dns_cancel_pending_resolve(dnsconn->address, NULL);
 
-    dnsconn->marked_for_close = 1;
+    connection_mark_for_close(dnsconn,0);
     num_dnsworkers_busy--;
     num_dnsworkers--;
   }
@@ -484,7 +480,7 @@ static void spawn_enough_dnsworkers(void) {
            num_dnsworkers-num_dnsworkers_needed, num_dnsworkers);
     dnsconn = connection_get_by_type_state(CONN_TYPE_DNSWORKER, DNSWORKER_STATE_IDLE);
     assert(dnsconn);
-    dnsconn->marked_for_close = 1;
+    connection_mark_for_close(dnsconn,0);
     num_dnsworkers--;
   }
 }

src/or/main.c

@@ -272,7 +272,7 @@ static void run_connection_housekeeping(int i, time_t now) {
       /* we're an onion proxy, with no circuits; or our handshake has expired. kill it. */
       log_fn(LOG_INFO,"Expiring connection to %d (%s:%d).",
              i,conn->address, conn->port);
-      conn->marked_for_close = 1;
+      connection_mark_for_close(conn,0); /* Suppress end ??? */
     } else {
       /* either a full router, or we've got a circuit. send a padding cell. */
       log_fn(LOG_DEBUG,"Sending keepalive to (%s:%d)",

src/or/or.h

@@ -189,12 +189,14 @@
 #define RELAY_COMMAND_TRUNCATED 9
 #define RELAY_COMMAND_DROP 10
 
+#define _MIN_END_STREAM_REASON 1
 #define END_STREAM_REASON_MISC 1
 #define END_STREAM_REASON_RESOLVEFAILED 2
 #define END_STREAM_REASON_CONNECTFAILED 3
 #define END_STREAM_REASON_EXITPOLICY 4
 #define END_STREAM_REASON_DESTROY 5
 #define END_STREAM_REASON_DONE 6
+#define _MAX_END_STREAM_REASON 6
 
 /* default cipher function */
 #define DEFAULT_CIPHER CRYPTO_CIPHER_AES_CTR
@@ -450,6 +452,9 @@ typedef struct {
 struct circuit_t {
   uint32_t magic; /* for memory debugging. */
 
+  int marked_for_close; /* Should we close this circuit at the end of the main
+                         * loop? */
+
   uint32_t n_addr;
   uint16_t n_port;
   connection_t *p_conn;
@@ -631,6 +636,16 @@ connection_t *connection_new(int type);
 void connection_free(connection_t *conn);
 void connection_free_all(void);
 
+int _connection_mark_for_close(connection_t *conn, char reason);
+
+#define connection_mark_for_close(c,r)                                  \
+  do {                                                                  \
+    if (_connection_mark_for_close(c,r)<0) {                            \
+      log(LOG_WARN,"Duplicate call to connection_mark_for_close at %s:%d", \
+          __FILE__,__LINE__);                                           \
+    }                                                                   \
+  } while (0)
+
 int connection_create_listener(char *bindaddress, uint16_t bindport, int type);
 
 int connection_connect(connection_t *conn, char *address, uint32_t addr, uint16_t port);