Opened 2 months ago

Last modified 9 days ago

#31241 needs_revision task

Refactor config validation

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 Actual Points:
Parent ID: #29211 Points: 1.5
Reviewer: Sponsor: Sponsor31-can

Description

Here's the signature for the callback for validating a configuration object:

typedef int (*validate_fn_t)(void*,void*,void*,int,char**);

This is not a great interface. For example, takes three copies of the configuration object. One is the object it's validating; one is the previous value of the object (to see if we're making a bad transition); and the last one is the default values for the object (to see if we have changed anything we weren't allowed to change).

We should divide this callback into separate pieces, and fix everything that users options_validate() directly to instead call a config_validate() wrapper function.

Child Tickets

Change History (9)

comment:1 Changed 3 weeks ago by nickm

This has a branch (ticket31241) based on #31240. There is a merge-branch, ticket31241_merged_1.

https://github.com/torproject/tor/pull/1266 is the PR for the merge-branch, but note that many of the commits on it are part of #31240.

I will summarize the changes here below.

comment:2 Changed 3 weeks ago by nickm

Status: assignedneeds_review

comment:3 Changed 3 weeks ago by nickm

(Here is a PR with only the changes for #31241 : https://github.com/nmathewson/tor/pull/4 )

comment:4 Changed 3 weeks ago by nickm

Summary of design.

Previously, there were "validate" functions for config objects, most notably options_validate(). There was no generic way to call the "validate" function for a given config object -- you would just call it directly.

These functions had a horrible signature, since they were allowed to modify the configuration object, compare it to an old object, learn whether it came from setconf (!), and compare it to its defaults (!).

So the old process was:

  • Call options_verify.
  • Call options_transition_allowed.

Now the configuration verification process follows a more strict multi-stage process:

  • Call config_validate()
    • For each config_format_t, call config_validate_single():
      • Callback to modifications to the configuration object (early, pre-validation)
      • Callback to validation function (only sees current object, may not change it).
      • legacy validation callback (whatever we have not moved out of options_validate)
      • callback to check transition (can see current and previous object, may not change them)
      • modifications to the configuration object (final, late-validation)

comment:5 Changed 3 weeks ago by teor

Status: needs_reviewneeds_revision

Two unit tests failed on Windows:

options/validate__uname_for_server: [forking] 
6702  FAIL ../src/test/test_options.c:523: expected log to not contain entries  Captured logs:
6703    1. warn: "Path for GeoIPFile (<default>) is relative and will resolve to C:\\projects\\tor\\x86_64-w64-mingw32\\<default>. Is this what you wanted?\n"
6704    2. warn: "Path for GeoIPv6File (<default>) is relative and will resolve to C:\\projects\\tor\\x86_64-w64-mingw32\\<default>. Is this what you wanted?\n"
6705
6706  [validate__uname_for_server FAILED]
…
6720options/validate__paths_needed: [forking] 
6721  FAIL ../src/test/test_options.c:1460: expected log to not contain entries  Captured logs:
6722    1. warn: "Path for GeoIPFile (<default>) is relative and will resolve to C:\\projects\\tor\\x86_64-w64-mingw32\\<default>. Is this what you wanted?\n"
6723    2. warn: "Path for GeoIPv6File (<default>) is relative and will resolve to C:\\projects\\tor\\x86_64-w64-mingw32\\<default>. Is this what you wanted?\n"
6724
6725  [validate__paths_needed FAILED]
6726options/validate__max_client_circuits: [forking] OK

comment:6 Changed 3 weeks ago by teor

In general, I feel like the interfaces to callbacks have less documentation than regular functions.
Can you document all the callbacks (or callback type definitions) like we would a regular function?
Or am I looking in the wrong place?

comment:7 Changed 3 weeks ago by teor

I have finished my first review, this code needs revision.

If any changes need to be deferred, they should be made shortly after merging this branch.
There should not be any more branches until after this branch is merged, and the outstanding bugs are fixed.

comment:8 Changed 3 weeks ago by nickm

Based on review and backlog, I think I should postpone working on this branch till after #31240 is merged, and after we've caught up with other backlog. This will probably cause other conflicts as I re-create a new version of this branch, but so be it.

comment:9 Changed 9 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.