r12936@catbus: nickm | 2007-05-24 14:12:34 -0400

 Review XXXX comments without a version; upgrade some to XXXX020.


svn:r10315

src/common/tortls.c

@@ -183,7 +183,7 @@ tor_tls_get_error(tor_tls_t *tls, int r, int extra,
         return _TOR_TLS_ZERORETURN;
       log(severity, LD_NET, "TLS error: Zero return");
       tls_log_errors(severity, doing);
-      /* XXXX Actually, a 'zero return' error has a pretty specific meaning:
+      /* XXXX020 Actually, a 'zero return' error has a pretty specific meaning:
        * the connection has been closed cleanly.  */
       return TOR_TLS_ERROR_MISC;
     default:

src/or/connection_edge.c

@@ -1884,7 +1884,7 @@ connection_ap_make_bridge(char *address, uint16_t port,
                   digest, DIGEST_LEN);
   }
 
-  conn->_base.address = tor_strdup("(local bridge)"); /*XXXX020 no "bridge"*/
+  conn->_base.address = tor_strdup("(local link)");
   conn->_base.addr = 0;
   conn->_base.port = 0;
 
@@ -2445,7 +2445,7 @@ int
 connection_edge_is_rendezvous_stream(edge_connection_t *conn)
 {
   tor_assert(conn);
-  if (*conn->rend_query) /* XXX */
+  if (*conn->rend_query) /* XXX */ /* XXXX Why is this XXX? -NM */
     return 1;
   return 0;
 }

src/or/control.c

@@ -876,7 +876,9 @@ handle_control_setevents(control_connection_t *conn, uint32_t len,
       else if (!strcasecmp(ev, "GUARD"))
         event_code = EVENT_GUARD;
       else if (!strcasecmp(ev, "GUARDS")) {
-        /* XXX tolerate buggy spec in 0.1.2.5-alpha through 0.1.2.10-rc */
+        /* XXXX021 This check is here to tolerate the controllers that
+         * depended on the buggy spec in 0.1.2.5-alpha through 0.1.2.10-rc.
+         * Once those versions are obsolete, stop supporting this. */
         log_warn(LD_CONTROL, "Controller used obsolete 'GUARDS' event name; "
                  "use GUARD instead.");
         event_code = EVENT_GUARD;
@@ -3083,7 +3085,7 @@ control_event_networkstatus_changed(smartlist_t *statuses)
       smartlist_add(strs, s);
     });
   smartlist_add(strs, tor_strdup("\r\n.\r\n"));
-  /* XXX the above strdup has an extra \r\n in it, resulting in
+  /* XXX020 the above strdup has an extra \r\n in it, resulting in
    * a blank line in the NS output. Can we remove it, or is that
    * bad since the output of networkstatus_getinfo_helper_single()
    * only adds \n, not \r\n? */

src/or/directory.c

