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

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

changed milestone to %Tor: 0.4.4.x-final

Trac:
Parent Ticket: #31851 (moved)

added 043-deferred actualpoints::0.5 component::core tor/tor milestone::Tor: 0.4.4.x-final network-team-roadmap-2020Q1 owner::nickm parent::31851 points::0.5 priority::medium reviewer::catalyst severity::normal status::needs-revision tor-design type::defect labels

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.

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

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to catalyst

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.

Trac:
Status: needs_review to merge_ready

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

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

Trac:
Status: merge_ready to needs_revision

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 (moved) 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

Trac:
Status: needs_revision to needs_review

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.

I also opened #33789 (moved), 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()

Mostly looks good!

Replying to 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? +1 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. It looks like init_keys_client() has a function comment. (It was previously in the header file.) I'm not sure I understand how init_keys_common() is a relay-only function, though. It looks like init_keys_client() calls it?

OK I looked a bit more closely and it's more confusing than I thought. I think it really could use better documentation in comments.

The init_keys_common() definition in router.c is not built on clients, because router.c is not built on clients. However, clientkey.c includes router.h, which has a macro to stub out init_keys_common(). Also oddly enough, these patches change init_keys_common() to remove the static qualifier, which seems unnecessary to change?

Or maybe rewrite things so that the naming isn't so confusing?

Trac:
Status: needs_review to needs_revision

changed time estimate to 4h

added 4h of time spent

mentioned in issue #31851 (moved)

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