Opened 8 months ago

Closed 3 months ago

#30866 closed enhancement (implemented)

Teach config.c to work with options configured in other modules

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points: 4
Parent ID: #29211 Points: 2
Reviewer: teor Sponsor: Sponsor31-can

Description

The last part of the groundwork for refactoring config.c will be to teach it about subsystems, so that it can find configuration options that it does not itself own. It needs to find them, parse them, validate them, pass them to the appropriate subsystems.

Child Tickets

TicketTypeStatusOwnerSummary
#32319defectclosednickmMacros and definitions for declaring per-module configuration
#32382defectclosednickmStop messing with HardwareAccel option
#32406defectclosedAdd ability to make an accelname required.

Change History (19)

comment:1 Changed 5 months ago by nickm

Type: defectenhancement

Mark a number of current 0.4.2.x "defects" as "enhancements."

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

This one isn't ready for serious review yet, but I'd like a design review. It incorporates an improved #32319. Branch is config_subsys, with PR at https://github.com/torproject/tor/pull/1488.

Once #32339 and #32344 are merged, I want to rebase this to simplify the WIP part, and make it actually compile.

comment:4 Changed 3 months ago by nickm

Reviewer: teor
Status: assignedneeds_review

Hi, teor! If you can look at this for design, great, but otherwise I'll return to it once #32339 and #32344 are in.

comment:5 Changed 3 months ago by nickm

I've pushed a new version of the branch; it incorporates an improved #32319, and is also based on #32339 and #32344. There's still more work to do.

comment:6 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

I like the design here. I am looking forward to being able to disable the dirauth and relay options in a generic way.

I did a review of the code on GitHub - I had some questions, and found some typos.

I expect that we will have some parseconf tests in this branch, or some more unit tests (or both). I opened #32374 to make it easier to test options_act*(). It would have been helpful in #32213, and would also help with testing this ticket.

I resolved a conflict between #32339 and #32344 when merging to master, so this branch needs to be rebased.

Can we squash the "Use the confdecl.h macros to declare a crypto_options_t struct" and WIP commits, so that the eventual code is cleaner? Or just delete the draft "Use the confdecl.h macros to declare a crypto_options_t struct" ?

comment:7 Changed 3 months ago by nickm

I've rebased as config_subsys_v2 and started making the requested changes. There's a PR at https://github.com/torproject/tor/pull/1501 so I can keep an eye on coverage and CI.

Last edited 3 months ago by nickm (previous) (diff)

comment:8 Changed 3 months ago by nickm

Status: needs_revisionneeds_review

comment:9 Changed 3 months ago by teor

This looks great, here's how we want to move forward:

  1. I'll do #32397
  2. nickm will add some tests for crypto NSS using test_parseconf.sh
  3. we can merge

Optionally, if we want to do a lot of state refactoring:

  1. I'll do #32396
  2. Someone will add some tests for mainloop state using test_parseconf.sh

But this is not a high priority at this time.

comment:10 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

(Please remember to add actual points!)

comment:11 Changed 3 months ago by nickm

I've run into a hitch with "nickm will add some tests for crypto NSS using test_parseconf.sh". There is no way to make crypto_openssl_late_init() fail. When it gets a nonexistent engine, it warns, but does not consider it an error. This behavior is probably reasonable, so I don't think we should change it.

I've added the test nonetheless, but I don't think the behavior will change for NSS: it is a success in both cases.

comment:12 Changed 3 months ago by nickm

Status: needs_revisionneeds_review

comment:13 Changed 3 months ago by nickm

Okay, I've done some more work here to make it better. I've done #32406 so that we can mark an accelerator as required, which lets us test failures in options_set().

And wouldn't you know it, there was a bug in options_set()! If options_act() failed while we did not have an event loop initialized, we would crash in tor_shutdown_event_loop_and_exit(). That's #32407: I've done a separate branch for that against 0.3.5.

I've squashed the fixups on the branch, rebased it on master, and added the #32407 fix. The new branch is config_subsys_v3. Its PR is https://github.com/torproject/tor/pull/1514.

I'm going to wait for CI to pass here, at least. I think that #32406 and #32407 are enough of a change that I should wait for review on them before I merge.

comment:14 Changed 3 months ago by nickm

Actual Points: 4

comment:15 Changed 3 months ago by nickm

FWIW, one of the CI tests is failing, but I think that isn't caused by this. It's caused by the fact that we reject DirCache 1 when running with the relay module disabled, but DirCache 1 is the default.
I've opened #32410 for this issue.

comment:16 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

I merged #32407 to 0.4.2 and later, and added a fix to #32410 for you to review.

I had some questions about the required engines feature, feel free to resolve them however you like, and then merge this ticket. You should probably squash and rebase first, there are some fixups and merge conflicts, and I merged #32407 to master.

comment:17 Changed 3 months ago by nickm

Okay, I've rebased once more as config_subsys_v4, and taken both of your suggestions on #32406. New PR is https://github.com/torproject/tor/pull/1516

comment:18 Changed 3 months ago by nickm

Status: needs_revisionneeds_review

Per your last review, I will merge this once CI has passed.

comment:19 Changed 3 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

CI is happy; merged to master!

Note: See TracTickets for help on using tickets.