Opened 12 months ago

Last modified 9 months ago

#32408 assigned defect

Handle options_act_reversible() in new config system.

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 043-deferred
Cc: teor Actual Points:
Parent ID: #29211 Points: 1.5
Reviewer: teor Sponsor:

Description

Our new configuration system needs to handle options_act_reversible too.

But on further examination, there are only two actual "reversible" pieces of options_act_reversible:

  • Adjusting the logs.
  • Adjusting the listeners.

The remainder of the function is all about handling "irreversible", immutable options that need to be handled before we can mess with the logs or listeners.

I think this means that we want to split this function into two pieces: one part that handles startup-only initialization, and one that handles truly reversible option changes.

I think that the callback for the startup-only initialization should be called options_set_init(), options_set_onece(), options_set_early(), or something like that.

The callbacks for truly reversible options should look something like this:

int act_reversible(const void *old_options, void *new_options, void **transaction_state_out);
int commit(const void *old_options, void *new_options, void *transaction_state);
int rollback(const void *old_options, void *rejected_options, void *transaction_state);

Child Tickets

TicketStatusOwnerSummaryComponent
#32427closednickmRefactor options_act_reversible into manageable chunksCore Tor/Tor
#32594closednickmFix CID 1455953: Resource leaks in #32427Core Tor/Tor

Change History (6)

comment:1 Changed 12 months ago by nickm

Status: newneeds_review

I'd like a general design review on the ideas and names in this ticket's description before I get too far ahead on this work.

comment:2 Changed 12 months ago by nickm

Owner: set to nickm
Status: needs_reviewassigned

Okay, this needs to be more tricky than I had thought at first. The problem is that for some of these "early irreversible" options, they need to happen before _any_ reversible options... and some need to happen _after_ opening ports but _before_ opening logs.

This needs a better pair of names than "early" + "reversible". Maybe "early" and "transaction" or something.

comment:3 Changed 12 months ago by teor

The design looks good.

How about naming things by what they *are*:

  • options_act_pre_port()
  • options_act_port()
  • options_act_pre_log()
  • options_act_log()
  • options_act...()

And then if someone wants to know the order, they can read the comments or the code?

comment:4 Changed 9 months ago by nickm

Keywords: 043-deferred added

All 0.4.3.x tickets without 043-must, 043-should, or 043-can are about to be deferred.

comment:5 Changed 9 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: unspecified

comment:6 Changed 9 months ago by gaba

Sponsor: Sponsor31-can

No more sponsor 31. All this tickets remained open after sponsor 31 ended.

Note: See TracTickets for help on using tickets.