Opened 8 months ago

Last modified 7 months ago

#33414 needs_revision 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 (10)

comment:1 Changed 8 months 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 8 months 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 8 months ago by dgoulet

Reviewer: catalyst

comment:4 Changed 7 months 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 7 months 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 7 months ago by teor

Status: merge_readyneeds_revision

comment:7 Changed 7 months 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 7 months 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 7 months 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()

comment:10 in reply to:  8 Changed 7 months ago by catalyst

Status: needs_reviewneeds_revision

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?

Note: See TracTickets for help on using tickets.