Commit 4f1a04ff authored by Nick Mathewson's avatar Nick Mathewson
Browse files

Replace nearly all XXX0vv comments with smarter ones

So, back long ago, XXX012 meant, "before Tor 0.1.2 is released, we
had better revisit this comment and fix it!"

But we have a huge pile of such comments accumulated for a large
number of released versions!  Not cool.

So, here's what I tried to do:

  * 0.2.9 and 0.2.8 are retained, since those are not yet released.

  * XXX+ or XXX++ or XXX++++ or whatever means, "This one looks
    quite important!"

  * The others, after one-by-one examination, are downgraded to
    plain old XXX.  Which doesn't mean they aren't a problem -- just
    that they cannot possibly be a release-blocking problem.
parent ce31db43
......@@ -1809,7 +1809,7 @@ MOCK_IMPL(smartlist_t *,get_interface_address6_list,(int severity,
/* ======
* IPv4 helpers
* XXXX024 IPv6 deprecate some of these.
* XXXX IPv6 deprecate some of these.
*/
/** Given an address of the form "ip:port", try to divide it into its
......
......@@ -15,7 +15,8 @@
/* This is required on rh7 to make strptime not complain.
* We also need it to make memmem get defined (where available)
*/
/* XXXX024 We should just use AC_USE_SYSTEM_EXTENSIONS in our autoconf,
/* XXXX We should just use AC_USE_SYSTEM_EXTENSIONS in our autoconf,
* and get this (and other important stuff!) automatically. Once we do that,
* make sure to also change the extern char **environ detection in
* configure.ac, because whether that is declared or not depends on whether
......
......@@ -116,11 +116,11 @@ struct tor_process_monitor_t {
* periodically check whether the process we have a handle to has
* ended. */
HANDLE hproc;
/* XXX023 We can and should have Libevent watch hproc for us,
* if/when some version of Libevent 2.x can be told to do so. */
/* XXXX We should have Libevent watch hproc for us,
* if/when some version of Libevent can be told to do so. */
#endif
/* XXX023 On Linux, we can and should receive the 22nd
/* XXXX On Linux, we can and should receive the 22nd
* (space-delimited) field (‘starttime’) of /proc/$PID/stat from the
* owning controller and store it, and poll once in a while to see
* whether it has changed -- if so, the kernel has *definitely*
......@@ -130,7 +130,8 @@ struct tor_process_monitor_t {
* systems whose admins have mounted procfs, or the start-time field
* of the process-information structure returned by kvmgetprocs() on
* any system. The latter is ickier. */
/* XXX023 On FreeBSD (and possibly other kqueue systems), we can and
/* XXXX On FreeBSD (and possibly other kqueue systems), we can and
* should arrange to receive EVFILT_PROC NOTE_EXIT notifications for
* pid, so we don't have to do such a heavyweight poll operation in
* order to avoid the PID-reassignment race condition. (We would
......
......@@ -2004,8 +2004,7 @@ evdns_request_timeout_callback(int fd, short events, void *arg) {
} else {
/* retransmit it */
/* Stop waiting for the timeout. No need to do this in
* request_finished; that one already deletes the timeout event.
* XXXX023 port this change to libevent. */
* request_finished; that one already deletes the timeout event. */
del_timeout_event(req);
evdns_request_transmit(req);
}
......
......@@ -509,12 +509,12 @@ read_to_chunk_tls(buf_t *buf, chunk_t *chunk, tor_tls_t *tls,
* (because of EOF), set *<b>reached_eof</b> to 1 and return 0. Return -1 on
* error; else return the number of bytes read.
*/
/* XXXX024 indicate "read blocked" somehow? */
/* XXXX indicate "read blocked" somehow? */
int
read_to_buf(tor_socket_t s, size_t at_most, buf_t *buf, int *reached_eof,
int *socket_error)
{
/* XXXX024 It's stupid to overload the return values for these functions:
/* XXXX 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;
......@@ -687,7 +687,7 @@ flush_chunk_tls(tor_tls_t *tls, buf_t *buf, chunk_t *chunk,
int
flush_buf(tor_socket_t s, buf_t *buf, size_t sz, size_t *buf_flushlen)
{
/* XXXX024 It's stupid to overload the return values for these functions:
/* XXXX It's stupid to overload the return values for these functions:
* "error status" and "number of bytes flushed" are not mutually exclusive.
*/
int r;
......
......@@ -85,7 +85,6 @@ pathbias_get_notice_rate(const or_options_t *options)
DFLT_PATH_BIAS_NOTICE_PCT, 0, 100)/100.0;
}
/* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */
/** The circuit success rate below which we issue a warn */
static double
pathbias_get_warn_rate(const or_options_t *options)
......@@ -98,7 +97,7 @@ pathbias_get_warn_rate(const or_options_t *options)
DFLT_PATH_BIAS_WARN_PCT, 0, 100)/100.0;
}
/* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */
/* XXXX I'd like to have this be static again, but entrynodes.c needs it. */
/**
* The extreme rate is the rate at which we would drop the guard,
* if pb_dropguard is also set. Otherwise we just warn.
......@@ -114,7 +113,7 @@ pathbias_get_extreme_rate(const or_options_t *options)
DFLT_PATH_BIAS_EXTREME_PCT, 0, 100)/100.0;
}
/* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */
/* XXXX I'd like to have this be static again, but entrynodes.c needs it. */
/**
* If 1, we actually disable use of guards that fall below
* the extreme_pct.
......
......@@ -805,6 +805,7 @@ circuit_pick_create_handshake(uint8_t *cell_type_out,
uint16_t *handshake_type_out,
const extend_info_t *ei)
{
/* XXXX029 Remove support for deciding to use TAP. */
if (!tor_mem_is_zero((const char*)ei->curve25519_onion_key.public_key,
CURVE25519_PUBKEY_LEN) &&
circuits_can_use_ntor()) {
......@@ -831,9 +832,8 @@ circuit_pick_extend_handshake(uint8_t *cell_type_out,
{
uint8_t t;
circuit_pick_create_handshake(&t, handshake_type_out, ei);
/* XXXX024 The check for whether the node has a curve25519 key is a bad
* proxy for whether it can do extend2 cells; once a version that
* handles extend2 cells is out, remove it. */
/* XXXX029 Remove support for deciding to use TAP. */
if (node_prev &&
*handshake_type_out != ONION_HANDSHAKE_TYPE_TAP &&
(node_has_curve25519_onion_key(node_prev) ||
......@@ -2139,7 +2139,6 @@ choose_good_middle_server(uint8_t purpose,
* If <b>state</b> is NULL, we're choosing a router to serve as an entry
* guard, not for any particular circuit.
*/
/* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */
const node_t *
choose_good_entry_server(uint8_t purpose, cpath_build_state_t *state)
{
......@@ -2172,7 +2171,7 @@ choose_good_entry_server(uint8_t purpose, cpath_build_state_t *state)
* This is an incomplete fix, but is no worse than the previous behaviour,
* and only applies to minimal, testing tor networks
* (so it's no less secure) */
/*XXXX025 use the using_as_guard flag to accomplish this.*/
/*XXXX++ use the using_as_guard flag to accomplish this.*/
if (options->UseEntryGuards
&& (!options->TestingTorNetwork ||
smartlist_len(nodelist_get_list()) > smartlist_len(get_entry_guards())
......
......@@ -203,7 +203,7 @@ circuit_is_better(const origin_circuit_t *oa, const origin_circuit_t *ob,
timercmp(&a->timestamp_began, &b->timestamp_began, OP_GT))
return 1;
if (ob->build_state->is_internal)
/* XXX023 what the heck is this internal thing doing here. I
/* XXXX++ what the heck is this internal thing doing here. I
* think we can get rid of it. circuit_is_acceptable() already
* makes sure that is_internal is exactly what we need it to
* be. -RD */
......@@ -222,7 +222,7 @@ circuit_is_better(const origin_circuit_t *oa, const origin_circuit_t *ob,
break;
}
/* XXXX023 Maybe this check should get a higher priority to avoid
/* XXXX Maybe this check should get a higher priority to avoid
* using up circuits too rapidly. */
a_bits = connection_edge_update_circuit_isolation(conn,
......@@ -1936,8 +1936,8 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
return -1;
}
} else {
/* XXXX024 Duplicates checks in connection_ap_handshake_attach_circuit:
* refactor into a single function? */
/* XXXX Duplicates checks in connection_ap_handshake_attach_circuit:
* refactor into a single function. */
const node_t *node = node_get_by_nickname(conn->chosen_exit_name, 1);
int opt = conn->chosen_exit_optional;
if (node && !connection_ap_can_use_exit(conn, node)) {
......@@ -2416,7 +2416,7 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
/* find the circuit that we should use, if there is one. */
retval = circuit_get_open_circ_or_launch(
conn, CIRCUIT_PURPOSE_C_GENERAL, &circ);
if (retval < 1) // XXX023 if we totally fail, this still returns 0 -RD
if (retval < 1) // XXXX++ if we totally fail, this still returns 0 -RD
return retval;
log_debug(LD_APP|LD_CIRC,
......@@ -2591,7 +2591,7 @@ mark_circuit_unusable_for_new_conns(origin_circuit_t *circ)
const or_options_t *options = get_options();
tor_assert(circ);
/* XXXX025 This is a kludge; we're only keeping it around in case there's
/* XXXX This is a kludge; we're only keeping it around in case there's
* something that doesn't check unusable_for_new_conns, and to avoid
* deeper refactoring of our expiration logic. */
if (! circ->base_.timestamp_dirty)
......
......@@ -99,7 +99,7 @@ static config_abbrev_t option_abbrevs_[] = {
{ "BandwidthRateBytes", "BandwidthRate", 0, 0},
{ "BandwidthBurstBytes", "BandwidthBurst", 0, 0},
{ "DirFetchPostPeriod", "StatusFetchPeriod", 0, 0},
{ "DirServer", "DirAuthority", 0, 0}, /* XXXX024 later, make this warn? */
{ "DirServer", "DirAuthority", 0, 0}, /* XXXX later, make this warn? */
{ "MaxConn", "ConnLimit", 0, 1},
{ "MaxMemInCellQueues", "MaxMemInQueues", 0, 0},
{ "ORBindAddress", "ORListenAddress", 0, 0},
......@@ -2004,11 +2004,6 @@ static const struct {
{ "--list-fingerprint", TAKES_NO_ARGUMENT },
{ "--keygen", TAKES_NO_ARGUMENT },
{ "--newpass", TAKES_NO_ARGUMENT },
#if 0
/* XXXX028: This is not working yet in 0.2.7, so disabling with the
* minimal code modification. */
{ "--master-key", ARGUMENT_NECESSARY },
#endif
{ "--no-passphrase", TAKES_NO_ARGUMENT },
{ "--passphrase-fd", ARGUMENT_NECESSARY },
{ "--verify-config", TAKES_NO_ARGUMENT },
......@@ -2489,7 +2484,6 @@ is_local_addr, (const tor_addr_t *addr))
if (get_options()->EnforceDistinctSubnets == 0)
return 0;
if (tor_addr_family(addr) == AF_INET) {
/*XXXX023 IP6 what corresponds to an /24? */
uint32_t ip = tor_addr_to_ipv4h(addr);
/* It's possible that this next check will hit before the first time
......@@ -5031,7 +5025,7 @@ config_register_addressmaps(const or_options_t *options)
/** As addressmap_register(), but detect the wildcarded status of "from" and
* "to", and do not steal a reference to <b>to</b>. */
/* XXXX024 move to connection_edge.c */
/* XXXX move to connection_edge.c */
int
addressmap_register_auto(const char *from, const char *to,
time_t expires,
......@@ -7572,7 +7566,7 @@ static void
config_maybe_load_geoip_files_(const or_options_t *options,
const or_options_t *old_options)
{
/* XXXX024 Reload GeoIPFile on SIGHUP. -NM */
/* XXXX Reload GeoIPFile on SIGHUP. -NM */
if (options->GeoIPFile &&
((!old_options || !opt_streq(old_options->GeoIPFile,
......
......@@ -115,7 +115,7 @@ int config_parse_commandline(int argc, char **argv, int ignore_errors,
config_line_t **cmdline_result);
void config_register_addressmaps(const or_options_t *options);
/* XXXX024 move to connection_edge.h */
/* XXXX move to connection_edge.h */
int addressmap_register_auto(const char *from, const char *to,
time_t expires,
addressmap_entry_source_t addrmap_source,
......
......@@ -98,7 +98,7 @@ static int get_proxy_type(void);
/** The last addresses that our network interface seemed to have been
* binding to. We use this as one way to detect when our IP changes.
*
* XXX024 We should really use the entire list of interfaces here.
* XXXX+ We should really use the entire list of interfaces here.
**/
static tor_addr_t *last_interface_ipv4 = NULL;
/* DOCDOC last_interface_ipv6 */
......@@ -2932,7 +2932,7 @@ static void
record_num_bytes_transferred(connection_t *conn,
time_t now, size_t num_read, size_t num_written)
{
/* XXX024 check if this is necessary */
/* XXXX check if this is necessary */
if (num_written >= INT_MAX || num_read >= INT_MAX) {
log_err(LD_BUG, "Value out of range. num_read=%lu, num_written=%lu, "
"connection type=%s, state=%s",
......@@ -3759,7 +3759,7 @@ evbuffer_inbuf_callback(struct evbuffer *buf,
connection_consider_empty_read_buckets(conn);
if (conn->type == CONN_TYPE_AP) {
edge_connection_t *edge_conn = TO_EDGE_CONN(conn);
/*XXXX024 check for overflow*/
/*XXXX++ check for overflow*/
edge_conn->n_read += (int)info->n_added;
}
}
......@@ -3780,7 +3780,7 @@ evbuffer_outbuf_callback(struct evbuffer *buf,
connection_consider_empty_write_buckets(conn);
if (conn->type == CONN_TYPE_AP) {
edge_connection_t *edge_conn = TO_EDGE_CONN(conn);
/*XXXX024 check for overflow*/
/*XXXX++ check for overflow*/
edge_conn->n_written += (int)info->n_deleted;
}
}
......@@ -4139,7 +4139,7 @@ connection_handle_write_impl(connection_t *conn, int force)
or_conn->bytes_xmitted += result;
or_conn->bytes_xmitted_by_tls += n_written;
/* So we notice bytes were written even on error */
/* XXXX024 This cast is safe since we can never write INT_MAX bytes in a
/* XXXX This cast is safe since we can never write INT_MAX bytes in a
* single set of TLS operations. But it looks kinda ugly. If we refactor
* the *_buf_tls functions, we should make them return ssize_t or size_t
* or something. */
......
/* Copyright (c) 2001 Matej Pfajfar.
/* Copyright (c) 2001 Matej Pfajfar.
* Copyright (c) 2001-2004, Roger Dingledine.
* Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
* Copyright (c) 2007-2016, The Tor Project, Inc. */
......@@ -919,7 +919,7 @@ connection_ap_warn_and_unmark_if_pending_circ(entry_connection_t *entry_conn,
/** Tell any AP streams that are waiting for a one-hop tunnel to
* <b>failed_digest</b> that they are going to fail. */
/* XXX024 We should get rid of this function, and instead attach
/* XXXX We should get rid of this function, and instead attach
* one-hop streams to circ->p_streams so they get marked in
* circuit_mark_for_close like normal p_streams. */
void
......@@ -1442,7 +1442,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
return -1;
}
/* XXXX024-1090 Should we also allow foo.bar.exit if ExitNodes is set and
/* XXXX-1090 Should we also allow foo.bar.exit if ExitNodes is set and
Bar is not listed in it? I say yes, but our revised manpage branch
implies no. */
}
......@@ -2290,7 +2290,7 @@ connection_ap_handshake_send_begin(entry_connection_t *ap_conn)
edge_conn->stream_id = get_unique_stream_id_by_circ(circ);
if (edge_conn->stream_id==0) {
/* XXXX024 Instead of closing this stream, we should make it get
/* XXXX+ Instead of closing this stream, we should make it get
* retried on another circuit. */
connection_mark_unattached_ap(ap_conn, END_STREAM_REASON_INTERNAL);
......@@ -2382,7 +2382,7 @@ connection_ap_handshake_send_resolve(entry_connection_t *ap_conn)
edge_conn->stream_id = get_unique_stream_id_by_circ(circ);
if (edge_conn->stream_id==0) {
/* XXXX024 Instead of closing this stream, we should make it get
/* XXXX+ Instead of closing this stream, we should make it get
* retried on another circuit. */
connection_mark_unattached_ap(ap_conn, END_STREAM_REASON_INTERNAL);
......
......@@ -583,7 +583,7 @@ connection_or_process_inbuf(or_connection_t *conn)
* check would otherwise just let data accumulate. It serves no purpose
* in 0.2.3.
*
* XXX024 Remove this check once we verify that the above paragraph is
* XXXX Remove this check once we verify that the above paragraph is
* 100% true. */
if (buf_datalen(conn->base_.inbuf) > MAX_OR_INBUF_WHEN_NONOPEN) {
log_fn(LOG_PROTOCOL_WARN, LD_NET, "Accumulated too much data (%d bytes) "
......
......@@ -1863,7 +1863,7 @@ getinfo_helper_dir(control_connection_t *control_conn,
*answer = tor_strndup(body, ri->cache_info.signed_descriptor_len);
}
} else if (!strcmpstart(question, "desc/name/")) {
/* XXX023 Setting 'warn_if_unnamed' here is a bit silly -- the
/* XXX Setting 'warn_if_unnamed' here is a bit silly -- the
* warning goes to the user, not to the controller. */
node = node_get_by_nickname(question+strlen("desc/name/"), 1);
if (node)
......@@ -1949,7 +1949,7 @@ getinfo_helper_dir(control_connection_t *control_conn,
*answer = tor_strndup(md->body, md->bodylen);
}
} else if (!strcmpstart(question, "md/name/")) {
/* XXX023 Setting 'warn_if_unnamed' here is a bit silly -- the
/* XXX Setting 'warn_if_unnamed' here is a bit silly -- the
* warning goes to the user, not to the controller. */
const node_t *node = node_get_by_nickname(question+strlen("md/name/"), 1);
/* XXXX duplicated code */
......
......@@ -494,8 +494,9 @@ MOCK_IMPL(void, directory_get_from_dirserver, (
* sort of dir fetch we'll be doing, so it won't return a bridge
* that can't answer our question.
*/
/* XXX024 Not all bridges handle conditional consensus downloading,
* so, for now, never assume the server supports that. -PP */
/* XXX+++++ Not all bridges handle conditional consensus downloading,
* so, for now, never assume the server supports that. -PP
* Is that assumption still so in 2016? -NM */
const node_t *node = choose_random_dirguard(type);
if (node && node->ri) {
/* every bridge has a routerinfo. */
......@@ -2247,7 +2248,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn)
ds->nickname);
/* XXXX use this information; be sure to upload next one
* sooner. -NM */
/* XXXX023 On further thought, the task above implies that we're
/* XXXX++ On further thought, the task above implies that we're
* basing our regenerate-descriptor time on when we uploaded the
* last descriptor, not on the published time of the last
* descriptor. If those are different, that's a bad thing to
......@@ -3031,7 +3032,7 @@ handle_get_status_vote(dir_connection_t *conn, const get_handler_args_t *args)
ssize_t estimated_len = 0;
smartlist_t *items = smartlist_new();
smartlist_t *dir_items = smartlist_new();
int lifetime = 60; /* XXXX023 should actually use vote intervals. */
int lifetime = 60; /* XXXX?? should actually use vote intervals. */
url += strlen("/tor/status-vote/");
current = !strcmpstart(url, "current/");
url = strchr(url, '/');
......
......@@ -823,7 +823,7 @@ running_long_enough_to_decide_unreachable(void)
void
dirserv_set_router_is_running(routerinfo_t *router, time_t now)
{
/*XXXX024 This function is a mess. Separate out the part that calculates
/*XXXX This function is a mess. Separate out the part that calculates
whether it's reachable and the part that tells rephist that the router was
unreachable.
*/
......@@ -1241,7 +1241,7 @@ dirserv_thinks_router_is_unreliable(time_t now,
{
if (need_uptime) {
if (!enough_mtbf_info) {
/* XXX024 Once most authorities are on v3, we should change the rule from
/* XXXX We should change the rule from
* "use uptime if we don't have mtbf data" to "don't advertise Stable on
* v3 if we don't have enough mtbf data." Or maybe not, since if we ever
* hit a point where we need to reset a lot of authorities at once,
......@@ -3252,7 +3252,7 @@ lookup_cached_dir_by_fp(const char *fp)
d = strmap_get(cached_consensuses, "ns");
} else if (memchr(fp, '\0', DIGEST_LEN) && cached_consensuses &&
(d = strmap_get(cached_consensuses, fp))) {
/* this here interface is a nasty hack XXXX024 */;
/* this here interface is a nasty hack XXXX */;
}
return d;
}
......
......@@ -3534,7 +3534,7 @@ dirvote_create_microdescriptor(const routerinfo_t *ri, int consensus_method)
if (consensus_method >= MIN_METHOD_FOR_P6_LINES &&
ri->ipv6_exit_policy) {
/* XXXX024 This doesn't match proposal 208, which says these should
/* XXXX+++ This doesn't match proposal 208, which says these should
* be taken unchanged from the routerinfo. That's bogosity, IMO:
* the proposal should have said to do this instead.*/
char *p6 = write_short_policy(ri->ipv6_exit_policy);
......
......@@ -722,8 +722,9 @@ entry_guards_compute_status(const or_options_t *options, time_t now)
*
* If <b>mark_relay_status</b>, also call router_set_status() on this
* relay.
*
* XXX024 change succeeded and mark_relay_status into 'int flags'.
*/
/* XXX We could change succeeded and mark_relay_status into 'int flags'.
* Too many boolean arguments is a recipe for confusion.
*/
int
entry_guard_register_connect_status(const char *digest, int succeeded,
......@@ -1466,7 +1467,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg)
}
entry_guards = new_entry_guards;
entry_guards_dirty = 0;
/* XXX024 hand new_entry_guards to this func, and move it up a
/* XXX hand new_entry_guards to this func, and move it up a
* few lines, so we don't have to re-dirty it */
if (remove_obsolete_entry_guards(now))
entry_guards_dirty = 1;
......
......@@ -962,7 +962,7 @@ conn_close_if_marked(int i)
connection_stop_writing(conn);
}
if (connection_is_reading(conn)) {
/* XXXX024 We should make this code unreachable; if a connection is
/* XXXX+ We should make this code unreachable; if a connection is
* marked for close and flushing, there is no point in reading to it
* at all. Further, checking at this point is a bit of a hack: it
* would make much more sense to react in
......
......@@ -925,7 +925,7 @@ we_use_microdescriptors_for_circuits(const or_options_t *options)
return 0;
/* Otherwise, we decide that we'll use microdescriptors iff we are
* not a server, and we're not autofetching everything. */
/* XXX023 what does not being a server have to do with it? also there's
/* XXXX++ what does not being a server have to do with it? also there's
* a partitioning issue here where bridges differ from clients. */
ret = !server_mode(options) && !options->FetchUselessDescriptors;
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment