#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 `g...@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 _______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs