Distribute config.c functionality across more modules

The config.c module and the or_options_t type are a layering problem: They initialize many other modules, and provide what amounts to a set of global variables for the configuration settings.

Instead, we could use the subsystems design to give modules the ability to declare and listen to various configuration options of their own, without requiring all the option handling to be in a single place.

changed milestone to %Tor: unspecified

Trac:
Child Ticket(s): #32003 (moved), #31494 (moved), #31495 (moved), #31240 (moved), #31241 (moved), #31624 (moved), #31625 (moved), #31626 (moved), #31627 (moved), #31502 (moved), #31629 (moved), #30864 (moved), #30865 (moved), #30866 (moved), #31630 (moved), #31508 (moved), #31509 (moved), #31631 (moved), #31511 (moved), #31637 (moved), #31638 (moved), #32279 (moved), #32404 (moved), #31516 (moved), #32408 (moved), #31527 (moved), #32295 (moved), #31529 (moved), #31532 (moved), #30893 (moved), #32304 (moved), #32396 (moved), #32397 (moved), #31544 (moved), #32399 (moved), #30914 (moved), #32450 (moved), #32451 (moved), #32339 (moved), #32467 (moved), #30935 (moved), #32344 (moved), #31078 (moved), #32373 (moved), #32374 (moved), #31611 (moved)

added 043-deferred 044-deferred component::core tor/tor milestone::Tor: unspecified network-team-roadmap-2020Q1 owner::nickm points::23 priority::medium severity::normal status::assigned type::task labels

Trac:
Milestone: N/A to Tor: 0.4.1.x-final

Move various s31 refactoring tasks to 0.4.2

Trac:
Milestone: Tor: 0.4.1.x-final to Tor: 0.4.2.x-final

Trac:
Status: new to assigned
Owner: N/A to nickm

Here is my current plan for the refactoring.

High-level goals

The end goal is that every module should define, via a hook in the subsystem API, which variables it uses. The config_var_t structure is a natural way to note this, since it maps a variable to a struct field, but it has some problems:

• Its implementation is a huge pile of switch statements and special cases
• It combines multiple responsibilities in one blob of code
• It is difficult to extend
• It describes a field in a single structure, and is difficult to aggregate into our target implementation, wherein every module has its own configuration structure.
• Its current implementation depends on every type in the code that can be used as a configuration variable. Since routerset_t is one such (high level) type, and since we may expect other high-level types to exist in the future, we need a better way to make this system extensible.

Code dependencies

src/lib/conf will be code that is used by modules that need to define configurable items.

src/lib/confmgt/ will be generic code for manipulating configuration variables in objects. It will handle reading and writing from strings to struct members, and finding the appropriate struct members to read and write, and so on. It will only be used by higher level modules that need to do data-driven access to configuration. Configurable modules will just need lib/conf and lib/susbsys.

src/app/config/ will do the work of collecting all the configurable objects, reloading the configuration when appropriate, and notifying different subsystems when the configuration has changed.

Branches so far

In #30893 (moved), I tried to get to 100% test coverage on confparse.c, since that's the module that will be most affected by this refactoring. I missed a couple of spots, but those will be fixed as part of the #30864 (moved) branch to avoid conflicts. This branch is now merged.

The next branch, #30864 (moved), extracts the lowest level of introspection code from confparse.c: the code to manipulate typed values via a pointer and a type definition. This functionality is in the typed_var module, and it's not meant for direct use: it's a very low level api.

The next branch, #30914 (moved), wraps the typed_var API inside a struct_var API, which instead of referring to a pointer to typed data, refers to a typed member within a struct. It extracts this API from confparse.c, and cleans it up a lot. Once again, it makes confparse.c simpler.

The next branch, #30935 (moved), refactors config_var_t a bit, and moves it to lib/conf where it belongs. It is full of miscellaneous refactorings on the confparse.c code and its users. Notably, it removes all code in confparse.c or config.c that does "special-case" inspection of a variable's type, and it removes the need to update tiny little macro definitions all over the code. It also removes special-case handling of "magic" variable names like those that start with __ and ___. Finally -- and necessarily for my sanity -- it refactors our horrible old handling of TestingTorNetwork.

The next branch, #31240 (moved), teaches our confparse.c backend code to handle multiple sub-objects and sub-formats. There is still only one actual format of each type; multiplicity isn't there yet.

Next steps

My current work-in-progress branch, #31241 (moved), is refactoring our validation logic so that it isn't all crammed into one big "validate" function with too many arguments, and extending it so that configuration sub-objects and sub-formats can have their own verification functions.

The next major branch here will refactor config_fmt_t so that instead of describing a mapping from variable names to a single struct, it describes an extensible mapping from variable names to many structs. There will need to be a corresponding "group of structs" object that confparse.c and config.c use in place of their current or_options_t manipulation. That will be done in #30866 (moved).

Trac:
Cc: N/A to nickm, teor
Points: 15 to 23
Keywords: N/A deleted, network-team-roadmap-august added

Trac:
Cc: nickm, teor to nickm, teor, gaba

I am collecting a list of tasks for future branches as child tickets of this ticket. They all start with the prefix "config refactor:".

These tasks should not block reviews for any code that is already in commits or branches.

But they should block reviews for any new commits.

Hi Nick,

Here are my initial thoughts on the branches I reviewed. I want to think about them a bit more, and review and revise them before the sponsor 31 meeting on Thursday. I'd like a response at that meeting, rather than on this ticket. But I wanted to give you time to think through them before the meeting.

The goal of sponsor 31 is to improve code quality. But I worry that this pull request series has not improved our code quality (yet!), team processes, or coding habits.

I wanted more:

