r14170@tombo: nickm | 2008-02-15 11:50:38 -0500

Fix or downgrade a bunch of xxx020 items.


svn:r13527

src/or/networkstatus.c

@@ -865,6 +865,7 @@ router_get_consensus_status_by_nickname(const char *nickname,
   if (!current_consensus || !nickname)
     return NULL;
 
+  /* Is this name really a hexadecimal identity digest? */
   if (nickname[0] == '$') {
     if (base16_decode(digest, DIGEST_LEN, nickname+1, strlen(nickname+1))<0)
       return NULL;
@@ -874,16 +875,23 @@ router_get_consensus_status_by_nickname(const char *nickname,
     return networkstatus_vote_find_entry(current_consensus, digest);
   }
 
+  /* Is there a server that is Named with this name? */
   if (named_server_map)
     named_id = strmap_get_lc(named_server_map, nickname);
   if (named_id)
     return networkstatus_vote_find_entry(current_consensus, named_id);
 
+  /* Okay; is this name listed as Unnamed? */
   if (unnamed_server_map &&
-      strmap_get_lc(unnamed_server_map, nickname))
+      strmap_get_lc(unnamed_server_map, nickname)) {
-    return NULL; /* XXXX020 should we warn? */
+    log_info(LD_GENERAL, "The name %s is listed as Unnamed; it is not the "
+             "canonical name of any server we know.", escaped(nickname));
+    return NULL;
+  }
 
-  /*XXXX020 is this behavior really what we want? */
+  /* This name is not canonical for any server; go through the list and
+   * see who it matches. */
+  /*XXXX021 This is inefficient. */
   matches = smartlist_create();
   SMARTLIST_FOREACH(current_consensus->routerstatus_list,
                     routerstatus_t *, lrs,
@@ -1000,7 +1008,9 @@ update_v2_networkstatus_cache_downloads(time_t now)
             * doing a tunneled conn. In that case it should be or_port.
             * How to guess from here? Maybe make the function less general
             * and have it know that it's looking for dir conns. -RD */
-           /* We are already fetching this one. */
+           /* Only directory caches download v2 networkstatuses, and they
+            * don't use tunneled connections.  I think it's okay to ignore
+            * this. */
            continue;
          }
          strlcpy(resource, "fp/", sizeof(resource));

src/or/relay.c

@@ -503,7 +503,7 @@ relay_send_command_from_edge(uint16_t stream_id, circuit_t *circ,
 
   if (cell_direction == CELL_DIRECTION_OUT && circ->n_conn) {
     /* if we're using relaybandwidthrate, this conn wants priority */
-    /* XXXX020 the call to time() seems little too frequent */
+    /* XXXX021 the call to time() seems little too frequent */
     circ->n_conn->client_used = time(NULL);
   }
 
@@ -1511,7 +1511,7 @@ circuit_consider_sending_sendme(circuit_t *circ, crypt_path_t *layer_hint)
 static int total_cells_allocated = 0;
 
 #ifdef ENABLE_CELL_POOL /* Defined in ./configure. True by default. */
-/* XXX020 make cell pools the only option once we know they work and improve
+/* XXX021 make cell pools the only option once we know they work and improve
  * matters? -RD */
 static mp_pool_t *cell_pool = NULL;
 /** Allocate structures to hold cells. */

src/or/rephist.c

@@ -678,8 +678,9 @@ rep_hist_record_mtbf_data(void)
 
   PUT("data\n");
 
-  /* XXX020 Nick: now bridge auths record this for all routers too.
+  /* XXX021 Nick: now bridge auths record this for all routers too.
-   * Should we make them record it only for bridge routers? */
+   * Should we make them record it only for bridge routers? -RD
+   * Not for 0.2.0. -NM */
   for (orhist_it = digestmap_iter_init(history_map);
        !digestmap_iter_done(orhist_it);
        orhist_it = digestmap_iter_next(history_map,orhist_it)) {

src/or/routerlist.c

@@ -837,9 +837,11 @@ router_pick_directory_server(authority_type_t type, int flags)
   if (choice)
     return choice;
 
-  /* XXXX020 arma: what's the point of *reloading* and trying again?? -NM */
+  /* XXXX021 arma: what's the point of *reloading* and trying again?? -NM */
-  /* XXXX020 <arma> once upon a time, reloading set the is_running back
+  /* XXXX021 <arma> once upon a time, reloading set the is_running back
      to 1. i think. i bet it has no purpose now. */
+  /* XXXX021 Let's stop reloading in 0.2.1.x, then, and see if anything
+   * breaks -NM */
   log_info(LD_DIR,"Still no %s router entries. Reloading and trying again.",
            (flags & PDS_IGNORE_FASCISTFIREWALL) ? "known" : "reachable");
   if (router_reload_router_list()) {
@@ -2447,8 +2449,9 @@ routerlist_remove_old(routerlist_t *rl, signed_descriptor_t *sd, int idx)
     idx = sd->routerlist_index;
   }
   tor_assert(0 <= idx && idx < smartlist_len(rl->old_routers));
-  /* XXX020 edmanm's bridge relay triggered the following assert while
+  /* XXXX edmanm's bridge relay triggered the following assert while
-   * running 0.2.0.12-alpha. */
+   * running 0.2.0.12-alpha.  If anybody triggers this again, see if we
+   * can ge a backtrace. */
   tor_assert(smartlist_get(rl->old_routers, idx) == sd);
   tor_assert(idx == sd->routerlist_index);
 
@@ -3288,8 +3291,10 @@ router_load_extrainfo_from_string(const char *s, const char *eos,
                         ei->cache_info.identity_digest,
                       DIGEST_LEN);
         smartlist_string_remove(requested_fingerprints, fp);
-        /* XXX020 We silently let people stuff us with extrainfos we
+        /* We silently let people stuff us with extrainfos we didn't ask for,
-         * didn't ask for. Is this a problem? -RD */
+         * so long as we would have wanted them anyway.  Since we always fetch
+         * all the extrainfos we want, and we never actually act on them
+         * inside Tor, this should be harmless. */
       }
     });
 
