Opened 4 weeks ago

Closed 2 weeks ago

#31494 closed task (fixed)

config refactoring: follow-ups from merged commits

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
Cc: nickm, teor, gaba Actual Points: 0
Parent ID: #29211 Points:
Reviewer: Sponsor: Sponsor31-must

Description

I'm using this ticket to track follow-up items from merged commits that I can try to do once we have caught up more with the backlog of commits.

Child Tickets

Change History (16)

comment:1 Changed 4 weeks ago by nickm

From #30914, now a separate ticket #31624:

  • Make sure that we have a good explanation of CONFIG_TYPE_EXTENDED, how it is used, and how it relates to the type_def pointer.
Last edited 2 weeks ago by nickm (previous) (diff)

comment:2 Changed 4 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 4 weeks ago by nickm

From #30935, now tracked as #31625:

  • Convert the "contained" flag to something more like "NOCOPY NOCHECK NODUMP".
  • Make the "invisible" flag more like "NODUMP NOREAD".
  • Make sure that all low-level flags are orthogonal.
  • Make sure that "invisible" vs "hidden" is more clear.
Last edited 2 weeks ago by nickm (previous) (diff)

comment:4 Changed 4 weeks ago by teor

At the higher level, we could use is_derived() rather than is_contained().

comment:5 Changed 4 weeks ago by teor

Move confparse.ch into lib/confmgt.

comment:6 Changed 4 weeks ago by teor

From #30914, now a separate ticket #31508:

  • write a quick summary of each refactor before review
  • write a summary of the ideal state after all the refactors
    • since we're refactoring modules and types, we might need several ideal hierarchies

comment:7 Changed 4 weeks ago by teor

From #31495, also affects every other refactor ticket:

Stem should check a lot of useful option combinations.
But the stem CI job is current set to allow_failure, because of #29437.
So we should check that the stem CI job passes, or fails due to a timeout.

comment:8 in reply to:  7 Changed 4 weeks ago by teor

Replying to teor:

From #31495, also affects every other refactor ticket:

Stem should check a lot of useful option combinations.
But the stem CI job is current set to allow_failure, because of #29437.
So we should check that the stem CI job passes, or fails due to a timeout.

I created a separate ticket #31509.

comment:9 in reply to:  3 Changed 4 weeks ago by teor

Replying to nickm:

From #30935:

  • Convert the "contained" flag to something more like "NOCOPY NOCHECK NODUMP".
  • at the higher level, split "contained" into "derived" and "obsolete". At the lower level, give both "derived" and "obsolete" the flags "NOCOPY NOCHECK NODUMP".
    • even though these concepts may have the same flags right now, we don't want to lock them in to having the same flags in future. Because they are separate concepts that are quite different. For example, "derived" has a (derived) value, but "obsolete" has no value.
  • Make the "invisible" flag more like "NODUMP NOREAD".
  • Make sure that all low-level flags are orthogonal.
  • Make sure that "invisible" vs "hidden" is more clear.

comment:10 Changed 4 weeks ago by teor

Sponsor: Sponsor31-canSponsor31-must

We have merged these commits, so we must make these changes in sponsor 31.

comment:11 Changed 4 weeks ago by teor

https://trac.torproject.org/projects/tor/ticket/30935#comment:7

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

comment:12 in reply to:  11 Changed 2 weeks ago by nickm

I have made new tickets for two of the groups of fixes above: #31624 for CONFIG_TYPE_EXTENDED, and #31625 for the flag refactoring.

comment:13 in reply to:  11 Changed 2 weeks ago by nickm

Replying to teor:

https://trac.torproject.org/projects/tor/ticket/30935#comment:7

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

That isn't actually guaranteed to work: read+write only preserves the file in some cases. Trivially, it does not preserve comments or spacing. Less trivially, it does not preserve non-semantic option ordering. I'll try to think of another way to test this.

(Edited to add: I've opened #31631 for testing this.)

Last edited 2 weeks ago by nickm (previous) (diff)

comment:14 in reply to:  5 Changed 2 weeks ago by nickm

Replying to teor:

Move confparse.ch into lib/confmgt.

This is now #31626.

comment:15 Changed 2 weeks ago by nickm

Actual Points: 0
Status: acceptedneeds_review

I think that all of the issues described above now have their own ticket. If you agree, let's close this ticket.

comment:16 Changed 2 weeks ago by teor

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.