Opened 8 months ago

Last modified 12 days ago

#29211 assigned task

Distribute config.c functionality across more modules

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-august
Cc: nickm, teor, gaba Actual Points:
Parent ID: Points: 23
Reviewer: Sponsor: Sponsor31-can

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
#30866assignednickmTeach 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
#31241needs_revisionnickmRefactor 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
#31508assignednickmconfig refactor: create simple summaries for each refactor stepCore Tor/Tor
#31509assignedconfig refactor: make test-stem should passCore Tor/Tor
#31511assignednickmconfig refactor: split mass change commits into automated and manual stepsCore Tor/Tor
#31516assignednickmconfig 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
#31611newWork 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

Change History (11)

comment:1 Changed 6 months ago by nickm

Milestone: Tor: 0.4.1.x-final

comment:2 Changed 4 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 3 months ago by gaba

Owner: set to nickm
Status: newassigned

comment:4 Changed 3 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 5 weeks ago by nickm (previous) (diff)

comment:5 Changed 2 months ago by gaba

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

comment:6 Changed 5 weeks ago by gaba

Cc: gaba added

comment:7 Changed 4 weeks 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 4 weeks 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 4 weeks ago by teor (previous) (diff)

comment:9 in reply to:  8 ; Changed 3 weeks 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 3 weeks 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 12 days 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.

Note: See TracTickets for help on using tickets.