Opened 6 months ago

Closed 3 months ago

#31241 closed task (implemented)

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: 4
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

TicketStatusOwnerSummaryComponent
#31999closedDefault log file is handled inconsistentlyCore Tor/Tor
#32175closednickmtest_options.c uses some very wonky options objects.Core Tor/Tor
#32185closednickmRemove need for options_validate to use "defaults".Core Tor/Tor
#32187closednickmClean up options_validate() interfaceCore Tor/Tor

Change History (16)

comment:1 Changed 5 months 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 5 months ago by nickm

Status: assignedneeds_review

comment:3 Changed 5 months ago by nickm

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

comment:4 Changed 5 months 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 5 months 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 5 months 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 5 months 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 5 months 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 5 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:10 Changed 3 months ago by nickm

I've got a WIP branch called ticket31241_v2 with a PR at https://github.com/torproject/tor/pull/1458 . Not yet ready for review; it needs more testing and documentation first. The PR is just so I can see what coveralls says.

comment:11 Changed 3 months ago by nickm

Here is the design change for this branch; please ignore description earlier on the ticket.

Previously, options_validate() was a function that we called in several places, as was options_transition_allowed().

With this change, the above functions are now callbacks associated with a configuration format. Each format or subformat can have its own set of validation callbacks. Calling config_validate() on a top-level configuration object invokes all of the callbacks as appropriate.

There are 5 kinds of validation/normalization callbacks:

  • pre-normalization: allowed to change the object, before validation.
  • validation: inspects the object for correctness.
  • "legacy" validation: provides backward compatibility for functions like options_validate_cb that mix normalization, validation, and transition checking.
  • transition checking: compares the object to its old value ot see if the transition is permissible.
  • post-normalization: allowed to change the object, after validation.

I've uploaded a new ticket31241_v3 branch with PR at https://github.com/torproject/tor/pull/1473 . I'm checking that it passes with make test-stem. I think it can be needs_review once CI passes.

comment:12 Changed 3 months ago by nickm

Actual Points: 4

comment:13 Changed 3 months ago by teor

Actual Points: 4

CI passed, and I did a review.

I have a few minor questions, feel free to fix (or not fix) and then merge.

comment:14 Changed 3 months ago by teor

Actual Points: 4

comment:15 Changed 3 months ago by nickm

ok! I've added the magic number check. The comment would be misleading, so I'm not planning to restore it. I agree with #32279.

comment:16 Changed 3 months ago by nickm

Resolution: implemented
Status: needs_revisionclosed

Merged to master.

Note: See TracTickets for help on using tickets.