Opened 3 years ago

Closed 15 months ago

#21349 closed defect (implemented)

Split up very long functions in entrynodes.c

Reported by: nickm Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: refactor, technical-debt, tor-client, 035-triaged-in-20180711
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:

Description

Some functions in entrynodes.c are now pretty huge, especially:

  • sampled_guards_update_from_consensus()
  • entry_guards_update_primary()
  • select_entry_guard_for_circuit()

It would be good for testing and maintenance to split them up.

Child Tickets

Change History (17)

comment:1 Changed 2 years ago by nickm

Keywords: refactor technical-debt tor-client added

comment:2 Changed 16 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:3 Changed 16 months ago by rl1987

Status: acceptedneeds_review

comment:4 Changed 16 months ago by asn

Reviewer: asn

comment:5 Changed 16 months ago by asn

Status: needs_reviewneeds_revision

Review done in the PR!

Extremely good work here by rl1987 and really good simplifications all around!

My review comments are mainly code-style and code-improvements, since I didn't manage to find a bug. The branch also needs to be rebased to latest master, since it doesn't merge cleanly currently because of all the various modularization improvements.

Thanks!

comment:6 Changed 16 months ago by rl1987

Status: needs_revisionneeds_review

comment:7 Changed 16 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

comment:8 Changed 16 months ago by asn

Status: needs_reviewneeds_revision

Thanks for the revisions! They look good to me, modulo the two minor things I pointed out in the new PR.

IMO, you should also squash the three latest commits (122ad11, 5453dd8, d3fcbcb) in the previous commits so that it's easier for nickm to review and merge.

I'm switching this to needs_revision for the above but we are almost done here!

comment:9 Changed 16 months ago by rl1987

Status: needs_revisionneeds_review

comment:10 Changed 16 months ago by asn

Status: needs_reviewmerge_ready

LGTM! :)

comment:11 Changed 16 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:12 Changed 16 months ago by nickm

I have a few questions on the ticket -- please let me know if you want to make the suggested changes, or if you think any of them aren't a good idea. Otherwise I will make the changes when I merge.

Thanks again!

comment:13 Changed 16 months ago by asn

Status: merge_readyneeds_revision

Marking as needs_revision while waiting for rl1987 to answer. rl1987, let me know if you want us to take over here. Thanks!

comment:14 Changed 15 months ago by rl1987

Status: needs_revisionneeds_review

Pushed some fixups to branch corresponding to !202 in response to comments from Nick. Please see if it's ready to merge now.

comment:15 Changed 15 months ago by rl1987

Squashed and rebased again: https://github.com/torproject/tor/pull/246

comment:16 Changed 15 months ago by asn

Status: needs_reviewmerge_ready

Fixes in ticket21349_3_clean LGTM and seem to follow Nick's review.

comment:17 Changed 15 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Awesome! Merged ticket21349_4 to master.

Note: See TracTickets for help on using tickets.