Opened 3 months ago

Last modified 2 months ago

#32408 assigned defect

Handle options_act_reversible() in new config system.

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

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 (3)

comment:1 Changed 3 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 3 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 2 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?

Note: See TracTickets for help on using tickets.