Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket25762 034 04 #56

Closed
wants to merge 9 commits into from
Closed

Conversation

Labels
None yet
Projects
None yet
3 participants
@dgoulet-tor
Copy link
Contributor

@dgoulet-tor dgoulet-tor commented Apr 18, 2018

No description provided.

dgoulet-tor and others added 8 commits Apr 18, 2018
No behavior change, just to make it easier to find callbacks and for the sake
of our human brain to parse the list properly.

Signed-off-by: David Goulet <dgoulet@torproject.org>
In tor, we have a series of possible "roles" that the tor daemon can be
enabled for. They are:

  Client, Bridge, Relay, Authority (directory or bridge) and Onion service.

They can be combined sometimes. For instance, a Directory Authority is also a
Relay. This adds a "roles" field to a periodic event item object which is used
to know for which roles the event is for.

The next step is to enable the event only if the roles apply. No behavior
change at this commit.

Pars of #25762

Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
In case we transitionned to a new role in Tor, we need to launch and/or
destroy some periodic events.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Two helper functions to enable an event and disable an event which wraps the
launch and destroy of an event but takes care of the enabled flag.

They are also idempotent that is can be called multiple time on the same event
without effect if the event was already enabled or disabled.

Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
@coveralls
Copy link

@coveralls coveralls commented Apr 18, 2018

Pull Request Test Coverage Report for Build 552

  • 50 of 54 (92.59%) changed or added relevant lines in 4 files are covered.
  • 4625 unchanged lines in 19 files lost coverage.
  • Overall coverage increased (+0.06%) to 58.873%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/or/periodic.c 16 20 80.0%
Files with Coverage Reduction New Missed Lines %
src/or/or.h 1 75.0%
src/or/shared_random.c 3 82.53%
src/or/dircollate.c 4 96.55%
src/or/connection.h 4 0.0%
src/or/periodic.c 5 75.0%
src/common/token_bucket.c 6 84.29%
src/common/sandbox.c 11 19.05%
src/or/hs_common.c 24 83.36%
src/or/command.c 40 2.35%
src/or/entrynodes.c 77 85.37%
Totals Coverage Status
Change from base Build 502: 0.06%
Covered Lines: 40443
Relevant Lines: 68695

💛 - Coveralls

Copy link
Contributor

@nmathewson nmathewson left a comment

I've checked this over, and I confirm nothing is dropped.

/* Routers (bridge and relay) only. */
CALLBACK(check_descriptor, PERIODIC_EVENT_ROLE_ROUTER),
CALLBACK(check_ed_keys, PERIODIC_EVENT_ROLE_ROUTER),
CALLBACK(check_for_reachability_bw, PERIODIC_EVENT_ROLE_ROUTER),
Copy link
Contributor

@nmathewson nmathewson Apr 23, 2018

Do bridges evaluate their reachability and bw? If so, this is right.

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Apr 23, 2018

I believe so as server_mode() is used in that function thus bridges should do it.

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Apr 23, 2018

Seems fine here from IRC discussion.

CALLBACK(check_onion_keys_expiry_time, PERIODIC_EVENT_ROLE_ROUTER),
CALLBACK(clean_consdiffmgr, PERIODIC_EVENT_ROLE_ROUTER),
CALLBACK(expire_old_ciruits_serverside, PERIODIC_EVENT_ROLE_ROUTER),
CALLBACK(retry_dns, PERIODIC_EVENT_ROLE_ROUTER),
Copy link
Contributor

@nmathewson nmathewson Apr 23, 2018

Bridges shouldn't do this, should they?

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Apr 23, 2018

retry_dns() uses server_mode() so maybe we should use public_server_mode() there if you think it is no good?

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Apr 23, 2018

We'll open a new ticket here since retry_dns() only applies to Exit node thus the role should be Relay but also the function should use public_server_mode() or actually straight options->ExitRelay.

Copy link
Contributor

@nmathewson nmathewson Apr 23, 2018

I opened #25899 for this.

CALLBACK(check_ed_keys, PERIODIC_EVENT_ROLE_ROUTER),
CALLBACK(check_for_reachability_bw, PERIODIC_EVENT_ROLE_ROUTER),
CALLBACK(check_onion_keys_expiry_time, PERIODIC_EVENT_ROLE_ROUTER),
CALLBACK(clean_consdiffmgr, PERIODIC_EVENT_ROLE_ROUTER),
Copy link
Contributor

@nmathewson nmathewson Apr 23, 2018

I think this needs a separate "DirServer" role, unless it is going to be "Everyone". We have a bug recently where we assumed that all DirServers were relays of some kind, but that doesn't seem to be true.. See trac:23693

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Apr 23, 2018

Again as above, server_mode() is used meaning bridge go in that path. And because all relays are DirServer, it seems OK to assign the Router role where a bridge also will serve consensus to their clients thus need to clean the cache?

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Apr 23, 2018

Per IRC discussion, we need a DirServer role since DirCache 0 will make this callback pointless for a relay.

Copy link
Contributor

@nmathewson nmathewson Apr 23, 2018

I opened #25900 for this

@dgoulet-tor
Copy link
Contributor Author

@dgoulet-tor dgoulet-tor commented Apr 27, 2018

This has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment