commit 84bfa895d725338d92f677a31a4dcf6381845e0c Author: Nick Mathewson <nickm@torproject.org> Date: Tue Nov 29 11:47:12 2016 -0500 Change return value of entry_guard_succeeded to an enum. George pointed out that (-1,0,1) for (never usable, maybe usable later, usable right now) was a pretty rotten convention that made the code harder to read. --- src/or/circuitbuild.c | 13 +++++++------ src/or/entrynodes.c | 28 +++++++++++++--------------- src/or/entrynodes.h | 7 ++++++- src/test/test_entrynodes.c | 29 ++++++++++++++++------------- 4 files changed, 42 insertions(+), 35 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 5d0a04f..c7e116e 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -964,28 +964,29 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) memset(&ec, 0, sizeof(ec)); if (!hop) { /* done building the circuit. whew. */ - int r; + guard_usable_t r; if (get_options()->UseDeprecatedGuardAlgorithm) { // The circuit is usable; we already marked the guard as okay. - r = 1; + r = GUARD_USABLE_NOW; } else if (! circ->guard_state) { if (circuit_get_cpath_len(circ) != 1) { log_warn(LD_BUG, "%d-hop circuit %p with purpose %d has no " "guard state", circuit_get_cpath_len(circ), circ, circ->base_.purpose); } - r = 1; + r = GUARD_USABLE_NOW; } else { r = entry_guard_succeeded(&circ->guard_state); } - const int is_usable_for_streams = (r == 1); - if (r == 1) { + const int is_usable_for_streams = (r == GUARD_USABLE_NOW); + if (r == GUARD_USABLE_NOW) { circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_OPEN); - } else if (r == 0) { + } else if (r == GUARD_MAYBE_USABLE_LATER) { // XXXX prop271 we might want to probe for whether this // XXXX one is ready even before the next second rolls over. circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_GUARD_WAIT); } else { + tor_assert_nonfatal(r == GUARD_USABLE_NEVER); return - END_CIRC_REASON_INTERNAL; } diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index af1869f..aa90566 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1965,28 +1965,26 @@ entry_guard_pick_for_circuit(guard_selection_t *gs, } /** - * Called by the circuit building module when a circuit has succeeded: - * informs the guards code that the guard in *<b>guard_state_p</b> is - * working, and advances the state of the guard module. On a -1 return - * value, the circuit is broken and should not be used. On a 1 return - * value, the circuit is ready to use. On a 0 return value, the circuit - * should not be used until we find out whether preferred guards will - * work for us. - * - * XXXXX prop271 tristates are ugly; reconsider that interface. + * Called by the circuit building module when a circuit has succeeded: informs + * the guards code that the guard in *<b>guard_state_p</b> is working, and + * advances the state of the guard module. On a GUARD_USABLE_NEVER return + * value, the circuit is broken and should not be used. On a GUARD_USABLE_NOW + * return value, the circuit is ready to use. On a GUARD_MAYBE_USABLE_LATER + * return value, the circuit should not be used until we find out whether + * preferred guards will work for us. */ -int +guard_usable_t entry_guard_succeeded(circuit_guard_state_t **guard_state_p) { if (get_options()->UseDeprecatedGuardAlgorithm) - return 1; + return GUARD_USABLE_NOW; if (BUG(*guard_state_p == NULL)) - return -1; + return GUARD_USABLE_NEVER; entry_guard_t *guard = entry_guard_handle_get((*guard_state_p)->guard); if (! guard || BUG(guard->in_selection == NULL)) - return -1; + return GUARD_USABLE_NEVER; unsigned newstate = entry_guards_note_guard_success(guard->in_selection, guard, @@ -1996,9 +1994,9 @@ entry_guard_succeeded(circuit_guard_state_t **guard_state_p) (*guard_state_p)->state_set_at = approx_time(); if (newstate == GUARD_CIRC_STATE_COMPLETE) { - return 1; + return GUARD_USABLE_NOW; } else { - return 0; + return GUARD_MAYBE_USABLE_LATER; } } diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index 21dab6e..ceccd0f 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -359,7 +359,12 @@ void circuit_guard_state_free(circuit_guard_state_t *state); int entry_guard_pick_for_circuit(guard_selection_t *gs, const node_t **chosen_node_out, circuit_guard_state_t **guard_state_out); -int entry_guard_succeeded(circuit_guard_state_t **guard_state_p); +typedef enum { + GUARD_USABLE_NEVER = -1, + GUARD_MAYBE_USABLE_LATER = 0, + GUARD_USABLE_NOW = 1, +} guard_usable_t; +guard_usable_t entry_guard_succeeded(circuit_guard_state_t **guard_state_p); void entry_guard_failed(circuit_guard_state_t **guard_state_p); void entry_guard_cancel(circuit_guard_state_t **guard_state_p); void entry_guard_chan_failed(channel_t *chan); diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 32af7ff..6a3048b 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -2324,6 +2324,7 @@ test_entry_guard_select_for_circuit_highlevel_primary(void *arg) const node_t *node = NULL; circuit_guard_state_t *guard = NULL; entry_guard_t *g; + guard_usable_t u; /* * Make sure that the pick-for-circuit API basically works. We'll get * a primary guard, so it'll be usable on completion. @@ -2343,8 +2344,8 @@ test_entry_guard_select_for_circuit_highlevel_primary(void *arg) /* Call that circuit successful. */ update_approx_time(start+15); - r = entry_guard_succeeded(&guard); - tt_int_op(r, OP_EQ, 1); /* We can use it now. */ + u = entry_guard_succeeded(&guard); + tt_int_op(u, OP_EQ, GUARD_USABLE_NOW); /* We can use it now. */ tt_assert(guard); tt_int_op(guard->state, OP_EQ, GUARD_CIRC_STATE_COMPLETE); g = entry_guard_handle_get(guard->guard); @@ -2411,8 +2412,8 @@ test_entry_guard_select_for_circuit_highlevel_primary(void *arg) /* Call this one up; watch it get confirmed. */ update_approx_time(start+90); - r = entry_guard_succeeded(&guard); - tt_int_op(r, OP_EQ, 1); /* We can use it now. */ + u = entry_guard_succeeded(&guard); + tt_int_op(u, OP_EQ, GUARD_USABLE_NOW); tt_assert(guard); tt_int_op(guard->state, OP_EQ, GUARD_CIRC_STATE_COMPLETE); g = entry_guard_handle_get(guard->guard); @@ -2440,6 +2441,7 @@ test_entry_guard_select_for_circuit_highlevel_confirm_other(void *arg) circuit_guard_state_t *guard = NULL; int i, r; const node_t *node = NULL; + guard_usable_t u; /* Declare that we're on the internet. */ entry_guards_note_internet_connectivity(gs); @@ -2471,13 +2473,13 @@ test_entry_guard_select_for_circuit_highlevel_confirm_other(void *arg) tt_int_op(g->is_pending, OP_EQ, 1); (void)start; - r = entry_guard_succeeded(&guard); + u = entry_guard_succeeded(&guard); /* We're on the internet (by fiat), so this guard will get called "confirmed" * and should immediately become primary. * XXXX prop271 -- I don't like that behavior, but it's what is specified */ tt_int_op(guard->state, OP_EQ, GUARD_CIRC_STATE_COMPLETE); - tt_assert(r == 1); + tt_assert(u == GUARD_USABLE_NOW); tt_int_op(g->confirmed_idx, OP_EQ, 0); tt_int_op(g->is_primary, OP_EQ, 1); tt_int_op(g->is_pending, OP_EQ, 0); @@ -2503,6 +2505,7 @@ test_entry_guard_select_for_circuit_highlevel_primary_retry(void *arg) int i, r; const node_t *node = NULL; entry_guard_t *g; + guard_usable_t u; /* Declare that we're on the internet. */ entry_guards_note_internet_connectivity(gs); @@ -2540,8 +2543,8 @@ test_entry_guard_select_for_circuit_highlevel_primary_retry(void *arg) update_approx_time(start + 3600); /* Say that guard has succeeded! */ - r = entry_guard_succeeded(&guard); - tt_int_op(r, OP_EQ, 0); // can't use it yet. + u = entry_guard_succeeded(&guard); + tt_int_op(u, OP_EQ, GUARD_MAYBE_USABLE_LATER); tt_int_op(guard->state, OP_EQ, GUARD_CIRC_STATE_WAITING_FOR_BETTER_GUARD); g = entry_guard_handle_get(guard->guard); @@ -2556,8 +2559,8 @@ test_entry_guard_select_for_circuit_highlevel_primary_retry(void *arg) r = entry_guard_pick_for_circuit(gs, &node, &guard2); tt_assert(r == 0); tt_int_op(guard2->state, OP_EQ, GUARD_CIRC_STATE_USABLE_ON_COMPLETION); - r = entry_guard_succeeded(&guard2); - tt_assert(r == 1); + u = entry_guard_succeeded(&guard2); + tt_assert(u == GUARD_USABLE_NOW); tt_int_op(guard2->state, OP_EQ, GUARD_CIRC_STATE_COMPLETE); tt_assert(! entry_guards_all_primary_guards_are_down(gs)); @@ -2689,18 +2692,18 @@ upgrade_circuits_setup(const struct testcase_t *testcase) tor_assert(data->guard2_state->state == GUARD_CIRC_STATE_USABLE_IF_NO_BETTER_GUARD); - int r; + guard_usable_t r; update_approx_time(data->start + 32); if (make_circ1_succeed) { r = entry_guard_succeeded(&data->guard1_state); - tor_assert(r == 0); + tor_assert(r == GUARD_MAYBE_USABLE_LATER); tor_assert(data->guard1_state->state == GUARD_CIRC_STATE_WAITING_FOR_BETTER_GUARD); } update_approx_time(data->start + 33); if (make_circ2_succeed) { r = entry_guard_succeeded(&data->guard2_state); - tor_assert(r == 0); + tor_assert(r == GUARD_MAYBE_USABLE_LATER); tor_assert(data->guard2_state->state == GUARD_CIRC_STATE_WAITING_FOR_BETTER_GUARD); }