Opened 6 months ago

Closed 5 months ago

Last modified 11 days ago

#25610 closed enhancement (fixed)

module: Modularized directory authority subsystem

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: modularization, 034-roadmap-subtask, tor-dirauth, 034-triage-20180328, 034-included-20180328
Cc: Actual Points:
Parent ID: #21814 Points:
Reviewer: nickm Sponsor: Sponsor8

Description

Make the directory subsystem a module that is enabled at configure time.

This is part of our initial modularization effort.

Child Tickets

TicketTypeStatusOwnerSummary
#25953taskclosedahfModule: Add Travis target for modularized directory authority subsystem
#25988defectcloseddgouletmodule: Post-merge tasks for dirauth modularization

Change History (18)

comment:1 Changed 6 months ago by nickm

Keywords: 034-triage-20180328 added

comment:2 Changed 6 months ago by nickm

Keywords: 034-included-20180328 added

comment:3 Changed 6 months ago by dgoulet

Status: assignedneeds_review

I've discussed with ahf an action plan. Nickm took a quick look as well and doesn't seem anything "bad" for now, I'm sure we'll find things as we go. But for now, to start somewhere, this is the plan:

Isolate the module into a sub-directory. This will require an important refactoring on many parts of the code...

src/or/dirauth/

Put in a _disable_ dirauth code build option within the build system.

$ ./configure --disable-dirauth-support

Which will introduce something like TOR_DIRAUTH_ENABLED = [0|1]

Compile the subdir conditionally according to TOR_DIRAUTH_ENABLED which is checked in the include.am file of src/or/

Everything that is an entry point function into the subsystem called from other part in Tor will be #ifdef/#else in public header file in ./src/or/dirauth.

One of the main goal of all this is to:

  1. Isolate Tor modules at the code level for better semantic and easier build system integration.
  1. Improve namespacing so every single part of a module can be identified across the code base. For instance, we do have functions that are in dirserv_* namespace but only used by tor clients.
  1. Avoid at all cost a pile of #ifdef in C files. Entry point functions will be static inline or NOP if module is disabled.

comment:4 Changed 6 months ago by dgoulet

Owner: changed from ahf to dgoulet
Status: needs_reviewaccepted

comment:5 Changed 6 months ago by dgoulet

On going development in ticket25610_034_01 for now. It is heavy rebasing, heavy WIP, might change, will change, but has the latest on dev and experimentation.

comment:6 Changed 6 months ago by chelseakomlo

Excited to see this development!

One note about heavy refactoring- having sufficient tests in place is really helpful before splitting apart large pieces of functionality and refactoring. As part of this plan, maybe add a goal to audit existing tests/write any more necessary tests ideally as one of the first steps to help when touching particularly old/convoluted parts of the code.

comment:7 in reply to:  6 Changed 6 months ago by dgoulet

Replying to chelseakomlo:

One note about heavy refactoring- having sufficient tests in place is really helpful before splitting apart large pieces of functionality and refactoring. As part of this plan, maybe add a goal to audit existing tests/write any more necessary tests ideally as one of the first steps to help when touching particularly old/convoluted parts of the code.

Indeed. That part of the code does have many tests but not in majority I think. Fortunately, for now we are just moving code around, no code behavior change. And ideally we keep it that way for the most part. I expect that it won't be true for some parts as we are still discovering those tricky them. For sure at that point, testing is needed.

However, being able to test the entry points to the dirauth subsystem when it is *disable* would be desirable so we make sure that tor still works properly. Again, we are lucky in some ways with this subsystem because most of the entry point are guarded with "Am I a dirauth" so this part we can hard assert that if the module is disabled, it can NOT send back true.

comment:8 Changed 5 months ago by dgoulet

Reviewer: ahf
Status: acceptedneeds_review

Alpha version: https://github.com/torproject/tor/pull/58

The gist is:

  • --disable-module-dirauth configure option is the option. The module is enabled by default.
  • Currently, when disabling the module, the tests do not build due to a linking error which is what the WIP commit is for but not succeeding for now.
  • All the commits are moving code around and very few do move code into *new* functions.
  • dirserv.c and directory.c have a lot of dirauth specific code but we should clean it up and extract the code to the module as a second step to this.

comment:9 Changed 5 months ago by dgoulet

PR: https://github.com/torproject/tor/pull/64
Branch: ticket25610_034_01

There is very little code "change" per-se that might affect the code behavior. The vast majority is moving code.

The dirauth module is enabled by default so to test this, you'll need to disable it and see how it goes with --disable-module-dirauth. The unit tests will *always* build the module.

This is a milestone, it is not all we can do. There are a couple things that need to happen to fully modularized dirauth and make it also better:

  1. We need to wrap the authdir_mode() and cie functions so they NEVER return true if the module is disabled as extra protection.
  1. The directory.c, dirserv.c and networkstatus.c files have a lot of things that are dirauth only. Basically, everything that touches vote document should be extracted into the dirauth module. This is quite a bit of work so we decided to do that as a second step if time permits.
  1. Write a documentation in doc/HACKING/ probably on how to proceed with a module. It is not that complicated but there are couple things to follow with the build system and code standards.
Last edited 5 months ago by dgoulet (previous) (diff)

comment:10 Changed 5 months ago by dgoulet

Reviewer: ahfnickm

comment:11 Changed 5 months ago by nickm

Status: needs_reviewneeds_revision

Okay, I've left a bunch of comments on the PR. +1 on the additional changes, but we should do those IMO only after revising and merging this.

comment:12 Changed 5 months ago by dgoulet

Status: needs_revisionneeds_review

Some back and forth with nickm on this. Back in needs_review and I believe soon in a merge ready state.

comment:13 Changed 5 months ago by nickm

Okay; we've talked a bit. Pre-merge tasks are:

  • Decide what we're renaming the "common" modules to.

Post-merge tasks are:

  • Splitting up dirvote_common.
  • Adding more #ifdefs (nickm)
  • Rename *common
  • Rename and move get_sig_by_alg (nickm)
  • Make dirvote_parse_sr_commits take a const ptr (nickm)

comment:14 Changed 5 months ago by dgoulet

Status: needs_reviewmerge_ready

Squashed branch with extra commit to fix a compiling issue that a previous fixup introduced: ticket25610_034_01-squashed.

Extra commit is d8509b450a1de815.

comment:15 Changed 5 months ago by nickm

merged!

comment:16 Changed 5 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

All child tickets have been resolved, closing.

comment:17 in reply to:  9 Changed 5 months ago by dgoulet

Replying to dgoulet:

For completion:

  1. We need to wrap the authdir_mode() and cie functions so they NEVER return true if the module is disabled as extra protection.

#25990

  1. The directory.c, dirserv.c and networkstatus.c files have a lot of things that are dirauth only. Basically, everything that touches vote document should be extracted into the dirauth module. This is quite a bit of work so we decided to do that as a second step if time permits.

#25989

  1. Write a documentation in doc/HACKING/ probably on how to proceed with a module. It is not that complicated but there are couple things to follow with the build system and code standards.

#25991

comment:18 Changed 11 days ago by nickm

Parent ID: #25494#21814
Note: See TracTickets for help on using tickets.