Opened 3 months ago

Closed 6 weeks ago

#31240 closed enhancement (fixed)

Make confparse able to handle multiple config_format_t objects at once

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-august, asn-merge dgoulet-merge
Cc: nickm, teor Actual Points: 3
Parent ID: #29211 Points: 3
Reviewer: teor Sponsor: Sponsor31-can

Description

Before we can move on to #30866 and have the responsibility for configuration formats spread across modules, we need to have the confparse code able to handle multiple configuration objects and formats as if they were one.

I'm going to do this by introducing a "configuration manager" type that knows about a bunch of config_format_t objects. The configuration objects will be collected under a single top-level configuration object. (This approach lets us keep backward compatibility with a lot of the existing options management code.)

Child Tickets

Change History (22)

comment:1 Changed 3 months ago by nickm

Branch is ticket31240 with PR at https://github.com/torproject/tor/pull/1190 . It is based on #30935.

comment:2 Changed 3 months ago by nickm

Actual Points: 3
Status: assignedneeds_review

comment:3 Changed 3 months ago by nickm

Sponsor: Sponsor31-can

comment:4 Changed 2 months ago by asn

Reviewer: teor

comment:5 Changed 7 weeks ago by nickm

I've made a fresh PR here as https://github.com/torproject/tor/pull/1260

Until #30935 is merged, the best way to review this may be at https://github.com/nmathewson/tor/pull/3 , which is a PR for only the stuff that changed since #30935.

comment:6 Changed 7 weeks ago by teor

Here's what I think this change is doing to the config type hierarchy:

Initial state:

  • config_format_t
    • (existing config types)

Final state:

  • configuration manager
    • multiple config_format_t
      • (existing config types)

comment:7 Changed 7 weeks ago by teor

On PR 1260, "make test-stem" passes all the tor tests, but fails due to bugs in stem that are unrelated to tor (#31510).

comment:8 Changed 7 weeks ago by teor

Status: needs_reviewneeds_revision

There are some missing tests, and a bunch of missing comments.

I also have a few questions about the design,

comment:9 Changed 7 weeks ago by nickm

I agree with your summary of the change above. More specifically, it will be like this:

Final state:

  • configuration manager
    • Toplevel config_format_t
      • (existing config types)
    • Multiple second-level config_format_t objects
      • (existing config types)

There will be a similar change eventually, but not in this branch, for the corresponding configuration objects. For example:

Current state:

  • or_options_t
    • (many option-holding variables)

Eventual state:

  • or_options_t
    • (variables for all options that have not been ported)
    • config_suite_t
      • mainloop_options_t (example)
        • (variables for mainloop options)
      • process_options_t (example)
        • (variables for process options)
      • ...

Note that every second-level config_format_t object in the config_manager corresponds to a member of the config_suite_t. They are linked by their indices.

comment:10 Changed 7 weeks ago by nickm

I've pushed the requested tests, plus a few more to increase the test coverage on confparse.c. I'll put this back in needs_review if CI approves.

comment:11 Changed 7 weeks ago by nickm

Status: needs_revisionneeds_review

comment:12 Changed 7 weeks ago by teor

Status: needs_reviewneeds_revision

This commit message typo blocks merging this branch:
https://github.com/nmathewson/tor/pull/3#discussion_r317549159

commit 94bfbf7859
Author: Nick Mathewson <nickm@torproject.org>
Date:   Tue Jul 23 10:41:57 2019 -0400

    Use special magic to enforce manager/object connection.
    
    Every time we finalize a config manager, we now generate a new magic
    number for it, so that we'll get an assertion failure if we ever two
    to use an object with a different configuration manager than the one
    that generated it.

I have tried to guess the commits that fix these issues, but I'd like you to confirm on the PR:

Please add runtime magic generation uniqueness tests:
https://github.com/nmathewson/tor/pull/3#discussion_r317550341

Please add subconfig tests:
https://github.com/nmathewson/tor/pull/3#discussion_r317552964

Do we need unit tests that have non-managed fields?
https://github.com/nmathewson/tor/pull/3#discussion_r317553659

There aren't any tests that use a clear callback. Please add some.
https://github.com/nmathewson/tor/pull/3#discussion_r317554429

This CI build hung:

https://ci.appveyor.com/project/nmathewson/tor/builds/26996434
Let's check both torproject and nmathewson CI again after the fixes listed above.

comment:13 Changed 7 weeks ago by nickm

I've answered the questions on the ticket; your conclusions were correct.

To fix the commit message, I've done a rebase of the orignal branch, plus the commits on the merge branch. This has created a new branch, called ticket31240v2, and a new merge branch, ticket31240v2_merged_2. The new PR is at https://github.com/torproject/tor/pull/1268 .

comment:14 Changed 7 weeks ago by nickm

Status: needs_revisionneeds_review

comment:15 Changed 7 weeks ago by teor

Status: needs_reviewneeds_revision

I think I'm still missing answers to these PR questions:

https://github.com/nmathewson/tor/pull/3#discussion_r317548828

https://github.com/nmathewson/tor/pull/3#discussion_r317549159

https://github.com/nmathewson/tor/pull/3#discussion_r318399721

Maybe there is too much code, too many commits, or too many comments in these PRs.
GitHub doesn't seem to want to display them all.

comment:16 Changed 7 weeks ago by nickm

Status: needs_revisionneeds_review

I've tried to answer those questions; in doing so, I've added another commit to ticket31240v2_merged_2.

comment:17 Changed 6 weeks ago by teor

Status: needs_reviewmerge_ready

Thanks for explaining how everything got fixed, let's merge this, then work on the other bugfix tickets under #29211.

comment:18 Changed 6 weeks ago by nickm

Keywords: asn-merge added

comment:19 Changed 6 weeks ago by dgoulet

Keywords: dgoulet-merge added

comment:20 Changed 6 weeks ago by dgoulet

Status: merge_readyneeds_revision

PR 1268 is not building for me (gcc-9):

src/test/test_confparse.c: In function ‘test_confparse_reset’:
src/test/test_confparse.c:613:21: error: passing argument 1 of ‘config_reset_line’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  613 |   config_reset_line(&test_fmt, tst, "routerset", 0);
      |                     ^~~~~~~~~
      |                     |
      |                     const config_format_t * {aka const struct config_format_t *}
In file included from src/test/test_confparse.c:17:
./src/app/config/confparse.h:152:51: note: expected ‘const config_mgr_t *’ {aka ‘const struct config_mgr_t *’} but argument is of type ‘const config_format_t *’ {aka ‘const struct config_format_t *’}
  152 | STATIC void config_reset_line(const config_mgr_t *mgr, void *options,

comment:21 Changed 6 weeks ago by nickm

Status: needs_revisionmerge_ready

It looks like the new test I added to test_confparse_reset() postdated the API changes here.

New branch: ticket31240v2_merged_2_merged. New PR: https://github.com/torproject/tor/pull/1290 .

comment:22 Changed 6 weeks ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged to master!

Note: See TracTickets for help on using tickets.