From 418c2e1b6b7900a1b0b7974738b66f84485b4c36 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 15 Feb 2008 23:39:04 +0000 Subject: [PATCH] r14181@tombo: nickm | 2008-02-15 16:48:17 -0500 Fix all but 2 DOCDOC items; defer many XXX020s (particularly those where fixing them would fix no bugs at the risk of introducing some bugs). svn:r13529 --- src/common/compat.h | 2 +- src/or/buffers.c | 23 ++++++++--------------- src/or/circuitlist.c | 2 +- src/or/connection_edge.c | 28 ++++++++++++++++++---------- src/or/connection_or.c | 2 +- src/or/dnsserv.c | 2 +- src/or/eventdns.c | 2 +- src/or/or.h | 2 +- src/or/rephist.c | 15 ++++++++++----- src/or/routerlist.c | 20 +++++++++----------- 10 files changed, 51 insertions(+), 47 deletions(-) diff --git a/src/common/compat.h b/src/common/compat.h index d8a9826826..7a00edefec 100644 --- a/src/common/compat.h +++ b/src/common/compat.h @@ -340,7 +340,7 @@ typedef struct tor_addr_t } addr; } tor_addr_t; -/* XXXX020 rename these. */ +/* XXXX021 rename these. */ static INLINE uint32_t IPV4IP(const tor_addr_t *a); static INLINE uint32_t IPV4IPh(const tor_addr_t *a); static INLINE uint32_t IPV4MAPh(const tor_addr_t *a); diff --git a/src/or/buffers.c b/src/or/buffers.c index df6b3ed6a0..0c18f9a179 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -119,10 +119,8 @@ typedef struct chunk_freelist_t { /** Static array of freelists, sorted by alloc_len, terminated by an entry * with alloc_size of 0. */ -/**XXXX020 tune these values. And all allocation sizes, really. */ static chunk_freelist_t freelists[] = { - FL(256, 1024, 16), FL(512, 1024, 16), FL(1024, 512, 8), FL(4096, 256, 8), - FL(8192, 128, 4), FL(16384, 64, 4), FL(32768, 32, 2), FL(65536, 16, 2), + FL(4096, 256, 8), FL(8192, 128, 4), FL(16384, 64, 4), FL(32768, 32, 2), FL(0, 0, 0) }; #undef FL @@ -180,7 +178,7 @@ chunk_new_with_alloc_size(size_t alloc) freelist->lowest_length = freelist->cur_length; ++freelist->n_hit; } else { - /* XXXX020 take advantage of tor_malloc_roundup, once we know how that + /* XXXX021 take advantage of tor_malloc_roundup, once we know how that * affects freelists. */ if (freelist) ++freelist->n_alloc; @@ -244,8 +242,6 @@ chunk_grow(chunk_t *chunk, size_t sz) static INLINE size_t preferred_chunk_size(size_t target) { - /* XXXX020 use log2 code, maybe. */ - /* XXXX020 or make sizing code more fine-grained! */ size_t sz = MIN_CHUNK_ALLOC; while (CHUNK_SIZE_WITH_ALLOC(sz) < target) { sz <<= 1; @@ -418,10 +414,7 @@ buf_pullup(buf_t *buf, size_t bytes, int nulterminate) } /** Resize buf so it won't hold extra memory that we haven't been - * using lately (that is, since the last time we called buf_shrink). - * Try to shrink the buf until it is the largest factor of two that - * can contain buf->highwater, but never smaller than - * MIN_LAZY_SHRINK_SIZE. + * using lately. */ void buf_shrink(buf_t *buf) @@ -454,8 +447,8 @@ buf_remove_from_front(buf_t *buf, size_t n) check(); } -/** Create and return a new buf with capacity size. - * (Used for testing). */ +/** Create and return a new buf with default chunk capacity size. + */ buf_t * buf_new_with_capacity(size_t size) { @@ -609,11 +602,11 @@ read_to_chunk_tls(buf_t *buf, chunk_t *chunk, tor_tls_t *tls, * (because of EOF), set *reached_eof to 1 and return 0. Return -1 on * error; else return the number of bytes read. */ -/* XXXX020 indicate "read blocked" somehow? */ +/* XXXX021 indicate "read blocked" somehow? */ int read_to_buf(int s, size_t at_most, buf_t *buf, int *reached_eof) { - /* XXXX020 It's stupid to overload the return values for these functions: + /* XXXX021 It's stupid to overload the return values for these functions: * "error status" and "number of bytes read" are not mutually exclusive. */ int r = 0; @@ -777,7 +770,7 @@ flush_chunk_tls(tor_tls_t *tls, buf_t *buf, chunk_t *chunk, int flush_buf(int s, buf_t *buf, size_t sz, size_t *buf_flushlen) { - /* XXXX020 It's stupid to overload the return values for these functions: + /* XXXX021 It's stupid to overload the return values for these functions: * "error status" and "number of bytes flushed" are not mutually exclusive. */ int r; diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 45c189b3f6..b1ad2e73db 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -806,7 +806,7 @@ origin_circuit_t * circuit_find_to_cannibalize(uint8_t purpose, extend_info_t *info, int flags) { - /*XXXX020 arma: The purpose argument is ignored. Can that possibly be + /*XXXX021 arma: The purpose argument is ignored. Can that possibly be * right? */ /* XXXX i don't know of any actual bugs that this causes. since i * think we only call the function for purposes where we want it to do what diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 16403bd527..15bea7f632 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -925,9 +925,12 @@ client_dns_set_reverse_addressmap(const char *address, const char *v, * * These options are configured by parse_virtual_addr_network(). */ -/*DOCDOC options */ +/** Which network should we use for virtual IPv4 addresses? Only the first + * bits of this value are fixed. */ static uint32_t virtual_addr_network = 0x7fc00000u; +/** How many bits of virtual_addr_network are fixed? */ static maskbits_t virtual_addr_netmask_bits = 10; +/** What's the next virtual address we will hand out? */ static uint32_t next_virtual_addr = 0x7fc00000u; /** Read a netmask of the form 127.192.0.0/10 from "val", and check whether @@ -1944,7 +1947,7 @@ connection_ap_handshake_send_resolve(edge_connection_t *ap_conn) uint32_t a; size_t len = strlen(ap_conn->socks_request->address); char c = 0; - /* XXXX020 This logic is a little ugly: we check for an in-addr.arpa ending + /* XXXX021 This logic is a little ugly: we check for an in-addr.arpa ending * on the address. If we have one, the address is already in the right * order, so we'll leave it alone later. Otherwise, we reverse it and * turn it into an in-addr.arpa address. */ @@ -1958,11 +1961,11 @@ connection_ap_handshake_send_resolve(edge_connection_t *ap_conn) return -1; } if (c) { - /* this path happens on DNS. Can we unify? XXXX020 */ + /* this path happens on DNS. Can we unify? XXXX021 */ ap_conn->socks_request->address[len-13] = c; strlcpy(inaddr_buf, ap_conn->socks_request->address, sizeof(inaddr_buf)); } else { - /* this path happens on tor-resolve. Can we unify? XXXX020 */ + /* this path happens on tor-resolve. Can we unify? XXXX021 */ a = ntohl(in.s_addr); tor_snprintf(inaddr_buf, sizeof(inaddr_buf), "%d.%d.%d.%d.in-addr.arpa", (int)(uint8_t)((a )&0xff), @@ -2085,12 +2088,15 @@ tell_controller_about_resolved_result(edge_connection_t *conn, } } -/** Send an answer to an AP connection that has requested a DNS lookup - * via SOCKS. The type should be one of RESOLVED_TYPE_(IPV4|IPV6|HOSTNAME) or - * -1 for unreachable; the answer should be in the format specified - * in the socks extensions document. - * DOCDOC ttl expires +/** Send an answer to an AP connection that has requested a DNS lookup via + * SOCKS. The type should be one of RESOLVED_TYPE_(IPV4|IPV6|HOSTNAME) or -1 + * for unreachable; the answer should be in the format specified in the socks + * extensions document. ttl is the ttl for the answer, or -1 on + * certain errors or for values that didn't come via DNS. expires is + * a time when the answer expires, or -1 or TIME_MAX if there's a good TTL. **/ +/* XXXX021 the use of the ttl and expires fields is nutty. Let's make this + * interface and those that use it less ugly. */ void connection_ap_handshake_socks_resolved(edge_connection_t *conn, int answer_type, @@ -2341,7 +2347,9 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ) address = tor_strdup(or_circ->p_conn->_base.address); else address = tor_strdup("127.0.0.1"); - port = 1; /*XXXX020 set this to something sensible? - NM*/ + port = 1; /* XXXX This value is never actually used anywhere, and there + * isn't "really" a connection here. But we + * need to set it to something nonzero. */ } else { log_warn(LD_BUG, "Got an unexpected command %d", (int)rh.command); end_payload[0] = END_STREAM_REASON_INTERNAL; diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 812a2d4d6e..7017ec0ef7 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -404,7 +404,7 @@ connection_or_init_conn_from_address(or_connection_t *conn, /* Override the addr/port, so our log messages will make sense. * This is dangerous, since if we ever try looking up a conn by * its actual addr/port, we won't remember. Careful! */ - /* XXXX020 arma: this is stupid, and it's the reason we need real_addr + /* XXXX021 arma: this is stupid, and it's the reason we need real_addr * to track is_canonical properly. What requires it? */ /* XXXX i believe the reason we did this, originally, is because * we wanted to log what OR a connection was to, and if we logged the diff --git a/src/or/dnsserv.c b/src/or/dnsserv.c index 799f473974..933ab699ab 100644 --- a/src/or/dnsserv.c +++ b/src/or/dnsserv.c @@ -227,7 +227,7 @@ dnsserv_resolved(edge_connection_t *conn, if (!req) return; - /* XXXX020 Re-do; this is dumb. */ + /* XXXX021 Re-do; this is dumb. */ if (ttl < 60) ttl = 60; diff --git a/src/or/eventdns.c b/src/or/eventdns.c index a80e019298..744077c724 100644 --- a/src/or/eventdns.c +++ b/src/or/eventdns.c @@ -1829,7 +1829,7 @@ server_port_free(struct evdns_server_port *port) } (void) event_del(&port->event); CLEAR(&port->event); - /* XXXX020 actually free the port? -NM */ + /* XXXX021 actually free the port? -NM */ /* XXXX yes, and fix up evdns_close_server_port to dtrt. -NM */ } diff --git a/src/or/or.h b/src/or/or.h index 5ae2c00737..3cd7fd7d53 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -882,7 +882,7 @@ typedef struct connection_t { /** Annother connection that's connected to this one in lieu of a socket. */ struct connection_t *linked_conn; - /* XXXX020 move this into a subtype!!! */ + /* XXXX021 move this into a subtype. */ struct evdns_server_port *dns_server_port; } connection_t; diff --git a/src/or/rephist.c b/src/or/rephist.c index 34d51261eb..5153d719f7 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -311,7 +311,8 @@ rep_hist_note_router_unreachable(const char *id, time_t when) if (!started_tracking_stability) started_tracking_stability = time(NULL); if (hist && hist->start_of_run) { - /*XXXX020 treat failure specially? */ + /*XXXX We could treat failed connections differently from failed + * conect attempts. */ long run_length = when - hist->start_of_run; hist->weighted_run_length += run_length; hist->total_run_weights += 1.0; @@ -388,7 +389,8 @@ get_stability(or_history_t *hist, time_t when) return total / total_weights; } -/** DODDOC */ +/** Return the total amount of time we've been observing, with each run of + * time downrated by the appropriate factor. */ static long get_total_weighted_time(or_history_t *hist, time_t when) { @@ -464,7 +466,7 @@ rep_hist_get_weighted_time_known(const char *id, time_t when) int rep_hist_have_measured_enough_stability(void) { - /* XXXX020 This doesn't do so well when we change our opinion + /* XXXX021 This doesn't do so well when we change our opinion * as to whether we're tracking router stability. */ return started_tracking_stability < time(NULL) - 4*60*60; } @@ -755,7 +757,11 @@ parse_possibly_bad_iso_time(const char *s, time_t *time_out) return parse_iso_time(s, time_out); } -/** DOCDOC */ +/** We've read a time t from a file stored at stored_at, which + * says we started measuring at started_measuring. Return a new number + * that's about as much before now as t was before + * stored_at. + */ static INLINE time_t correct_time(time_t t, time_t now, time_t stored_at, time_t started_measuring) { @@ -868,7 +874,6 @@ rep_hist_load_mtbf_data(time_t now) wfu_timebuf[0] = '\0'; if (format == 1) { - /* XXXX020 audit the heck out of my scanf usage. */ n = sscanf(line, "%40s %ld %lf S=%10s %8s", hexbuf, &wrl, &trw, mtbf_timebuf, mtbf_timebuf+11); if (n != 3 && n != 5) { diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 1f0e603d69..2cf91d327e 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -1388,16 +1388,14 @@ get_max_believable_bandwidth(void) * If statuses is zero, then sl is a list of * routerinfo_t's. Otherwise it's a list of routerstatus_t's. * - * If for_exit, we're picking an exit node: consider all nodes' - * bandwidth equally regardless of their Exit status, since there may be - * some in the list because they exit to obscure ports. If not for_exit, - * we're picking a non-exit node: weight exit-node's bandwidth less - * depending on the smallness of the fraction of Exit-to-total bandwidth. - * - * If for_guard, we're picking a guard node: consider all guard's - * bandwidth equally. Otherwise, weight guards proportionally less. - * - * XXX DOCDOC the above args aren't args anymore + * If rule==WEIGHT_FOR_EXIT. we're picking an exit node: consider all + * nodes' bandwidth equally regardless of their Exit status, since there may + * be some in the list because they exit to obscure ports. If + * rule==NO_WEIGHTING, we're picking a non-exit node: weight + * exit-node's bandwidth less depending on the smallness of the fraction of + * Exit-to-total bandwidth. If rule==WEIGHT_FOR_GUARD, we're picking a + * guard node: consider all guard's bandwidth equally. Otherwise, weight + * guards proportionally less. */ static void * smartlist_choose_by_bandwidth(smartlist_t *sl, bandwidth_weight_rule_t rule, @@ -3666,7 +3664,7 @@ launch_router_descriptor_downloads(smartlist_t *downloadable, time_t now) } } } - /* XXX020 should we consider having even the dir mirrors delay + /* XXX should we consider having even the dir mirrors delay * 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