[tor-bugs] #18873 [Core Tor/Tor]: Refactor circuit_predict_and_launch_new()

[Tor Bug Tracker & Wiki]

#18873: Refactor circuit_predict_and_launch_new()
--------------------------+------------------------------------
 Reporter:  asn           |          Owner:
     Type:  defect        |         Status:  needs_revision
 Priority:  Low           |      Milestone:  Tor: 0.3.0.x-final
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:  refactoring   |  Actual Points:
Parent ID:                |         Points:
 Reviewer:  dgoulet       |        Sponsor:
--------------------------+------------------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 * Syntax nitpick. This has an extra line as it should be `MOCK_IMPL(int,`.
 See basically how we do it across the code.
 {{{
 +MOCK_IMPL(
 +int,
 +circuit_all_predicted_ports_handled, (time_t now, int *need_uptime,
 +                                      int *need_capacity))
 }}}

 * This comment confuses me as it doesn't appear to indicate what the
 function does or return. As I understand it, the function returns true iff
 the circuit matches some criteria that made it available for use.
 Documenting those criteria would be useful here. Also, what kind of
 circuit can be passed. Any kind? Or origin only or? as I see that it must
 be a GENERAL purpose for instance.
 {{{
 +/* Figure out how many circuits we have open that we can use. */
 +STATIC int
 +circuit_is_available_for_use(circuit_t *circ)
 }}}

 * I would `const circuit_t *circ` here and then use
 `CONST_TO_ORIGIN_CIRCUIT()`.
 {{{
 circuit_is_available_for_use(circuit_t *circ)
 }}}

 * Syntax nitpicK: all the `needs_*` have indentation issues. They should
 be aligned to each other like such:
 {{{
   return (!circuit_all_predicted_ports_handled(now, needs_uptime,
                                                needs_capacity) &&
           router_have_consensus_path() == CONSENSUS_PATH_EXIT);

 }}}

  In the case of this one above, it goes beyond 79 chars on the first line
 thus the newline. Note that ideally we like `&&` to be at the end of the
 line instead of beginning.

 * Further more on this function. This one is tricky as `needs_uptime` and
 `needs_capacity` are only set if we do need more Exit circuits so I would
 mention it in the comment as there is only one return value that sets
 them.
 {{{
 needs_exit_circuits(time_t now, int *needs_uptime, int *needs_capacity)
 }}}

 * I would honestly break this one down inside the function. This has quite
 the complicated conditions and the clearer the better as right now it's
 not that easy to get and also very easy to misunderstand:
 {{{
 needs_hs_client_circuits(...)
 }}}

 * Syntax:
 {{{
 #define CBT_MAX_UNUSED_OPEN_CIRCUITS (MAX_UNUSED_OPEN_CIRCUITS- \
     CBT_MIN_REMAINING_PREEMPTIVE_CIRCUITS)
 }}}

  to
  {{{
 #define CBT_MAX_UNUSED_OPEN_CIRCUITS (MAX_UNUSED_OPEN_CIRCUITS - \
 CBT_MIN_REMAINING_PREEMPTIVE_CIRCUITS)
  }}}

 * This condition has disappeared which is not good as
 `TO_ORIGIN_CIRCUIT()` in `circuit_is_available_for_use()` will explode if
 it's not an origin circuit.
 {{{
 -    if (!CIRCUIT_IS_ORIGIN(circ))
 -      continue;
 }}}

  and furthermore, this also could explode:
  {{{
 cpath_build_state_t *build_state = TO_ORIGIN_CIRCUIT(circ)->build_state;
  }}}

 That's it for now. Note to myself, next time use Gitlab for code review as
 it would have been easier for you, sorry about that :).

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18873#comment:14>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online