Opened 4 weeks ago

Closed 2 weeks ago

#32427 closed enhancement (implemented)

Refactor options_act_reversible into manageable chunks

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

Description


Child Tickets

Change History (19)

comment:1 Changed 4 weeks ago by nickm

Branch is reversible with PR at https://github.com/torproject/tor/pull/1519.

This does not yet integrate add support for options_act_reversible() to the new system: instead it just splits the function into comprehensible parts.

Not yet ready for review; probably needs more tests.

comment:2 Changed 4 weeks ago by nickm

(I mean, you _could_ review it, but I bet it isn't ready for merge till the test coverage is higher.)

comment:3 Changed 4 weeks ago by nickm

Reviewer: teor
Status: assignedneeds_review

This still needs tests, but I would like your feedback on whether this is a good idea. Also, whether we should push ahead with this and #32408, or whether we should table them till we want to refactor something in that happens in this stage of configuration.

comment:4 Changed 4 weeks ago by teor

We have already refactored part of options_act() and options_act_reversible(), by splitting out the relay and dirauth config actions. I tried to keep the same order, but I did shift some small pieces of code around.

comment:5 Changed 4 weeks ago by teor

Status: needs_reviewneeds_revision

I think I just reviewed this in another ticket?

comment:6 Changed 4 weeks ago by teor

No, I got some ticket numbers confused. I reviewed this branch, but commented on another ticket.

comment:7 Changed 4 weeks ago by nickm

Teor's comment was:

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:8 Changed 4 weeks ago by nickm

Those renamings sound reasonable. I hope that in the long run they'll be irrelavant as this function becomes subsystem-driven (see parent), but for now they seem okay to me.

I've made the easier changes on the ticket, and added a ControlSocketsGroupWritable test. Here's my current plan:

  1. add tests for creating directories
  2. try unit tests for logs and sockets, assuming I can do that in no more than a couple of hours each.
  3. rename the functions.
  4. Back in needs review.

comment:9 Changed 2 weeks ago by nickm

I've done phase 1 (add tests for making directories) and solved #27992 while I was doing it, since it was in the way. Next I'm moving on to 2, which I expect will be somewhat harder.

comment:10 Changed 2 weeks ago by nickm

Had to rebase to avoid conflicts. New branch is reversible_2. PR is https://github.com/torproject/tor/pull/1552

comment:11 Changed 2 weeks ago by nickm

Okay, here's the status: I have tests for the log and directory code, but I wasn't able to figure out testing for the listener code: that is very hairy in tor.

I've hit a bug in the branch when I rebased: it looks like we are logging less than we were before, and some of the new test_parseconf log tests are complaining. I'll try to look at them tonight or tomorrow.

Teor, can you have a look at the new tests and see if you're okay without full testing on listeners? My next steps here are to get the current tests passing again, do some function renaming, and then needs_review again.

comment:12 Changed 2 weeks ago by teor

Keywords: network-team-roadmap-november added

Looks good!

I left a documentation question on the pull request.

I think the listening tests are fine, it's totally understandable that we can't test everything yet. We can do more refactoring and tests in future.

This branch seems to have stopped logging:

Your log may contain sensitive information

That seems like a log we might actually want to keep?

There's only one test that fails, and it's the "large" one.
Here's the full diagnostic output:

large_1: FAIL: Expected log success './expected_log':
Your log may contain sensitive information
Tor --verify-config said:
$ /Users/travis/build/torproject/tor/src/app/tor --verify-config -f ./torrc --defaults-torrc /var/folders/17/5mc7816d3mndxjqgplq6057w0000gn/T/tor_parseconf_tests.XXXXXX.vkKDe7kz/EMPTY 
Nov 19 22:16:24.511 [notice] Tor 0.4.3.0-alpha-dev (git-f5a08f6966d72594) running on Darwin with Libevent 2.1.11-stable, OpenSSL 1.0.2t, Zlib 1.2.11, Liblzma 5.2.4, and Libzstd 1.4.3.
Nov 19 22:16:24.513 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Nov 19 22:16:24.513 [notice] This version is not a stable Tor release. Expect more bugs than usual.
Nov 19 22:16:24.513 [notice] Read configuration file "/var/folders/17/5mc7816d3mndxjqgplq6057w0000gn/T/tor_parseconf_tests.XXXXXX.vkKDe7kz/EMPTY".
Nov 19 22:16:24.513 [notice] Read configuration file "/Users/travis/build/torproject/tor/src/test/conf_examples/large_1/./torrc".
Nov 19 22:16:24.517 [warn] The ClientPreferIPv6DirPort option is deprecated, and will most likely be removed in a future version of Tor. It has no effect on relays, and has had no effect on clients since 0.2.8. (If you think this is a mistake, please let us know!)
Nov 19 22:16:24.518 [warn] The ReachableDirAddresses option is deprecated, and will most likely be removed in a future version of Tor. It has no effect on relays, and has had no effect on clients since 0.2.8. (If you think this is a mistake, please let us know!)
Nov 19 22:16:24.520 [warn] You have asked to exclude certain relays from all positions in your circuits. Expect hidden services and other Tor features to be broken in unpredictable ways.
Nov 19 22:16:24.524 [notice] Not disabling debugger attaching for unprivileged users.

comment:13 Changed 2 weeks ago by teor

Reminder: please remember to run make test-stem on your changes, because it doesn't work in CI.

comment:14 Changed 2 weeks ago by nickm

I fixed the bug; make test-stem worked okay, but I hit a preexisting problem when I tried chutney. (#32555).

Assuming CI passes, I'm going to squash this branch down and continue.

comment:15 Changed 2 weeks ago by nickm

Okay, this is looking better. Tests are passing, chutney is passing, test-stem is passing.

I've added a bunch of comments to explain the ordering in options_act_reversible. I think that the actual correct division of responsibility is something like:

  • Pre-configuration
  • Post-configuration, pre-fork.
  • Post-fork, pre-setuid.
  • post-setuid.

Having the torrc, having daemonized and having setuid are the important barriers here.

comment:16 Changed 2 weeks ago by nickm

After a couple of false starts, I think that the right point at which to do the renaming is along with the refactoring. It's going to be a bit tricky, and I think we'll have to do it in stages, beyond the end of november, to get it right.

comment:17 Changed 2 weeks ago by nickm

Actual Points: .32.5
Status: needs_revisionneeds_review

Putting this in needs_review; what do you think about my suggestion above to do the renaming with the refactoring?

comment:18 Changed 2 weeks ago by teor

Status: needs_reviewmerge_ready

Seems fine to me.

The chutney CI job failed due to a network error, I restarted it.

The master merge in the PR doesn't have #32555. But the chutney job succeeded, probably because it's on gcc.

So I opened #32559 to switch our Travis CI chutney to clang. I'm testing to see if it fails now.

comment:19 Changed 2 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Okay, merged to master! Thanks for helping me through this one.

Note: See TracTickets for help on using tickets.