Opened 6 weeks ago

Last modified 3 days ago

#33414 needs_review defect

Split router.c, and disable it (mostly) when building without relay support.

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-design, network-team-roadmap-2020Q1, 043-deferred
Cc: teor Actual Points: 0.5
Parent ID: #31851 Points: 0.5
Reviewer: catalyst Sponsor:

Description

The last big piece of code in feature/relay that we want to turn off when --disable-module-relay is set is router.c.

Child Tickets

Change History (9)

comment:1 Changed 6 weeks ago by nickm

I have a branch called extract_router with a PR at https://github.com/torproject/tor/pull/1762`.

There are a few notes here:

  • The branch starts out by splitting the descriptor-generation code into its own file.
  • There is some code (currently in clientkey.[ch]) that I was not able to extract, since it is used when running as a client. I believe this code to be useless for clients, and I'll open another ticket to look at removing it or making it relay only. But that refactoring seems orthogonal to this one.
  • I've tried using some helper macros to generate the stubs in router.h. If we like those, we could extract them and use them to generate stubs in other files.

I'm traveling today, so I won't be able to check whether or not CI has passed. I'll put this into needs_review if it does.

comment:2 Changed 6 weeks ago by nickm

Status: assignedneeds_review

CI has passed; I've added a changes file and am putting this in needs_review.

comment:3 Changed 6 weeks ago by dgoulet

Reviewer: catalyst

comment:4 Changed 12 days ago by catalyst

Status: needs_reviewmerge_ready

Looks good! There's a typo that I commented on in the pull request.

I manually tested building with --disable-module-relay, but I didn't verify that the removed code was absent from the executable.

comment:5 Changed 12 days ago by teor

I think there's a missing keys lock when relay mode is disabled.
And there might be merge conflict with #32588.

Let's also open a ticket for the TODO dirauth change?

comment:6 Changed 12 days ago by teor

Status: merge_readyneeds_revision

comment:7 Changed 9 days ago by nickm

Status: needs_revisionneeds_review

I've answered the comment about the missing lock: that lock is only used to protect onion keys, which clients don't have.

I've also fixed the typo and rebased to master. I checked the conflicting diff in question (07ab22bfd3ce43a7c3f8a5e7dd8608ceb1de18b6) with --color-moved to make sure I didn't mess up any other code, but an extra look-see wouldn't hurt.

I opened #33739 for moving the dirauth-only keys to the dirauth module.

The new branch is extract_router_rebased with PR at https://github.com/torproject/tor/pull/1839

comment:8 Changed 3 days ago by teor

I've answered the comment about the missing lock: that lock is only used to protect onion keys, which clients don't have.

Thanks!

Can we add some comments that document this information?

init_keys_common() and init_keys_client() don't have function comments, and init_keys_common() is a really confusing name for a relay-only function.

Let's open another ticket?

Otherwise the changes look good here.

comment:9 Changed 3 days ago by teor

I also opened #33789, to move the address-picking functions to their own files.

nickm, catalyst, if you have an opinion on making these functions client-level functions, please comment on that ticket:

  • router_new_address_suggestion()
  • router_guess_address_from_dir_headers()
Note: See TracTickets for help on using tickets.