@@ -1140,7 +1140,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn)
     log_info(LD_DIR,"Received networkstatus objects (size %d) from server "
              "'%s:%d'",(int) body_len, conn->_base.address, conn->_base.port);
     if (status_code != 200) {
-      /* XXXX This warning tends to freak out clients who get a 403. */
+      /* XXXX020 This warning tends to freak out clients who get a 403. */
       log_warn(LD_DIR,
            "Received http status code %d (%s) from server "
            "'%s:%d' while fetching \"/tor/status/%s\". I'll try again soon.",
@@ -1223,7 +1223,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn)
         (status_code == 400 && !strcmp(reason, "Servers unavailable."));
       /* 404 means that it didn't have them; no big deal.
        * Older (pre-0.1.1.8) servers said 400 Servers unavailable instead. */
-      /* XXXX This warning tends to freak out clients who get a 403. */
+      /* XXXX020 This warning tends to freak out clients who get a 403. */
       log_fn(dir_okay ? LOG_INFO : LOG_WARN, LD_DIR,
              "Received http status code %d (%s) from server '%s:%d' "
              "while fetching \"/tor/server/%s\". I'll try again soon.",

src/or/dns.c

@@ -565,6 +565,8 @@ dns_resolve(edge_connection_t *exitconn)
         // If it's marked for close, it's on closeable_connection_lst in
         // main.c.  If it's on the closeable list, it will get freed from
         // main.c. -NM
+        // "<armadev> If that's true, there are other bugs arond, where we
+        //  don't check if it's marked, and will end up double-freeing."
       }
       break;
     default:
@@ -814,6 +816,7 @@ dns_cancel_pending_resolve(const char *address)
 
   if (!resolve->pending_connections) {
     /* XXX this should never trigger, but sometimes it does */
+    /* XXXX020 is the above still true? -NM */
     log_warn(LD_BUG,
              "Address %s is pending but has no pending connections!",
              escaped_safe_str(address));
@@ -940,7 +943,7 @@ dns_found_answer(const char *address, int is_reverse, uint32_t addr,
   assert_resolve_ok(resolve);
 
   if (resolve->state != CACHE_STATE_PENDING) {
-    /* XXXX Maybe update addr? or check addr for consistency? Or let
+    /* XXXX020 Maybe update addr? or check addr for consistency? Or let
      * VALID replace FAILED? */
     int is_test_addr = is_test_address(address);
     if (!is_test_addr)

src/or/main.c

@@ -100,7 +100,7 @@ int has_completed_circuit=0;
 
 SERVICE_STATUS service_status;
 SERVICE_STATUS_HANDLE hStatus;
-/* XXXX This 'backup argv' and 'backup argc' business is an ugly hack. This
+/* XXXX020 This 'backup argv' and 'backup argc' business is an ugly hack. This
  * is a job for arguments, not globals. */
 static char **backup_argv;
 static int backup_argc;
@@ -510,7 +510,7 @@ conn_write_callback(int fd, short events, void *_conn)
           edge_conn->end_reason = END_STREAM_REASON_INTERNAL;
         conn->edge_has_sent_end = 1;
       }
-      /* XXX do we need a close-immediate here, so we don't try to flush? */
+      /* XXX020 do we need a close-immediate here, so we don't try to flush? */
       connection_mark_for_close(conn);
     }
   }
@@ -583,7 +583,7 @@ conn_close_if_marked(int i)
         LOG_FN_CONN(conn, (LOG_INFO,LD_NET,
                            "Holding conn (fd %d) open for more flushing.",
                            conn->s));
-      /* XXX should we reset timestamp_lastwritten here? */
+      /* XXX020 should we reset timestamp_lastwritten here? */
       return 0;
     }
     if (connection_wants_to_flush(conn)) {
@@ -902,7 +902,7 @@ run_scheduled_events(time_t now)
   if (time_to_fetch_directory < now) {
     /* Only caches actually need to fetch directories now. */
     if (options->DirPort && !authdir_mode_v1(options)) {
-      /* XXX actually, we should only do this if we want to advertise
+      /* XXX020 actually, we should only do this if we want to advertise
        * our dirport. not simply if we configured one. -RD */
       if (any_trusted_dir_is_v1_authority())
         directory_get_from_dirserver(DIR_PURPOSE_FETCH_DIR, NULL, 1);
@@ -1380,7 +1380,7 @@ do_main_loop(void)
 
     /* refilling buckets and sending cells happens at the beginning of the
      * next iteration of the loop, inside prepare_for_poll()
-     * XXXX No longer so.
+     * XXXX020 No longer so; fix comment.
      */
   }
 }
@@ -1483,7 +1483,7 @@ signal_callback(int fd, short events, void *arg)
             "Rate limiting NEWNYM request: delaying by %d second(s)",
             (int)(MAX_SIGNEWNYM_RATE+time_of_last_signewnym-now));
       } else {
-        /* XXX refactor someday: these two calls are in
+        /* XXX020 refactor someday: these two calls are in
          * run_scheduled_events() above too, and they should be in just
          * one place. */
         circuit_expire_all_dirty_circs();
@@ -1775,7 +1775,10 @@ tor_cleanup(void)
     or_state_mark_dirty(get_or_state(), 0); /* force an immediate save. */
     or_state_save(time(NULL));
   }
-  tor_free_all(0); /* move tor_free_all back into the ifdef below later. XXX*/
+  tor_free_all(0); /* We could move tor_free_all back into the ifdef below
+                      later, if it makes shutdown unacceptably slow.  But for
+                      now, leave it here: it's helped us catch bugs in the
+                      past. */
   crypto_global_cleanup();
 #ifdef USE_DMALLOC
   dmalloc_log_unfreed();

src/or/relay.c

@@ -358,7 +358,8 @@ circuit_package_relay_cell(cell_t *cell, circuit_t *circ,
   } else { /* incoming cell */
     or_circuit_t *or_circ;
     if (CIRCUIT_IS_ORIGIN(circ)) {
-      /* XXXX RD This is a bug, right? */
+      /* We should never package an _incoming_ cell from the circuit
+       * origin; that means we messed up somewhere. */
       log_warn(LD_BUG,"incoming relay cell at origin circuit. Dropping.");
       assert_circuit_ok(circ);
       return 0; /* just drop it */

src/or/routerlist.c

@@ -1467,7 +1467,7 @@ router_get_by_nickname(const char *nickname, int warn_if_unnamed)
       if (router_hex_digest_matches(router, nickname))
         return router;
       else
-        best_match = router; // XXXX NM not exactly right.
+        best_match = router; // XXXX020 NM not exactly right.
     }
   });