Re: [tor-bugs] #21039 [Core Tor/Tor]: Refactor and simplify guard code of circuit_send_next_onion_skin()

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#21039: Refactor and simplify guard code of circuit_send_next_onion_skin()
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  asn
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.1.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-guard, refactor, review-         |  Actual Points:
  group-16                                       |
Parent ID:  #20822                               |         Points:  0.3
 Reviewer:  asn                                  |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Hello ordex and thanks for refactoring your refactoring branch to make it
 easier for review.

 FWIW this is an '''initial''' review from me. As a disclaimer, I have
 almost zero knowledge about the whole onionskin part of the codebase, so
 this review is not easy at all for me. It took me over one hour reviewing
 about 60% of the branch, so I'm not done yet, and I have to move to other
 stuff.

 So here is an analysis per-commit:

 ----

 Commit `d7c9680` looks good to me.

 Commit `ea79dc2` is a bit weird. The basic logic is reasonable, but it
 doesn't just refactor it actually changes some code. For example, some
 asserts are not in the same place as before I think. Specifically
 `tor_assert(TO_CIRCUIT(circ)->state == CIRCUIT_STATE_BUILDING)` and
 `tor_assert(circ->cpath->state == CPATH_STATE_OPEN);` I think changed
 position. I'm not sure if anything else changed position; need to look
 deeper.

 Commit `fcc1497` looks good to me. The whitespace changes were a bit
 awkward to review, but then I was reminded about `git diff -w`.

 Commit `b35ea7f` is good in principle and it's the subject of this ticket.
 I have some comments on this:
   `new_state` should probably be `new_guard_state`.
   `circuit_update_state()` should probably be
 `circuit_update_guard_state()`.
   `usable` should probably be `usable_when`.
   `TO_CIRCUIT(circ)->purpose != CIRCUIT_PURPOSE_TESTING` was changed to
 `circ->base_.purpose != CIRCUIT_PURPOSE_TESTING` o.o

 Commit `c0df04d` is unittests and I have not had time to look at it. I
 actually have no idea what kind of unittests we would write for this
 function. I need more time to look at this.

 Commit `86df54f` is reasonable.

 ----

 All in all I think we are getting closer to getting this merged but I
 still have not looked at like 40% of the branch (mainly the unittests).

 That said, this refactoring is touching ''very'' ''very'' important parts
 of the codebase that I'm not familiar with and we should definitely not
 break. Also that part of the codebase is super hairy making the review
 even harder. That's why I'm taking this slow.

 Another approach to make this branch easier to digest would be to merge
 the guard changes first (`b35a7f`) in this ticket, and then make a new
 ticket about the other onionskin changes. Unfortunately, I can't cherry-
 pick just the guard changes due to the rest of the refactoring...

 Marking this as `needs_revision` for now, and I need to take another look
 soon; probs next week.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21039#comment:10>
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