Opened 3 months ago

Closed 4 weeks ago

#30935 closed enhancement (fixed)

Move variable definition code out of confparse.c, and refactor

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

Description


Child Tickets

Change History (14)

comment:1 Changed 3 months ago by nickm

Actual Points: 0.5

See branch ticket30935 with PR at https://github.com/torproject/tor/pull/1126 .

It is based on #30914, so the earlier commits in this branch should be reviewed there.

comment:2 Changed 3 months ago by nickm

Status: assignedneeds_review

comment:3 Changed 3 months ago by dgoulet

Reviewer: catalyst

comment:4 Changed 3 months ago by nickm

Here's an overview of the commits in this branch.

Move config_var_t info conftypes.h

Pure code movement.

Move responsibility for config var macros

Refactors the macros that we have used to create config_var_t, and reduces code duplication.

Previously the macros used to define variables were redefined in every module that needed them; now they are derived from a common source. This makes them easier to change and refactor.

This commit also moves code only used for test builds into a new "conftesting.h" header.

Add a "flags" member to config_var_t

Add an (unused) flags member, and makes corresponding adjustments to the macros.

Extend macros to allow flag arguments.

Add a flags argument to the variable-definition macros.

Turn several properties of types or variables into flags.
Make "invisibility" and "undumpability" properties of variables.

These two commits are the bigger refactorings of this branch: they remove all the places in config or confparse that had previously tried to do manual inspection of a variable's type or names. To follow OO principles, we should turn these checks and inspections into properties of the types or variables themselves.

Refactor handling of TestingTorNetwork

The TestingTorNetwork option overrides the defaults for a bunch of other options. Its previous implementation was a nasty kludge that included a bunch of duplicated code, and involved overwriting the "initval" field of a bunch of config_var_t objects.

This new implementation should be much cleaner and easier to understand. It works by modifying the initially constructed or_options_t object whenever any code ask for an "initialized to defaults" object.

Make config_var and config_fmt const.

Thanks to the previous commit, we can now make more object const -- and should.

Move confparse.ch into lib/confmgt.

Code movement and header renaming. Once we moved the responsibility for handling routerset_t out of confparse.c, we no longer need confparse.c to be at a higher level than src/core. This patch lowers it.

(Edited to add: I've removed this patch from the branch for now, since I expect it will make fixup commits on this and previous patches very hard.)

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

comment:5 Changed 2 months ago by teor

Reviewer: catalystteor

comment:6 Changed 5 weeks ago by nickm

As before I've made a prettier, merged version of this branch at https://github.com/torproject/tor/pull/1254 . It is called `ticket30935_merged.

This PR is almost pure cleanup to make subsequent refactoring cleaner.

comment:7 Changed 5 weeks ago by teor

I'm going to add questions here as I think of them:

Do we have tests that read in whole torrc, state, and SR files, then write them out again, and make sure they are unchanged?

Last edited 5 weeks ago by teor (previous) (diff)

comment:8 Changed 5 weeks ago by teor

Status: needs_reviewneeds_revision

I think the flags need a redesign, so they are orthogonal, at least at the lowest level. And then the higher levels should use combinations of those flags. At the very least, there needs to be an overview comment describing how the flags interact at each level of abstraction, and between levels.

In particular, it's weird having obsolete / no dump / invisible options, and then flags that do similar things a higher level of abstraction.
See my review comment on the PR for details.

comment:9 Changed 4 weeks ago by nickm

Status: needs_revisionneeds_review

I've made the easier changes, and I agree with the harder ones but propose to queue them until more of this branch is merged, since I am worried about nontrivial conflicts later on. What do you think?

comment:10 Changed 4 weeks ago by teor

Status: needs_reviewmerge_ready

That's fine, but let's try to avoid this process issue in future.

comment:11 in reply to:  10 ; Changed 4 weeks ago by nickm

Keywords: dgoulet-merge asn-merge added

Replying to teor:

That's fine, but let's try to avoid this process issue in future.

Agreed; we need to not get so far ahead, and we need reviews to stay in step with coding. FWIW, I have stopped working on new code for #29211 while we catch up, so I hope that #31241 will be and #31242 will be the last queued-up branches.

comment:12 in reply to:  11 Changed 4 weeks ago by teor

Replying to nickm:

Replying to teor:

That's fine, but let's try to avoid this process issue in future.

Agreed; we need to not get so far ahead, and we need reviews to stay in step with coding. FWIW, I have stopped working on new code for #29211 while we catch up, so I hope that #31241 will be and #31242 will be the last queued-up branches.

Did you mean #31240 and #31241?

comment:13 Changed 4 weeks ago by nickm

I believe I did.

comment:14 Changed 4 weeks ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.