Opened 3 years ago

Last modified 10 months ago

#21039 needs_revision defect

Refactor and simplify guard code of circuit_send_next_onion_skin()

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-guard, refactor, 034-triage-20180328, 034-included-20180401, 034-removed-20180502, 035-roadmap-subtask, 035-triaged-in-20180711
Cc: Actual Points:
Parent ID: #24986 Points: 0.3
Reviewer: asn Sponsor: SponsorV-can

Description

As part of prop271 (#19877), we added about 70 fresh lines of code to circuit_send_next_onion_skin() which is an already convoluted function.

Ideally we should abstract this new circuit-related guard code and put it in its own functions, to reduce complexity of circuit_send_next_onion_skin() and maybe even make it unittestable.

Child Tickets

Change History (26)

comment:1 Changed 3 years ago by ordex

A potential solution for this ticket has been implemented and uploaded on guthub in my "ticket21039" branch (5 patches in total): https://github.com/ordex/tor/commits/ticket21039 .
The branch is based on the current master.

On top of what the ticket is asking, I also tried to simplify and restyle circuit_send_next_onion_skin() as a whole.

In the last patch I implemented a new unittest for the circuit_send_next_onion_skin() function. However, being this my first experience with the tor unittest code, I am not sure I did it right :) feedback is welcome!

comment:2 Changed 3 years ago by ordex

Owner: set to ordex
Status: newaccepted

comment:3 Changed 3 years ago by ordex

Status: acceptedneeds_review

comment:4 Changed 3 years ago by nickm

Keywords: review-group-16 added

comment:5 Changed 3 years ago by asn

Reviewer: asn

comment:6 Changed 3 years ago by asn

Hello,

one thing that makes this patch quite hard to review in depth is that commit bfbf516 moves _and_ edits code in the same commit. It would have been much easier to review if the commit was splitteed into two: one commit that only moves code, and a second commit that edits the moved code to fit the new codebase. Let me know if you are willing to do this change to ease review.

For a more hands-on comment, this cast in the unittests is sketchy:

static guard_usable_t
mock_entry_guard_succeeded(circuit_guard_state_t **guard_state_p)
{
  return **(guard_usable_t **)guard_state_p;
}

Those two objects have nothing in common.

comment:7 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

comment:8 Changed 3 years ago by ordex

Owner: changed from ordex to asn
Status: needs_revisionassigned

A new set of changes has been pushed in the same branch at https://github.com/ordex/tor/commits/ticket21039 . The "move & restyle" patch has been split as much as possible hoping this could help the review.

About the last comment: although the two objects "have nothing in common", I used this hack-ish trick to make the mock function work as expected in the unit-test. In a nutshell, instead of having mock_entry_guard_succeeded() compute the return value, I force it to return the same value that was passed as argument.

This way, the unit-test can pass any value it wants and "force" the return of the wanted state.

This is helpful to test the behaviour of circuit_send_next_onion_skin() for every possible value returned by entry_guard_succeeded()

comment:9 Changed 3 years ago by ordex

Status: assignedneeds_review

comment:10 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

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.

comment:11 Changed 3 years ago by nickm

Keywords: review-group-16 removed

comment:12 Changed 3 years ago by asn

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

comment:13 Changed 2 years ago by nickm

Defer more needs_revision items to 0.3.3

comment:14 Changed 2 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

comment:15 Changed 23 months ago by asn

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

comment:16 Changed 21 months ago by nickm

Keywords: 034-triage-20180328 added

comment:17 Changed 21 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:18 Changed 21 months ago by nickm

Keywords: 034-included-20140401 added; 034-removed-20180328 removed
Parent ID: #20822#24986
Sponsor: SponsorV-can

comment:19 Changed 21 months ago by nickm

Keywords: 034-included-20180401 added; 034-included-20140401 removed

comment:20 Changed 20 months ago by asn

Keywords: 034-removed-20180502 added
Milestone: Tor: 0.3.4.x-finalTor: unspecified

Triaging this out of 034.

comment:21 Changed 18 months ago by jvsg

Hi Ordex, the link you shared is no longer valid.

comment:22 Changed 18 months ago by nickm

Keywords: 035-roadmap-subtask added
Milestone: Tor: unspecifiedTor: 0.3.5.x-final

comment:23 Changed 17 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:24 Changed 15 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Deferring various feature-y things to 0.3.6. If one of these is actually happening in 0.3.5, please let me know!

comment:25 Changed 13 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:26 Changed 10 months ago by asn

Milestone: Tor: 0.4.0.x-finalTor: unspecified
Note: See TracTickets for help on using tickets.