Opened 6 months ago

Last modified 3 weeks ago

#29211 assigned task

Distribute config.c functionality across more modules

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points: 15
Reviewer: Sponsor: Sponsor31-can


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

#30864merge_readynickmMove variable manipulation code out of confparse.cCore Tor/Tor
#30865assignednickmMove 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
#30914needs_reviewnickmMove struct manipulation code out of confparse.cCore Tor/Tor
#30935needs_reviewnickmMove variable definition code out of confparse.c, and refactorCore Tor/Tor
#31078newimprove docs for config var abstractionCore Tor/Tor

Change History (4)

comment:1 Changed 3 months ago by nickm

Milestone: Tor: 0.4.1.x-final

comment:2 Changed 2 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 5 weeks ago by gaba

Owner: set to nickm
Status: newassigned

comment:4 Changed 3 weeks 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.

Next steps

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.

Note: See TracTickets for help on using tickets.