[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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

#18873: Refactor circuit_predict_and_launch_new()
--------------------------+------------------------------------
 Reporter:  asn           |          Owner:
     Type:  defect        |         Status:  needs_review
 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 teor):

 This is an incomplete review:

 Replying to [comment:3 chelseakomlo]:
 > Second, I refactored circuit_predict_and_launch_new. This is mainly a
 refactor that improves readability/testability, but I did remove one piece
 of logic which- after writing unit tests to support this- seemed
 unnecessary.
 >
 https://github.com/chelseakomlo/tor_patches/commit/c06005d35d35339e61659cb5c5e43e8bcb60db7d
 >
 > The piece of logic that I removed is here:
 https://github.com/chelseakomlo/tor_patches/commit/c06005d35d35339e61659cb5c5e43e8bcb60db7d
 #diff-748f68b44784d7a77ebf42a2db48694eL1025
 >
 > As later, we exit if the circuit is not general purpose:
 >
 https://github.com/chelseakomlo/tor_patches/commit/c06005d35d35339e61659cb5c5e43e8bcb60db7d
 #diff-748f68b44784d7a77ebf42a2db48694eL1031
 >
 > Let me know if this isn't a good idea and I can add it back in.

 In general, if you're going to change behaviour in a refactor, please put
 it in a separate commit that describes the changes. That way, you separate
 changes that are meant to keep the same behaviour, from changes that are
 meant to modify behaviour.

 In this particular case, if you're convinced it's unnecessary, I would
 prefer it if we replaced this check with a BUG macro like so:
 {{{
 if (BUG(!CIRCUIT_IS_ORIGIN(circ)))
     continue;
 origin_circ = TO_ORIGIN_CIRCUIT(circ);
 if (origin_circ->unusable_for_new_conns)
     continue;
 ...
 }}}

 That way, we won't assert and crash in TO_ORIGIN_CIRCUIT() if the check
 turns out to be needed now, or if someone breaks that assumption in
 calling code in future.

 Also, I find `add_flags(1, 1, 1)` much less readable than `flags =
 (CIRCLAUNCH_NEED_CAPACITY | CIRCLAUNCH_NEED_UPTIME |
 CIRCLAUNCH_IS_INTERNAL)`.

 Can we either remove the add_flags part of the refactor, or keep the flag
 names?

 > Third, I extracted some magic numbers into named variables. I am not
 sure if the naming for these is the best, please let me know ideas for
 better names.
 >
 ​https://github.com/chelseakomlo/tor_patches/commit/9ea57c857b3c5939ca3394e017f8de4ea1ac9d08

 I don't think the name SUFFICIENT_UNUSED_OPEN_CIRCUITS captures the intent
 here:
 {{{
 -    if (num < MAX_UNUSED_OPEN_CIRCUITS-2
 +    if (num < MAX_UNUSED_OPEN_CIRCUITS-SUFFICIENT_UNUSED_OPEN_CIRCUITS
 }}}

 The comment above that explains why we subtract 2:
 {{{
   /* Finally, check to see if we still need more circuits to learn
    * a good build timeout. But if we're close to our max number we
    * want, don't do another -- we want to leave a few slots open so
    * we can still build circuits preemptively as needed.
 }}}

 I suggest instead:

 {{{
 #define CBT_MIN_REMAINING_PREEMPTIVE_CIRCUITS 2
 #define CBT_MAX_UNUSED_OPEN_CIRCUITS (MAX_UNUSED_OPEN_CIRCUITS-
 CBT_MIN_REMAINING_PREEMPTIVE_CIRCUITS)
 }}}

 And then putting the defines under the existing comment.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18873#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs