[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:
--------------------------+------------------------------------

Comment (by chelseakomlo):

 Ok, see changes at `git at github.com:chelseakomlo/tor_patches.git`, branch
 `circuituse`. Thanks for all the great feedback!



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


 Great, let me know if it is more clear now.


 > * 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(...)
 > }}}


 Ok, I pulled some of the logic out into separate variables. Let me know if
 this makes it more readable/easy to understand.


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

 Hey! Ok, here is my understanding:

 A circuit with purpose CIRCUIT_PURPOSE_C_GENERAL will always be an origin
 circuit, because:

 in or.h
 {{{
 #define CIRCUIT_IS_ORIGIN(c) (CIRCUIT_PURPOSE_IS_ORIGIN((c)->purpose))
 #define CIRCUIT_PURPOSE_IS_ORIGIN(p) ((p)>CIRCUIT_PURPOSE_OR_MAX_)
 #define CIRCUIT_PURPOSE_OR_MAX_ 4
 #define CIRCUIT_PURPOSE_C_GENERAL 5
 }}}

 so in circuit_is_available_for_use, circuituse.c

 `origin_circ = TO_ORIGIN_CIRCUIT(circ);`

 should not explode, because of the previous check:

 {{{
 if (circ->purpose != CIRCUIT_PURPOSE_C_GENERAL)
   return 0;/* only pay attention to general
               purpose circs */
 }}}

 So that is the reason we should be able to safely remove

 {{{
     if (!CIRCUIT_IS_ORIGIN(circ))
       continue;
 }}}

 Let me know if you have a different understanding of this.

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