Opened 3 years ago

Last modified 19 months ago

#17847 new enhancement

Unify router_pick_directory_server_impl and router_pick_trusteddirserver_impl

Reported by: teor Owned by: ahf
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, refactor, triaged-out-20170124
Cc: Actual Points:
Parent ID: Points: 2
Reviewer: nickm Sponsor:

Description

router_pick_directory_server_impl and router_pick_trusteddirserver_impl are very similar.

I wonder if we could refactor them into a single function?
(Even if we have to use a macro to do it.)

Child Tickets

Change History (19)

comment:1 Changed 3 years ago by teor

Another alternative is refactoring out common pieces.

comment:2 Changed 3 years ago by teor

I did some work on this in #17840.

comment:3 Changed 2 years ago by ahf

I have a partial fix, that could use a review and an iteration or two, in the ahf/bugs/17847 branch on https://gitlab.com/ahf/tor.git

Direct link to the branch: https://gitlab.com/ahf/tor/commits/ahf/bugs/17847

comment:4 Changed 2 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.3.0.x-final

comment:5 Changed 2 years ago by nickm

Status: newneeds_review

comment:6 Changed 2 years ago by nickm

Keywords: review-group-12 added

comment:7 Changed 2 years ago by teor

Status: needs_reviewneeds_revision

Thanks for this patch!

Many of the macros are missing arguments - it's confusing to have a macro use or modify a variable that isn't listed in its arguments.

Almost all of SKIP_IF_ALREADY_DIR_FETCHING() could become a function, except for the body of the if statement, which is small enough to make it ok to duplicate.

Some code just seems to move around or be reformatted - it might be better to put whitespace-only changes in their own commit.

In general, these changes would be easier to review if each refactored macro or function was in its own commit.

comment:8 Changed 2 years ago by nickm

Keywords: review-group-12 removed

comment:9 Changed 2 years ago by nickm

Points: 2

comment:10 Changed 2 years ago by ahf

Sorry for the delay, this month has been quite intense so far.

I've made a revised version of the patch, which is now split into two patches, and can be found at:

https://gitlab.com/ahf/tor/commits/ahf/bugs/17847_2

I've removed all the whitespace changes, split the changes into two isolated commits, removed the SKIP_IF_ALREADY_DIR_FETCHING macro from the previous patch and instead generalised the function to support both of the variants that was needed before.

I've removed the SKIP_EXCLUDED macro from the previous revision since I do not believe it actually adds any simplification to the code, since the body is just an increment and a continue statement.

comment:11 Changed 2 years ago by nickm

Status: needs_revisionneeds_review

comment:12 Changed 2 years ago by nickm

Owner: set to ahf
Status: needs_reviewassigned

Setting owner

comment:13 Changed 2 years ago by nickm

Status: assignedneeds_review

comment:14 Changed 2 years ago by nickm

Keywords: review-group-15 added

comment:15 Changed 2 years ago by nickm

Reviewer: nickm

comment:16 Changed 2 years ago by nickm

Keywords: review-group-15 removed
Status: needs_reviewnew

Looks like a good start, and I'd be happy to take it as-is, but unless I'm missing something it doesn't yet actually unify the two functions yet? They're still two separate functions with mostly duplicated logic.

I've merged it, with a change to pass tor_addr_t by reference rather than on the stack. Calling this ticket "new" again.

comment:17 Changed 2 years ago by nickm

Keywords: triaged-out-20170124 added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

comment:18 Changed 23 months ago by nickm

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

0.3.1 feature freeze today: these don't seem like they will be all sorted out in time. Let's try to make progress in 0.3.2.

comment:19 Changed 19 months ago by dgoulet

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