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