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

@@ -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;
       }
 

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;
   }
 }
 

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);

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);
   }