Unify router_pick_directory_server_impl and router_pick_trusteddirserver_impl

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.)

changed milestone to %Tor: unspecified

added component::core tor/tor easy milestone::Tor: unspecified points::2 priority::medium refactor reviewer::nickm severity::normal status::new triaged-out-20170124 type::enhancement labels

Another alternative is refactoring out common pieces.

I did some work on this in #17840 (moved).

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

Trac:
Reviewer: N/A to N/A

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

Trac:
Status: new to needs_review

Trac:
Keywords: easy refactor deleted, refactor, easy, review-group-12 added

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.

Trac:
Status: needs_review to needs_revision

Trac:
Keywords: review-group-12 deleted, N/A added

Trac:
Points: N/A to 2

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.

Trac:
Status: needs_revision to needs_review

Setting owner

Trac:
Status: needs_review to assigned
Owner: N/A to ahf

Trac:
Status: assigned to needs_review

Trac:
Keywords: N/A deleted, review-group-15 added

Trac:
Reviewer: N/A to nickm

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.

Trac:
Keywords: review-group-15 deleted, N/A added
Status: needs_review to new

Trac:
Milestone: Tor: 0.3.0.x-final to Tor: 0.3.1.x-final
Keywords: N/A deleted, triaged-out-20170124 added

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.

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.2.x-final

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: unspecified

Liberating some of the tickets that ahf had.

Trac:
Status: new to assigned
Owner: ahf to N/A
Cc: N/A to ahf

Change tickets that are assigned to nobody to "new".

Trac:
Status: assigned to new

changed time estimate to 16h

mentioned in issue #17903 (moved)

moved to tpo/core/tor#17847 (closed)

mentioned in issue tpo/core/tor#17903 (closed)