@@ -3662,7 +3667,10 @@ launch_router_descriptor_downloads(smartlist_t *downloadable, time_t now)
     }
   }
   /* XXX020 should we consider having even the dir mirrors delay
-   * a little bit, so we don't load the authorities as much? -RD */
+   * a little bit, so we don't load the authorities as much? -RD
+   * I don't think so.  If we do, clients that want those descriptors may
+   * not actually find them if the caches haven't got them yet. -NM
+   */
 
   if (! should_delay && n_downloadable) {
     int i, n_per_request;
@@ -4328,7 +4336,7 @@ routerlist_assert_ok(routerlist_t *rl)
     tor_assert(&(r->cache_info) == sd2);
     tor_assert(r->cache_info.routerlist_index == r_sl_idx);
 #if 0
-    /* XXXX020.
+    /* XXXX021.
      *
      *   Hoo boy.  We need to fix this one, and the fix is a bit tricky, so
      * commenting this out is just a band-aid.
@@ -4343,7 +4351,8 @@ routerlist_assert_ok(routerlist_t *rl)
      * refactoring once consensus directories are in.  For now,
      * this rep violation is probably harmless: an adversary can make us
      * reset our retry count for an extrainfo, but that's not the end
-     * of the world.
+     * of the world.  Changing the representation in 0.2.0.x would just
+     * destabilize the codebase.
      */
     if (!tor_digest_is_zero(r->cache_info.extra_info_digest)) {
       signed_descriptor_t *sd3 =
@@ -4360,7 +4369,7 @@ routerlist_assert_ok(routerlist_t *rl)
     tor_assert(sd == sd2);
     tor_assert(sd->routerlist_index == sd_sl_idx);
 #if 0
-    /* XXXX020 see above. */
+    /* XXXX021 see above. */
     if (!tor_digest_is_zero(sd->extra_info_digest)) {
       signed_descriptor_t *sd3 =
         sdmap_get(rl->desc_by_eid_map, sd->extra_info_digest);
@@ -4386,7 +4395,7 @@ routerlist_assert_ok(routerlist_t *rl)
                        d, DIGEST_LEN));
     sd = sdmap_get(rl->desc_by_eid_map,
                    ei->cache_info.signed_descriptor_digest);
-    // tor_assert(sd); // XXXX020 see above
+    // tor_assert(sd); // XXXX021 see above
     if (sd) {
       tor_assert(!memcmp(ei->cache_info.signed_descriptor_digest,
                          sd->extra_info_digest, DIGEST_LEN));