Opened 16 months ago

Last modified 4 months ago

#29211 assigned task

Distribute config.c functionality across more modules

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

Description

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.

Child Tickets

TicketStatusOwnerSummaryComponent
#30864closednickmMove variable manipulation code out of confparse.cCore Tor/Tor
#30865closednickmMove option-listing, setting, validation code out of confparse.cCore Tor/Tor
#30866closednickmTeach config.c to work with options configured in other modulesCore Tor/Tor
#30893closednickmRefactor and improve test coverage in confparse.cCore Tor/Tor
#30914closednickmMove struct manipulation code out of confparse.cCore Tor/Tor
#30935closednickmMove variable definition code out of confparse.c, and refactorCore Tor/Tor
#31078assignednickmimprove docs for config var abstractionCore Tor/Tor
#31240closednickmMake confparse able to handle multiple config_format_t objects at onceCore Tor/Tor
#31241closednickmRefactor config validationCore Tor/Tor
#31494closednickmconfig refactoring: follow-ups from merged commitsCore Tor/Tor
#31495closednickmcannot configure bridgesCore Tor/Tor
#31502newRevise authority and fallback code so that they use the same "defaults" logic as other codeCore Tor/Tor
#31508closednickmconfig refactor: create simple summaries for each refactor stepCore Tor/Tor
#31509closedconfig refactor: make test-stem should passCore Tor/Tor
#31511closednickmconfig refactor: split mass change commits into automated and manual stepsCore Tor/Tor
#31516closednickmconfig refactor: every function table entry should be documented and unit testedCore Tor/Tor
#31527closednickmIn Tor Browser nightly, Tor fails to boostrap, hangs at 50%Core Tor/Tor
#31529closednickmconfig refactoring: fix redundant reset logicCore Tor/Tor
#31532closednickmUse ptrdiff_t for struct_member_t.offset, etcCore Tor/Tor
#31544assignednickmsr: Check why some sr_disk_state fields need to be clearedCore Tor/Tor
#31611closednickmWork out why chutney didn't fail due to #31495 cannot configure bridgesCore Tor/Tor
#31624closednickmExplain config_type_extended usage and purposeCore Tor/Tor
#31625closednickmconfig refactoring: fix hierarchy of configuration variable flagsCore Tor/Tor
#31626closednickmMove confparse.[ch] into lib/confmgtCore Tor/Tor
#31627closednickmFill in all missing documentation in config, confparse, etc.Core Tor/Tor
#31629closednickmRemove non-_ex typedvar.c functionCore Tor/Tor
#31630closednickmRemove unused assign/encode functions in structvar.Core Tor/Tor
#31631acceptednickmWrite a test for round-trip encode/decode operations on configuration objects.Core Tor/Tor
#31637closednickmMake sure we have test coverage for Option, +Option and /Option across defaults, torrc, command lineCore Tor/Tor
#31638assignednickmconfig: Always check for valid function arguments at the start of each functionCore Tor/Tor
#32003closednickmUnify handling of command-line option parsingCore Tor/Tor
#32279closednickmReplace config_suite_offset = -1 with a constantCore Tor/Tor
#32295closedteor"Skipping obsolete configuration option." doesn't say which oneCore Tor/Tor
#32304closednickmPreliminary movement and renaming for confmgt.Core Tor/Tor
#32339closedUse a table to drive options_check_transition and warn_about_relative_pathsCore Tor/Tor
#32344closednickmMake immutability into a config_var_t flagCore Tor/Tor
#32373newCreate a way to scan all configured file pathsCore Tor/Tor
#32374newCreate a test framework for options_act*()Core Tor/Tor
#32396newAdd state file support to test_parseconf.shCore Tor/Tor
#32397closedteorAdd lzma, nss, and zstd support to test_parseconf.shCore Tor/Tor
#32399closednickmFix "make test-stem" after merging #32339 and #32344Core Tor/Tor
#32404closednickmAdd a CFLG_OBSOLETE flag, and handle it at the confvar layerCore Tor/Tor
#32408assignednickmHandle options_act_reversible() in new config system.Core Tor/Tor
#32450closedteortest/parseconf: Document include behaviour, and add basic parsing testsCore Tor/Tor
#32451closedteortest/parseconf: Add support for required log messages on successCore Tor/Tor
#32467closednickmDocument tor's --dump-config command in the man pageCore Tor/Tor

Change History (15)

comment:1 Changed 14 months ago by nickm

Milestone: Tor: 0.4.1.x-final

comment:2 Changed 13 months ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final

Move various s31 refactoring tasks to 0.4.2

comment:3 Changed 12 months ago by gaba

Owner: set to nickm
Status: newassigned

comment:4 Changed 11 months ago by 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, 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 branch to avoid conflicts. This branch is now merged.

The next branch, #30864, 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, 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, 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, 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, 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.

Last edited 9 months ago by nickm (previous) (diff)

comment:5 Changed 10 months ago by gaba

Cc: nickm teor added
Keywords: network-team-roadmap-august added
Points: 1523

comment:6 Changed 9 months ago by gaba

Cc: gaba added

comment:7 Changed 9 months ago by teor

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.

comment:8 Changed 9 months ago by teor

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).

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, #31527),
  • 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?

Last edited 9 months ago by teor (previous) (diff)

comment:9 in reply to:  8 ; Changed 9 months ago by 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
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]

comment:10 in reply to:  9 Changed 9 months ago by teor

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
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.

comment:11 Changed 9 months ago by nickm

Milestone: Tor: 0.4.2.x-finalTor: 0.4.3.x-final

Move some Sponsor31 config refactoring tasks into 0.4.3.

comment:12 Changed 5 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added; network-team-roadmap-august removed

comment:13 Changed 4 months ago by nickm

Keywords: 043-deferred added

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

comment:14 Changed 4 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

comment:15 Changed 4 months ago by gaba

Sponsor: Sponsor31-can

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

Note: See TracTickets for help on using tickets.