• collaboration on the design,
• summary and detailed design documentation,
• opportunities to write code for large refactors myself,
• function documentation and callback documentation,
• code comments around tricky code (or less tricky code),
• automated code transform scripts (sed or coccinelle),
• unit tests,
• tests for typical options used by Tor Browser, and possibly other significant uses of tor, (unit tests, chutney, or stem),
• working stem tests in CI (rather than stem CI having allow failures due to #29437 (moved)).

Edited to add:

• unit test code coverage,
  • even if coveralls says 100%, we seem to be missing important tests,

I wanted less:

• CI failures,
• regressions (#31495 (moved), #31527 (moved)),
• technical debt,
• missing changes files,
• pull request merge conflicts / target branch mistakes (which make reviews difficult),
• pull requests in the review backlog,
• dependencies between commits and pull requests (which make fixes difficult),
• exceptions to the normal review rules and review processes,
• pressure to complete reviews by a deadline.

Edited to add:

• DOCDOC documentation placeholders and missing documentation,
  • please git log -S DOCDOC master...branch (or similar) before submitting the PR,
• repeated requests to fix the same issues in pull request comments and ticket comments,
  • if we keep missing fixes, then:
    • our changes are too large,
      • there is too much code, or too many commits for us to review effectively using GitHub's tools,
    • there are too many comments on each pull request,
    • there are too many revisions of the same pull request,
    • we are overloaded with other tasks,
    • we are rushing to meet deadlines,
• missing unit tests,
• guessing which fixups correspond to which PR comments,

Overall it seems that this process has been difficult for all of us. I would like us to pause for a while, and work through the tensions that this refactor has brought up.

I have some ideas about the root causes here.

But I would like to hear your ideas at the sponsor 31 meeting on Thursday.

Edited to add:

Overall, there seems to be a weird stop-start-rush-mistakes-rework dynamic here. I don't think any of us are enjoying this process. Can we work out a pace that works for all of us? Can we work out processes that handle and resolve our tensions?

Replying to teor:

[snip]

But I would like to hear your ideas at the sponsor 31 meeting on Thursday.

10:24 <+GeKo> teor4: so, i tried to find out somethign about the s31 meeting you 
              mentioned in comment:8 in #29211
10:24 -zwiebelbot:#tor-project- tor#29211: Distribute config.c functionality across 
          more modules - [assigned] - https://bugs.torproject.org/29211
10:24 <+GeKo> are there notes somewhere?
10:24 <+GeKo> i did not see any log from meetbot
10:24 <+GeKo> or was that a sekrit meeting?
10:25 <+GeKo> i'd be interested in the results as it seems this refactoring is 
              breaking a bunch of things
10:25 <+GeKo> which hurts at least tor browser nightly users

[snip]

Replying to gk:

Replying to teor:

[snip]

But I would like to hear your ideas at the sponsor 31 meeting on Thursday. {{{ 10:24 <+GeKo> teor4: so, i tried to find out somethign about the s31 meeting you mentioned in comment:8 in #29211 (moved) 10:24 -zwiebelbot:#tor-project- tor#29211: Distribute config.c functionality across more modules - [assigned] - https://bugs.torproject.org/29211 10:24 <+GeKo> are there notes somewhere? 10:24 <+GeKo> i did not see any log from meetbot 10:24 <+GeKo> or was that a sekrit meeting? 10:25 <+GeKo> i'd be interested in the results as it seems this refactoring is breaking a bunch of things 10:25 <+GeKo> which hurts at least tor browser nightly users }}} [snip]

We rescheduled the meeting to 2300 UTC Tuesday 3 September.

Move some Sponsor31 config refactoring tasks into 0.4.3.

Trac:
Milestone: Tor: 0.4.2.x-final to Tor: 0.4.3.x-final

Trac:
Keywords: network-team-roadmap-august deleted, network-team-roadmap-2020Q1 added

All 0.4.3.x tickets without 043-must, 043-should, or 043-can are about to be deferred.

Trac:
Keywords: N/A deleted, 043-deferred added

Trac:
Milestone: Tor: 0.4.3.x-final to Tor: 0.4.4.x-final

No more sponsor 31. All this tickets remained open after sponsor 31 ended.

Trac:
Sponsor: Sponsor31-can to N/A

Bulk-remove tickets from 0.4.4. Add the 044-deferred label to them.

Trac:
Keywords: N/A deleted, 044-deferred added
Milestone: Tor: 0.4.4.x-final to Tor: unspecified

changed time estimate to 184h

mentioned in issue #29270 (moved)

mentioned in issue #30914 (moved)

mentioned in issue #30935 (moved)

mentioned in issue #31078 (moved)

mentioned in issue #31240 (moved)

mentioned in issue #31475 (moved)

mentioned in issue #31495 (moved)

mentioned in issue #31527 (moved)

mentioned in issue #32295 (moved)

mentioned in issue #32319 (moved)

mentioned in issue #30865 (moved)

mentioned in issue #30864 (moved)

mentioned in issue #30866 (moved)

mentioned in issue #30893 (moved)

mentioned in issue #31241 (moved)

mentioned in issue #31494 (moved)

mentioned in issue #31511 (moved)

mentioned in issue #31502 (moved)

mentioned in issue #31516 (moved)

mentioned in issue #31509 (moved)

mentioned in issue #31508 (moved)

mentioned in issue #31532 (moved)

mentioned in issue #31544 (moved)

mentioned in issue #31529 (moved)

mentioned in issue #31611 (moved)

mentioned in issue #31624 (moved)

mentioned in issue #31625 (moved)

mentioned in issue #31626 (moved)

mentioned in issue #31627 (moved)

mentioned in issue #31638 (moved)