June 2013 - tor-commits - lists.torproject.org

[stem/master] Migrating to the mock library
by atagar＠torproject.org 13 Jun '13

13 Jun '13

commit 101cf0b71bab55d72a831eff0236dcef5edbff0f Merge: 65846e8 2b634a9 Author: Damian Johnson <atagar(a)torproject.org> Date: Thu Jun 13 09:48:30 2013 -0700 Migrating to the mock library Our homemade mocking framework has served us well, but over time it's taught me one very important lesson: writing a mocking framework is hard. On the surface it seems pretty simple: apply and revert a set of monkey patches. But how do you monkey patch class methods? What about alias imports like the os module? And god forbid you want to mock python's open() function. I'm finally taking a lesson from one of my coworkers and using a library for this. Python has several options but the most common is PyPI's mock module, which became part of the standard library in Python 3.3... http://www.voidspace.org.uk/python/mock/ run_tests.py | 17 ++ stem/prereq.py | 20 ++ test/integ/descriptor/reader.py | 5 - test/integ/descriptor/server_descriptor.py | 4 - test/integ/process.py | 20 +- test/integ/response/protocolinfo.py | 33 ++- test/integ/util/system.py | 138 +++++++----- test/mocking.py | 315 ---------------------------- test/settings.cfg | 1 + test/unit/connection/authentication.py | 47 ++--- test/unit/control/controller.py | 236 ++++++++++----------- test/unit/descriptor/export.py | 20 +- test/unit/descriptor/reader.py | 62 +++--- test/unit/descriptor/server_descriptor.py | 22 +- test/unit/response/control_message.py | 2 +- test/unit/response/events.py | 7 +- test/unit/response/protocolinfo.py | 30 +-- test/unit/tutorial.py | 120 ++++------- test/unit/util/proc.py | 104 +++++---- test/unit/util/system.py | 124 ++++++----- test/unit/version.py | 27 +-- test/util.py | 8 + 22 files changed, 541 insertions(+), 821 deletions(-)

1 0

[stem/master] Using mock for system integ tests
by atagar＠torproject.org 13 Jun '13

13 Jun '13

commit da365bc6ea5312d467c33f14942695c66c6f4e10 Author: Damian Johnson <atagar(a)torproject.org> Date: Wed Jun 12 21:39:25 2013 -0700 Using mock for system integ tests Unfortunately I'm not spotting a nice method via the mock module to get the original function. The patch() function has a get_original() method which looks promissing, but I didn't spot a method for accessing it. Oh well... --- test/integ/util/system.py | 138 +++++++++++++++++++++++++++++---------------- 1 file changed, 90 insertions(+), 48 deletions(-) diff --git a/test/integ/util/system.py b/test/integ/util/system.py index 4bfda1e..0694ffc 100644 --- a/test/integ/util/system.py +++ b/test/integ/util/system.py @@ -12,7 +12,7 @@ import stem.util.proc import stem.util.system import test.runner -from test import mocking +from mock import Mock, patch def filter_system_call(prefixes): @@ -21,10 +21,12 @@ def filter_system_call(prefixes): function if it matches one of the prefixes, and acts as a no-op otherwise. """ + original_call = stem.util.system.call + def _filter_system_call(command, default): for prefix in prefixes: if command.startswith(prefix): - real_call_function = mocking.get_real_function(stem.util.system.call) + real_call_function = original_call return real_call_function(command) return _filter_system_call @@ -59,9 +61,6 @@ class TestSystem(unittest.TestCase): results = [r for r in ps_results if r.endswith(" tor")] self.is_extra_tor_running = len(results) > 1 - def tearDown(self): - mocking.revert_mocking() - def test_is_available(self): """ Checks the stem.util.system.is_available function. @@ -118,10 +117,14 @@ class TestSystem(unittest.TestCase): return pgrep_prefix = stem.util.system.GET_PID_BY_NAME_PGREP % "" - mocking.mock(stem.util.system.call, filter_system_call([pgrep_prefix])) - tor_pid = test.runner.get_runner().get_pid() - self.assertEquals(tor_pid, stem.util.system.get_pid_by_name("tor")) + call_replacement = filter_system_call([pgrep_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + tor_pid = test.runner.get_runner().get_pid() + self.assertEquals(tor_pid, stem.util.system.get_pid_by_name("tor")) def test_get_pid_by_name_pidof(self): """ @@ -136,10 +139,14 @@ class TestSystem(unittest.TestCase): return pidof_prefix = stem.util.system.GET_PID_BY_NAME_PIDOF % "" - mocking.mock(stem.util.system.call, filter_system_call([pidof_prefix])) - tor_pid = test.runner.get_runner().get_pid() - self.assertEquals(tor_pid, stem.util.system.get_pid_by_name("tor")) + call_replacement = filter_system_call([pidof_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + tor_pid = test.runner.get_runner().get_pid() + self.assertEquals(tor_pid, stem.util.system.get_pid_by_name("tor")) def test_get_pid_by_name_ps_linux(self): """ @@ -157,10 +164,14 @@ class TestSystem(unittest.TestCase): return ps_prefix = stem.util.system.GET_PID_BY_NAME_PS_LINUX % "" - mocking.mock(stem.util.system.call, filter_system_call([ps_prefix])) - tor_pid = test.runner.get_runner().get_pid() - self.assertEquals(tor_pid, stem.util.system.get_pid_by_name("tor")) + call_replacement = filter_system_call([ps_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + tor_pid = test.runner.get_runner().get_pid() + self.assertEquals(tor_pid, stem.util.system.get_pid_by_name("tor")) def test_get_pid_by_name_ps_bsd(self): """ @@ -178,10 +189,14 @@ class TestSystem(unittest.TestCase): return ps_prefix = stem.util.system.GET_PID_BY_NAME_PS_BSD - mocking.mock(stem.util.system.call, filter_system_call([ps_prefix])) - tor_pid = test.runner.get_runner().get_pid() - self.assertEquals(tor_pid, stem.util.system.get_pid_by_name("tor")) + call_replacement = filter_system_call([ps_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + tor_pid = test.runner.get_runner().get_pid() + self.assertEquals(tor_pid, stem.util.system.get_pid_by_name("tor")) def test_get_pid_by_name_lsof(self): """ @@ -200,14 +215,17 @@ class TestSystem(unittest.TestCase): return lsof_prefix = stem.util.system.GET_PID_BY_NAME_LSOF % "" - mocking.mock(stem.util.system.call, filter_system_call([lsof_prefix])) - our_tor_pid = test.runner.get_runner().get_pid() + call_replacement = filter_system_call([lsof_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement - all_tor_pids = stem.util.system.get_pid_by_name("tor", multiple = True) + our_tor_pid = test.runner.get_runner().get_pid() + all_tor_pids = stem.util.system.get_pid_by_name("tor", multiple = True) - if len(all_tor_pids) == 1: - self.assertEquals(our_tor_pid, all_tor_pids[0]) + if len(all_tor_pids) == 1: + self.assertEquals(our_tor_pid, all_tor_pids[0]) def test_get_pid_by_port(self): """ @@ -257,10 +275,14 @@ class TestSystem(unittest.TestCase): return netstat_prefix = stem.util.system.GET_PID_BY_PORT_NETSTAT - mocking.mock(stem.util.system.call, filter_system_call([netstat_prefix])) - tor_pid = test.runner.get_runner().get_pid() - self.assertEquals(tor_pid, stem.util.system.get_pid_by_port(test.runner.CONTROL_PORT)) + call_replacement = filter_system_call([netstat_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + tor_pid = test.runner.get_runner().get_pid() + self.assertEquals(tor_pid, stem.util.system.get_pid_by_port(test.runner.CONTROL_PORT)) def test_get_pid_by_port_sockstat(self): """ @@ -282,10 +304,14 @@ class TestSystem(unittest.TestCase): return sockstat_prefix = stem.util.system.GET_PID_BY_PORT_SOCKSTAT % "" - mocking.mock(stem.util.system.call, filter_system_call([sockstat_prefix])) - tor_pid = test.runner.get_runner().get_pid() - self.assertEquals(tor_pid, stem.util.system.get_pid_by_port(test.runner.CONTROL_PORT)) + call_replacement = filter_system_call([sockstat_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + tor_pid = test.runner.get_runner().get_pid() + self.assertEquals(tor_pid, stem.util.system.get_pid_by_port(test.runner.CONTROL_PORT)) def test_get_pid_by_port_lsof(self): """ @@ -307,10 +333,14 @@ class TestSystem(unittest.TestCase): return lsof_prefix = stem.util.system.GET_PID_BY_PORT_LSOF - mocking.mock(stem.util.system.call, filter_system_call([lsof_prefix])) - tor_pid = test.runner.get_runner().get_pid() - self.assertEquals(tor_pid, stem.util.system.get_pid_by_port(test.runner.CONTROL_PORT)) + call_replacement = filter_system_call([lsof_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + tor_pid = test.runner.get_runner().get_pid() + self.assertEquals(tor_pid, stem.util.system.get_pid_by_port(test.runner.CONTROL_PORT)) def test_get_pid_by_open_file(self): """ @@ -358,10 +388,14 @@ class TestSystem(unittest.TestCase): # filter the call function to only allow this command pwdx_prefix = stem.util.system.GET_CWD_PWDX % "" - mocking.mock(stem.util.system.call, filter_system_call([pwdx_prefix])) - runner_pid, tor_cwd = runner.get_pid(), runner.get_tor_cwd() - self.assertEquals(tor_cwd, stem.util.system.get_cwd(runner_pid)) + call_replacement = filter_system_call([pwdx_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + runner_pid, tor_cwd = runner.get_pid(), runner.get_tor_cwd() + self.assertEquals(tor_cwd, stem.util.system.get_cwd(runner_pid)) def test_get_cwd_lsof(self): """ @@ -378,10 +412,14 @@ class TestSystem(unittest.TestCase): # filter the call function to only allow this command lsof_prefix = "lsof -a -p " - mocking.mock(stem.util.system.call, filter_system_call([lsof_prefix])) - runner_pid, tor_cwd = runner.get_pid(), runner.get_tor_cwd() - self.assertEquals(tor_cwd, stem.util.system.get_cwd(runner_pid)) + call_replacement = filter_system_call([lsof_prefix]) + + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + + runner_pid, tor_cwd = runner.get_pid(), runner.get_tor_cwd() + self.assertEquals(tor_cwd, stem.util.system.get_cwd(runner_pid)) def test_get_user_none(self): """ @@ -401,13 +439,17 @@ class TestSystem(unittest.TestCase): test.runner.skip(self, "(proc unavailable)") return - mocking.mock(stem.util.system.call, filter_system_call(['ps '])) + call_replacement = filter_system_call(['ps ']) - # we started our tor process so it should be running with the same user + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement - pid = test.runner.get_runner().get_pid() - self.assertTrue(getpass.getuser(), stem.util.system.get_user(pid)) + # we started our tor process so it should be running with the same user + + pid = test.runner.get_runner().get_pid() + self.assertTrue(getpass.getuser(), stem.util.system.get_user(pid)) + @patch('stem.util.proc.is_available', Mock(return_value = False)) def test_get_user_ps(self): """ Tests the get_user function with a ps response. @@ -417,8 +459,6 @@ class TestSystem(unittest.TestCase): test.runner.skip(self, "(ps unavailable)") return - mocking.mock(stem.util.proc.is_available, mocking.return_false()) - pid = test.runner.get_runner().get_pid() self.assertTrue(getpass.getuser(), stem.util.system.get_user(pid)) @@ -440,11 +480,15 @@ class TestSystem(unittest.TestCase): test.runner.skip(self, "(proc unavailable)") return - mocking.mock(stem.util.system.call, filter_system_call(['ps '])) + call_replacement = filter_system_call(['ps ']) - pid = test.runner.get_runner().get_pid() - self.assertTrue(stem.util.system.get_start_time(pid) >= 0) + with patch('stem.util.system.call') as call_mock: + call_mock.side_effect = call_replacement + pid = test.runner.get_runner().get_pid() + self.assertTrue(stem.util.system.get_start_time(pid) >= 0) + + @patch('stem.util.proc.is_available', Mock(return_value = False)) def test_get_start_time_ps(self): """ Tests the get_start_time function with a ps response. @@ -454,8 +498,6 @@ class TestSystem(unittest.TestCase): test.runner.skip(self, "(ps unavailable)") return - mocking.mock(stem.util.proc.is_available, mocking.return_false()) - pid = test.runner.get_runner().get_pid() self.assertTrue(stem.util.system.get_start_time(pid) >= 0)

1 0

[tor/master] Add support for offsetting the voting interval in order to bootstrap faster.
by nickm＠torproject.org 13 Jun '13

13 Jun '13

commit 4d54b9774d11f47c6a670ab9b380e027a524f5f9 Author: Linus Nordberg <linus(a)torproject.org> Date: Wed Jun 5 15:48:57 2013 +0200 Add support for offsetting the voting interval in order to bootstrap faster. A new option TestingV3AuthVotingStartOffset is added which offsets the starting time of the voting interval. This is possible only when TestingTorNetwork is set. This patch makes run_scheduled_events() check for new consensus downloads every second when TestingTorNetwork, instead of every minute. This should be fine, see #8532 for reasoning. This patch also brings MIN_VOTE_SECONDS and MIN_DIST_SECONDS down from 20 to 2 seconds, unconditionally. This makes sanity checking of misconfiguration slightly less sane. Addresses #8532. --- changes/bug8532 | 4 ++++ doc/tor.1.txt | 4 ++++ src/or/config.c | 10 ++++++++++ src/or/dirserv.c | 3 ++- src/or/dirvote.c | 21 ++++++++++++++------- src/or/dirvote.h | 8 +++++--- src/or/main.c | 15 ++++++++++++--- src/or/or.h | 4 ++++ 8 files changed, 55 insertions(+), 14 deletions(-) diff --git a/changes/bug8532 b/changes/bug8532 new file mode 100644 index 0000000..e9fd068 --- /dev/null +++ b/changes/bug8532 @@ -0,0 +1,4 @@ + o Minor features + - Add support for offsetting the voting interval in order to + bootstrap a network faster by adding configuration option + TestingV3AuthVotingStartOffset. Addresses #8532. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 72f75eb..37d374b 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -2032,6 +2032,10 @@ The following options are used for running a testing Tor network. the first consensus has been created. Changing this requires that **TestingTorNetwork** is set. (Default: 5 minutes) +**TestingV3AuthVotingStartOffset** __N__ **seconds**|**minutes**|**hours**:: + Directory authorities offset voting start time by this much. + Changing this requires that **TestingTorNetwork** is set. (Default: 0) + **TestingAuthDirTimeToLearnReachability** __N__ **minutes**|**hours**:: After starting as an authority, do not make claims about whether routers are Running until this much time has passed. Changing this requires diff --git a/src/or/config.c b/src/or/config.c index 2cdf5b2..f0cf312 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -413,6 +413,7 @@ static config_var_t option_vars_[] = { V(TestingV3AuthInitialVotingInterval, INTERVAL, "30 minutes"), V(TestingV3AuthInitialVoteDelay, INTERVAL, "5 minutes"), V(TestingV3AuthInitialDistDelay, INTERVAL, "5 minutes"), + V(TestingV3AuthVotingStartOffset, INTERVAL, "0"), V(V3AuthVotingInterval, INTERVAL, "1 hour"), V(V3AuthVoteDelay, INTERVAL, "5 minutes"), V(V3AuthDistDelay, INTERVAL, "5 minutes"), @@ -475,6 +476,7 @@ static const config_var_t testing_tor_network_defaults[] = { V(TestingV3AuthInitialVotingInterval, INTERVAL, "5 minutes"), V(TestingV3AuthInitialVoteDelay, INTERVAL, "20 seconds"), V(TestingV3AuthInitialDistDelay, INTERVAL, "20 seconds"), + V(TestingV3AuthVotingStartOffset, INTERVAL, "0"), V(TestingAuthDirTimeToLearnReachability, INTERVAL, "0 minutes"), V(TestingEstimatedDescriptorPropagationTime, INTERVAL, "0 minutes"), V(MinUptimeHidServDirectoryV2, INTERVAL, "0 minutes"), @@ -3224,6 +3226,7 @@ options_validate(or_options_t *old_options, or_options_t *options, CHECK_DEFAULT(TestingV3AuthInitialVotingInterval); CHECK_DEFAULT(TestingV3AuthInitialVoteDelay); CHECK_DEFAULT(TestingV3AuthInitialDistDelay); + CHECK_DEFAULT(TestingV3AuthVotingStartOffset); CHECK_DEFAULT(TestingAuthDirTimeToLearnReachability); CHECK_DEFAULT(TestingEstimatedDescriptorPropagationTime); CHECK_DEFAULT(TestingServerDownloadSchedule); @@ -3261,6 +3264,13 @@ options_validate(or_options_t *old_options, or_options_t *options, "must be less than half TestingV3AuthInitialVotingInterval"); } + if (options->TestingV3AuthVotingStartOffset > + MIN(options->TestingV3AuthInitialVotingInterval, + options->V3AuthVotingInterval)) { + REJECT("TestingV3AuthVotingStartOffset is higher than the voting " + "interval."); + } + if (options->TestingAuthDirTimeToLearnReachability < 0) { REJECT("TestingAuthDirTimeToLearnReachability must be non-negative."); } else if (options->TestingAuthDirTimeToLearnReachability > 2*60*60) { diff --git a/src/or/dirserv.c b/src/or/dirserv.c index c75f638..8bc55a8 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -3093,7 +3093,8 @@ dirserv_generate_networkstatus_vote_obj(crypto_pk_t *private_key, else last_consensus_interval = options->TestingV3AuthInitialVotingInterval; v3_out->valid_after = - dirvote_get_start_of_next_interval(now, (int)last_consensus_interval); + dirvote_get_start_of_next_interval(now, (int)last_consensus_interval, + options->TestingV3AuthVotingStartOffset); format_iso_time(tbuf, v3_out->valid_after); log_notice(LD_DIR,"Choosing valid-after time in vote as %s: " "consensus_set=%d, last_interval=%d", diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 0c386e6..4f0ab68 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -2523,12 +2523,13 @@ dirvote_get_preferred_voting_intervals(vote_timing_t *timing_out) timing_out->dist_delay = options->V3AuthDistDelay; } -/** Return the start of the next interval of size interval (in seconds) - * after now. Midnight always starts a fresh interval, and if the last - * interval of a day would be truncated to less than half its size, it is - * rolled into the previous interval. */ +/** Return the start of the next interval of size interval (in + * seconds) after now, plus offset. Midnight always + * starts a fresh interval, and if the last interval of a day would be + * truncated to less than half its size, it is rolled into the + * previous interval. */ time_t -dirvote_get_start_of_next_interval(time_t now, int interval) +dirvote_get_start_of_next_interval(time_t now, int interval, int offset) { struct tm tm; time_t midnight_today=0; @@ -2556,6 +2557,10 @@ dirvote_get_start_of_next_interval(time_t now, int interval) if (next + interval/2 > midnight_tomorrow) next = midnight_tomorrow; + next += offset; + if (next - interval > now) + next -= interval; + return next; } @@ -2619,8 +2624,10 @@ dirvote_recalculate_timing(const or_options_t *options, time_t now) vote_delay = dist_delay = interval / 4; start = voting_schedule.interval_starts = - dirvote_get_start_of_next_interval(now,interval); - end = dirvote_get_start_of_next_interval(start+1, interval); + dirvote_get_start_of_next_interval(now,interval, + options->TestingV3AuthVotingStartOffset); + end = dirvote_get_start_of_next_interval(start+1, interval, + options->TestingV3AuthVotingStartOffset); tor_assert(end > start); diff --git a/src/or/dirvote.h b/src/or/dirvote.h index b236452..a3e6cc0 100644 --- a/src/or/dirvote.h +++ b/src/or/dirvote.h @@ -13,9 +13,9 @@ #define TOR_DIRVOTE_H /** Lowest allowable value for VoteSeconds. */ -#define MIN_VOTE_SECONDS 20 +#define MIN_VOTE_SECONDS 2 /** Lowest allowable value for DistSeconds. */ -#define MIN_DIST_SECONDS 20 +#define MIN_DIST_SECONDS 2 /** Smallest allowable voting interval. */ #define MIN_VOTE_INTERVAL 300 @@ -86,7 +86,9 @@ authority_cert_t *authority_cert_dup(authority_cert_t *cert); /* vote scheduling */ void dirvote_get_preferred_voting_intervals(vote_timing_t *timing_out); -time_t dirvote_get_start_of_next_interval(time_t now, int interval); +time_t dirvote_get_start_of_next_interval(time_t now, + int interval, + int offset); void dirvote_recalculate_timing(const or_options_t *options, time_t now); void dirvote_act(const or_options_t *options, time_t now); diff --git a/src/or/main.c b/src/or/main.c index 90ffba3..9e4efef 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1153,6 +1153,7 @@ run_scheduled_events(time_t now) static time_t time_to_check_v3_certificate = 0; static time_t time_to_check_listeners = 0; static time_t time_to_check_descriptor = 0; + static time_t time_to_download_networkstatus = 0; static time_t time_to_shrink_memory = 0; static time_t time_to_try_getting_descriptors = 0; static time_t time_to_reset_descriptor_failures = 0; @@ -1442,10 +1443,18 @@ run_scheduled_events(time_t now) networkstatus_v2_list_clean(now); /* Remove dead routers. */ routerlist_remove_old_routers(); + } - /* Also, once per minute, check whether we want to download any - * networkstatus documents. - */ + /* 2c. Every minute (or every second if TestingTorNetwork), check + * whether we want to download any networkstatus documents. */ + +/* How often do we check whether we should download network status + * documents? */ +#define CHECK_NETWORKSTATUS_DOWNLOAD_INTERVAL (60) + + if (time_to_download_networkstatus < now && !options->DisableNetwork) { + time_to_download_networkstatus = now + + options->TestingTorNetwork ? 1 : CHECK_NETWORKSTATUS_DOWNLOAD_INTERVAL; update_networkstatus_downloads(now); } diff --git a/src/or/or.h b/src/or/or.h index daff6de..3e36eed 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3897,6 +3897,10 @@ typedef struct { * signatures. Only altered on testing networks.*/ int TestingV3AuthInitialDistDelay; + /** Offset in seconds added to the starting time for consensus + voting. Only altered on testing networks. */ + int TestingV3AuthVotingStartOffset; + /** If an authority has been around for less than this amount of time, it * does not believe its reachability information is accurate. Only * altered on testing networks. */

1 0

[tor/master] Merge remote-tracking branch 'linus/bug8532'
by nickm＠torproject.org 13 Jun '13

13 Jun '13

commit 45424b2ca1638d5b3d147bb518c092a41028e259 Merge: 4b781e2 c132427 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 13 12:42:49 2013 -0400 Merge remote-tracking branch 'linus/bug8532' changes/bug8532 | 4 ++++ doc/tor.1.txt | 4 ++++ src/or/config.c | 10 ++++++++++ src/or/dirserv.c | 3 ++- src/or/dirvote.c | 21 ++++++++++++++------- src/or/dirvote.h | 8 +++++--- src/or/main.c | 14 +++++++++++--- src/or/or.h | 4 ++++ 8 files changed, 54 insertions(+), 14 deletions(-)

1 0

[tor/master] Rename networkstatus_dl_interval() -> networkstatus_dl_check_interval()
by nickm＠torproject.org 13 Jun '13

13 Jun '13

commit 73ca1cf8b7502909108d5359b77c1c527aa3d272 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 13 12:44:46 2013 -0400 Rename networkstatus_dl_interval() -> networkstatus_dl_check_interval() --- src/or/main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index b2ff628..8e2845f 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1450,10 +1450,11 @@ run_scheduled_events(time_t now) /* How often do we check whether we should download network status * documents? */ -#define networkstatus_dl_interval(o) ((o)->TestingTorNetwork ? 1 : 60) +#define networkstatus_dl_check_interval(o) ((o)->TestingTorNetwork ? 1 : 60) if (time_to_download_networkstatus < now && !options->DisableNetwork) { - time_to_download_networkstatus = now + networkstatus_dl_interval(options); + time_to_download_networkstatus = + now + networkstatus_dl_check_interval(options); update_networkstatus_downloads(now); }

1 0

[tor/master] Hide consensus download interval, depending on TestingTorNetwork, in a macro.
by nickm＠torproject.org 13 Jun '13

13 Jun '13

commit c132427db4f207a32e4abaae0e070840e5e2b808 Author: Linus Nordberg <linus(a)torproject.org> Date: Wed Jun 5 15:48:57 2013 +0200 Hide consensus download interval, depending on TestingTorNetwork, in a macro. --- src/or/main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 9e4efef..b2ff628 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1450,11 +1450,10 @@ run_scheduled_events(time_t now) /* How often do we check whether we should download network status * documents? */ -#define CHECK_NETWORKSTATUS_DOWNLOAD_INTERVAL (60) +#define networkstatus_dl_interval(o) ((o)->TestingTorNetwork ? 1 : 60) if (time_to_download_networkstatus < now && !options->DisableNetwork) { - time_to_download_networkstatus = now + - options->TestingTorNetwork ? 1 : CHECK_NETWORKSTATUS_DOWNLOAD_INTERVAL; + time_to_download_networkstatus = now + networkstatus_dl_interval(options); update_networkstatus_downloads(now); }

1 0

[tor/master] Implement a placeholder mechanism in the channel, id->circ map
by nickm＠torproject.org 13 Jun '13

13 Jun '13

commit 967503c12c46f1c75209622ebddd15242e8af79a Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Mar 14 12:13:45 2013 -0400 Implement a placeholder mechanism in the channel,id->circ map We'll use this to help fix bug 7912, by providing a way to mark that a circuit ID can't get reused while a DESTROY is queued but not sent. --- src/or/circuitlist.c | 81 +++++++++++++++++++++++++++++++++++++++++++++----- src/or/circuitlist.h | 2 ++ 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 1903fbe..9bdf2ad 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -207,6 +207,61 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction, } } +/** Mark that circuit id id shouldn't be used on channel chan, + * even if there is no circuit on the channel. We use this to keep the + * circuit id from getting re-used while we have queued but not yet sent + * a destroy cell. */ +void +channel_mark_circid_unusable(channel_t *chan, circid_t id) +{ + chan_circid_circuit_map_t search; + chan_circid_circuit_map_t *ent; + + /* See if there's an entry there. That wouldn't be good. */ + memset(&search, 0, sizeof(search)); + search.chan = chan; + search.circ_id = id; + ent = HT_FIND(chan_circid_map, &chan_circid_map, &search); + + if (ent && ent->circuit) { + /* we have a problem. */ + log_warn(LD_BUG, "Tried to mark %u unusable on %p, but there was already " + "a circuit there.", (unsigned)id, chan); + } else if (ent) { + /* It's already marked. */ + } else { + ent = tor_malloc_zero(sizeof(chan_circid_circuit_map_t)); + ent->chan = chan; + ent->circ_id = id; + /* leave circuit at NULL */ + HT_INSERT(chan_circid_map, &chan_circid_map, ent); + } +} + +/** Mark that a circuit id id can be used again on chan. + * We use this to re-enable the circuit ID after we've sent a destroy cell. + */ +void +channel_mark_circid_usable(channel_t *chan, circid_t id) +{ + chan_circid_circuit_map_t search; + chan_circid_circuit_map_t *ent; + + /* See if there's an entry there. That wouldn't be good. */ + memset(&search, 0, sizeof(search)); + search.chan = chan; + search.circ_id = id; + ent = HT_REMOVE(chan_circid_map, &chan_circid_map, &search); + if (ent && ent->circuit) { + log_warn(LD_BUG, "Tried to mark %u usable on %p, but there was already " + "a circuit there.", (unsigned)id, chan); + return; + } + if (_last_circid_chan_ent == ent) + _last_circid_chan_ent = NULL; + tor_free(ent); +} + /** Set the p_conn field of a circuit circ, along * with the corresponding circuit ID, and add the circuit as appropriate * to the (chan,id)-\>circuit map. */ @@ -928,9 +983,13 @@ circuit_get_by_global_id(uint32_t id) * - circ-\>n_circ_id or circ-\>p_circ_id is equal to circ_id, and * - circ is attached to chan, either as p_chan or n_chan. * Return NULL if no such circuit exists. + * + * If found_entry_out is provided, set it to true if we have a + * placeholder entry for circid/chan, and leave it unset otherwise. */ static INLINE circuit_t * -circuit_get_by_circid_channel_impl(circid_t circ_id, channel_t *chan) +circuit_get_by_circid_channel_impl(circid_t circ_id, channel_t *chan, + int *found_entry_out) { chan_circid_circuit_map_t search; chan_circid_circuit_map_t *found; @@ -951,15 +1010,21 @@ circuit_get_by_circid_channel_impl(circid_t circ_id, channel_t *chan) " circ_id %u, channel ID " U64_FORMAT " (%p)", found->circuit, (unsigned)circ_id, U64_PRINTF_ARG(chan->global_identifier), chan); + if (found_entry_out) + *found_entry_out = 1; return found->circuit; } log_debug(LD_CIRC, - "circuit_get_by_circid_channel_impl() found nothing for" + "circuit_get_by_circid_channel_impl() found %s for" " circ_id %u, channel ID " U64_FORMAT " (%p)", + found ? "placeholder" : "nothing", (unsigned)circ_id, U64_PRINTF_ARG(chan->global_identifier), chan); + if (found_entry_out) + *found_entry_out = found ? 1 : 0; + return NULL; /* The rest of this checks for bugs. Disabled by default. */ /* We comment it out because coverity complains otherwise. @@ -993,7 +1058,7 @@ circuit_get_by_circid_channel_impl(circid_t circ_id, channel_t *chan) circuit_t * circuit_get_by_circid_channel(circid_t circ_id, channel_t *chan) { - circuit_t *circ = circuit_get_by_circid_channel_impl(circ_id, chan); + circuit_t *circ = circuit_get_by_circid_channel_impl(circ_id, chan, NULL); if (!circ || circ->marked_for_close) return NULL; else @@ -1009,7 +1074,7 @@ circuit_t * circuit_get_by_circid_channel_even_if_marked(circid_t circ_id, channel_t *chan) { - return circuit_get_by_circid_channel_impl(circ_id, chan); + return circuit_get_by_circid_channel_impl(circ_id, chan, NULL); } /** Return true iff the circuit ID circ_id is currently used by a @@ -1017,7 +1082,9 @@ circuit_get_by_circid_channel_even_if_marked(circid_t circ_id, int circuit_id_in_use_on_channel(circid_t circ_id, channel_t *chan) { - return circuit_get_by_circid_channel_impl(circ_id, chan) != NULL; + int found = 0; + return circuit_get_by_circid_channel_impl(circ_id, chan, &found) != NULL + || found; } /** Return the circuit that a given edge connection is using. */ @@ -1585,7 +1652,7 @@ assert_circuit_ok(const circuit_t *c) /* We use the _impl variant here to make sure we don't fail on marked * circuits, which would not be returned by the regular function. */ circuit_t *c2 = circuit_get_by_circid_channel_impl(c->n_circ_id, - c->n_chan); + c->n_chan, NULL); tor_assert(c == c2); } } @@ -1593,7 +1660,7 @@ assert_circuit_ok(const circuit_t *c) if (or_circ->p_circ_id) { /* ibid */ circuit_t *c2 = circuit_get_by_circid_channel_impl(or_circ->p_circ_id, - or_circ->p_chan); + or_circ->p_chan, NULL); tor_assert(c == c2); } } diff --git a/src/or/circuitlist.h b/src/or/circuitlist.h index d67f80b..434d2a8 100644 --- a/src/or/circuitlist.h +++ b/src/or/circuitlist.h @@ -23,6 +23,8 @@ void circuit_set_p_circid_chan(or_circuit_t *circ, circid_t id, channel_t *chan); void circuit_set_n_circid_chan(circuit_t *circ, circid_t id, channel_t *chan); +void channel_mark_circid_unusable(channel_t *chan, circid_t id); +void channel_mark_circid_usable(channel_t *chan, circid_t id); void circuit_set_state(circuit_t *circ, uint8_t state); void circuit_close_all_marked(void); int32_t circuit_initial_package_window(void);

1 0

[tor/master] Code to track on a circuit whether it has a "pending" delete cell
by nickm＠torproject.org 13 Jun '13

13 Jun '13

commit 801eea03ad71dafd31cc6bfa06fa5e421aa95cd6 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Mar 15 10:45:48 2013 -0400 Code to track on a circuit whether it has a "pending" delete cell This will be used in a fix for bug7912. --- src/or/circuitlist.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++--- src/or/circuitlist.h | 3 +++ src/or/or.h | 7 ++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 9bdf2ad..3e8caa8 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -262,18 +262,68 @@ channel_mark_circid_usable(channel_t *chan, circid_t id) tor_free(ent); } +/** Called to indicate that a DESTROY is pending on chan with + * circuit ID id, but hasn't been sent yet. */ +void +channel_note_destroy_pending(channel_t *chan, circid_t id) +{ + circuit_t *circ = circuit_get_by_circid_channel_even_if_marked(id,chan); + if (circ) { + if (circ->n_chan == chan && circ->n_circ_id == id) { + circ->n_delete_pending = 1; + } else { + or_circuit_t *orcirc = TO_OR_CIRCUIT(circ); + if (orcirc->p_chan == chan && orcirc->p_circ_id == id) { + circ->p_delete_pending = 1; + } + } + return; + } + channel_mark_circid_unusable(chan, id); +} + +/** Called to indicate that a DESTROY is no longer pending on chan with + * circuit ID id -- typically, because it has been sent. */ +void +channel_note_destroy_not_pending(channel_t *chan, circid_t id) +{ + circuit_t *circ = circuit_get_by_circid_channel_even_if_marked(id,chan); + if (circ) { + if (circ->n_chan == chan && circ->n_circ_id == id) { + circ->n_delete_pending = 0; + } else { + or_circuit_t *orcirc = TO_OR_CIRCUIT(circ); + if (orcirc->p_chan == chan && orcirc->p_circ_id == id) { + circ->p_delete_pending = 0; + } + } + /* XXXX this shouldn't happen; log a bug here. */ + return; + } + channel_mark_circid_usable(chan, id); +} + /** Set the p_conn field of a circuit circ, along * with the corresponding circuit ID, and add the circuit as appropriate * to the (chan,id)-\>circuit map. */ void -circuit_set_p_circid_chan(or_circuit_t *circ, circid_t id, +circuit_set_p_circid_chan(or_circuit_t *or_circ, circid_t id, channel_t *chan) { - circuit_set_circid_chan_helper(TO_CIRCUIT(circ), CELL_DIRECTION_IN, - id, chan); + circuit_t *circ = TO_CIRCUIT(or_circ); + channel_t *old_chan = or_circ->p_chan; + circid_t old_id = or_circ->p_circ_id; + + circuit_set_circid_chan_helper(circ, CELL_DIRECTION_IN, id, chan); if (chan) - tor_assert(bool_eq(circ->p_chan_cells.n, circ->next_active_on_p_chan)); + tor_assert(bool_eq(or_circ->p_chan_cells.n, + or_circ->next_active_on_p_chan)); + + if (circ->p_delete_pending && old_chan) { + channel_mark_circid_unusable(old_chan, old_id); + circ->p_delete_pending = 0; + } } /** Set the n_conn field of a circuit circ, along @@ -283,10 +333,18 @@ void circuit_set_n_circid_chan(circuit_t *circ, circid_t id, channel_t *chan) { + channel_t *old_chan = circ->n_chan; + circid_t old_id = circ->n_circ_id; + circuit_set_circid_chan_helper(circ, CELL_DIRECTION_OUT, id, chan); if (chan) tor_assert(bool_eq(circ->n_chan_cells.n, circ->next_active_on_n_chan)); + + if (circ->n_delete_pending && old_chan) { + channel_mark_circid_unusable(old_chan, old_id); + circ->n_delete_pending = 1; + } } /** Change the state of circ to state, adding it to or removing diff --git a/src/or/circuitlist.h b/src/or/circuitlist.h index 434d2a8..94887d5 100644 --- a/src/or/circuitlist.h +++ b/src/or/circuitlist.h @@ -64,5 +64,8 @@ void assert_cpath_layer_ok(const crypt_path_t *cp); void assert_circuit_ok(const circuit_t *c); void circuit_free_all(void); +void channel_note_destroy_pending(channel_t *chan, circid_t id); +void channel_note_destroy_not_pending(channel_t *chan, circid_t id); + #endif diff --git a/src/or/or.h b/src/or/or.h index 4e19140..529e455 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2779,6 +2779,13 @@ typedef struct circuit_t { * allowing n_streams to add any more cells. (OR circuit only.) */ unsigned int streams_blocked_on_p_chan : 1; + /** True iff we have queued a delete backwards on this circuit, but not put + * it on the output buffer. */ + unsigned int p_delete_pending : 1; + /** True iff we have queued a delete forwards on this circuit, but not put + * it on the output buffer. */ + unsigned int n_delete_pending : 1; + uint8_t state; /**< Current status of this circuit. */ uint8_t purpose; /**< Why are we creating this circuit? */

1 0

[tor/master] Implementation of a fix for bug 7912
by nickm＠torproject.org 13 Jun '13

13 Jun '13

commit 43d53e6d86acaf7555c31730a8230fa0cdf31306 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Mar 21 14:51:27 2013 -0400 Implementation of a fix for bug 7912 I added the code to pass a destroy cell to a queueing function rather than writing it immediately, and the code to remember that we shouldn't reuse the circuit id until the destroy is actually sent, and the code to release the circuit id once the destroy has been sent... and then I finished by hooking destroy_cell_queue into the rest of Tor. --- src/or/channel.c | 59 +++++++++++++++++++++++++++++++++------- src/or/circuitlist.c | 2 +- src/or/circuitmux.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++--- src/or/circuitmux.h | 7 ++++- src/or/relay.c | 19 +++++++++---- src/or/relay.h | 2 +- 6 files changed, 141 insertions(+), 21 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 4e9086f..e327bda 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -122,6 +122,8 @@ static cell_queue_entry_t * cell_queue_entry_new_fixed(cell_t *cell); static cell_queue_entry_t * cell_queue_entry_new_var(var_cell_t *var_cell); +static int is_destroy_cell(channel_t *chan, + const cell_queue_entry_t *q, circid_t *circid_out); /* Functions to maintain the digest map */ static void channel_add_to_digest_map(channel_t *chan); @@ -1685,6 +1687,13 @@ channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q) chan->timestamp_last_added_nonpadding = approx_time(); } + { + circid_t circ_id; + if (is_destroy_cell(chan, q, &circ_id)) { + channel_note_destroy_not_pending(chan, circ_id); + } + } + /* Can we send it right out? If so, try */ if (TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue) && chan->state == CHANNEL_STATE_OPEN) { @@ -2607,6 +2616,43 @@ channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) } } +/** DOCDOC */ +static int +is_destroy_cell(channel_t *chan, + const cell_queue_entry_t *q, circid_t *circid_out) +{ + *circid_out = 0; + switch (q->type) { + case CELL_QUEUE_FIXED: + if (q->u.fixed.cell->command == CELL_DESTROY) { + *circid_out = q->u.fixed.cell->circ_id; + return 1; + } + break; + case CELL_QUEUE_VAR: + if (q->u.var.var_cell->command == CELL_DESTROY) { + *circid_out = q->u.var.var_cell->circ_id; + return 1; + } + break; + case CELL_QUEUE_PACKED: + if (chan->wide_circ_ids) { + if (q->u.packed.packed_cell->body[4] == CELL_DESTROY) { + *circid_out = ntohl(get_uint32(q->u.packed.packed_cell->body)); + return 1; + } + } else { + if (q->u.packed.packed_cell->body[2] == CELL_DESTROY) { + *circid_out = ntohs(get_uint16(q->u.packed.packed_cell->body)); + return 1; + } + } + break; + } + return 0; +} + + /** * Send destroy cell on a channel * @@ -2618,25 +2664,20 @@ channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) int channel_send_destroy(circid_t circ_id, channel_t *chan, int reason) { - cell_t cell; - tor_assert(chan); /* Check to make sure we can send on this channel first */ if (!(chan->state == CHANNEL_STATE_CLOSING || chan->state == CHANNEL_STATE_CLOSED || - chan->state == CHANNEL_STATE_ERROR)) { - memset(&cell, 0, sizeof(cell_t)); - cell.circ_id = circ_id; - cell.command = CELL_DESTROY; - cell.payload[0] = (uint8_t) reason; + chan->state == CHANNEL_STATE_ERROR) && + chan->cmux) { + channel_note_destroy_pending(chan, circ_id); + circuitmux_append_destroy_cell(chan, chan->cmux, circ_id, reason); log_debug(LD_OR, "Sending destroy (circID %u) on channel %p " "(global ID " U64_FORMAT ")", (unsigned)circ_id, chan, U64_PRINTF_ARG(chan->global_identifier)); - - channel_write_cell(chan, &cell); } else { log_warn(LD_BUG, "Someone called channel_send_destroy() for circID %u " diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 3e8caa8..deb45b7 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -343,7 +343,7 @@ circuit_set_n_circid_chan(circuit_t *circ, circid_t id, if (circ->n_delete_pending && old_chan) { channel_mark_circid_unusable(old_chan, old_id); - circ->n_delete_pending = 1; + circ->n_delete_pending = 0; } } diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index 545cfd0..198e518 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -10,6 +10,7 @@ #include "channel.h" #include "circuitlist.h" #include "circuitmux.h" +#include "relay.h" /* * Private typedefs for circuitmux.c @@ -115,6 +116,18 @@ struct circuitmux_s { */ struct circuit_t *active_circuits_head, *active_circuits_tail; + /** List of queued destroy cells */ + cell_queue_t destroy_cell_queue; + /** Boolean: True iff the last cell to circuitmux_get_first_active_circuit + * returned the destroy queue. Used to force alternation between + * destroy/non-destroy cells. + * + * XXXX There is no reason to think that alternating is a particularly good + * approach -- it's just designed to prevent destroys from starving other + * cells completely. + */ + unsigned int last_cell_was_destroy : 1; + /* * Circuitmux policy; if this is non-NULL, it can override the built- * in round-robin active circuits behavior. This is how EWMA works in @@ -508,6 +521,8 @@ circuitmux_free(circuitmux_t *cmux) tor_free(cmux->chanid_circid_map); } + cell_queue_clear(&cmux->destroy_cell_queue); + tor_free(cmux); } @@ -816,7 +831,7 @@ circuitmux_num_cells(circuitmux_t *cmux) { tor_assert(cmux); - return cmux->n_cells; + return cmux->n_cells + cmux->destroy_cell_queue.n; } /** @@ -1368,16 +1383,36 @@ circuitmux_set_num_cells(circuitmux_t *cmux, circuit_t *circ, /** * Pick a circuit to send from, using the active circuits list or a * circuitmux policy if one is available. This is called from channel.c. + * + * If we would rather send a destroy cell, return NULL and set + * *destroy_queue_out to the destroy queue. + * + * If we have nothing to send, set *destroy_queue_out to NULL and + * return NULL. */ circuit_t * -circuitmux_get_first_active_circuit(circuitmux_t *cmux) +circuitmux_get_first_active_circuit(circuitmux_t *cmux, + cell_queue_t **destroy_queue_out) { circuit_t *circ = NULL; tor_assert(cmux); + tor_assert(destroy_queue_out); + + *destroy_queue_out = NULL; + + if (cmux->destroy_cell_queue.n && + (!cmux->last_cell_was_destroy || cmux->n_active_circuits == 0)) { + /* We have destroy cells to send, and either we just sent a relay cell, + * or we have no relay cells to send. */ - if (cmux->n_active_circuits > 0) { + /* XXXX We should let the cmux policy have some say in this eventually. */ + /* XXXX Alternating is not a terribly brilliant approach here. */ + *destroy_queue_out = &cmux->destroy_cell_queue; + + cmux->last_cell_was_destroy = 1; + } else if (cmux->n_active_circuits > 0) { /* We also must have a cell available for this to be the case */ tor_assert(cmux->n_cells > 0); /* Do we have a policy-provided circuit selector? */ @@ -1389,7 +1424,11 @@ circuitmux_get_first_active_circuit(circuitmux_t *cmux) tor_assert(cmux->active_circuits_head); circ = cmux->active_circuits_head; } - } else tor_assert(cmux->n_cells == 0); + cmux->last_cell_was_destroy = 0; + } else { + tor_assert(cmux->n_cells == 0); + tor_assert(cmux->destroy_cell_queue.n == 0); + } return circ; } @@ -1743,3 +1782,29 @@ circuitmux_assert_okay_pass_three(circuitmux_t *cmux) } } +/*DOCDOC */ +void +circuitmux_append_destroy_cell(channel_t *chan, + circuitmux_t *cmux, + circid_t circ_id, + uint8_t reason) +{ + cell_t cell; + memset(&cell, 0, sizeof(cell_t)); + cell.circ_id = circ_id; + cell.command = CELL_DESTROY; + cell.payload[0] = (uint8_t) reason; + + cell_queue_append_packed_copy(&cmux->destroy_cell_queue, &cell, + chan->wide_circ_ids, 0); + + /* XXXX Duplicate code from append_cell_to_circuit_queue */ + if (!channel_has_queued_writes(chan)) { + /* There is no data at all waiting to be sent on the outbuf. Add a + * cell, so that we can notice when it gets flushed, flushed_some can + * get called, and we can start putting more data onto the buffer then. + */ + log_debug(LD_GENERAL, "Primed a buffer."); + channel_flush_from_first_active_circuit(chan, 1); + } +} diff --git a/src/or/circuitmux.h b/src/or/circuitmux.h index 25644ff..da62196 100644 --- a/src/or/circuitmux.h +++ b/src/or/circuitmux.h @@ -120,7 +120,8 @@ unsigned int circuitmux_num_circuits(circuitmux_t *cmux); unsigned int circuitmux_num_active_circuits(circuitmux_t *cmux); /* Channel interface */ -circuit_t * circuitmux_get_first_active_circuit(circuitmux_t *cmux); +circuit_t * circuitmux_get_first_active_circuit(circuitmux_t *cmux, + cell_queue_t **destroy_queue_out); void circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, unsigned int n_cells); @@ -132,5 +133,9 @@ void circuitmux_clear_num_cells(circuitmux_t *cmux, circuit_t *circ); void circuitmux_set_num_cells(circuitmux_t *cmux, circuit_t *circ, unsigned int n_cells); +void circuitmux_append_destroy_cell(channel_t *chan, + circuitmux_t *cmux, circid_t circ_id, + uint8_t reason); + #endif /* TOR_CIRCUITMUX_H */ diff --git a/src/or/relay.c b/src/or/relay.c index 0ca3e56..ec86026 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2140,11 +2140,11 @@ cell_queue_append(cell_queue_t *queue, packed_cell_t *cell) /** Append a newly allocated copy of cell to the end of queue */ void cell_queue_append_packed_copy(cell_queue_t *queue, const cell_t *cell, - int wide_circ_ids) + int wide_circ_ids, int use_stats) { packed_cell_t *copy = packed_cell_copy(cell, wide_circ_ids); /* Remember the time when this cell was put in the queue. */ - if (get_options()->CellStatistics) { + if (get_options()->CellStatistics && use_stats) { struct timeval now; uint32_t added; insertion_time_queue_t *it_queue = queue->insertion_times; @@ -2339,7 +2339,7 @@ channel_flush_from_first_active_circuit(channel_t *chan, int max) { circuitmux_t *cmux = NULL; int n_flushed = 0; - cell_queue_t *queue; + cell_queue_t *queue, *destroy_queue=NULL; circuit_t *circ; or_circuit_t *or_circ; int streams_blocked; @@ -2352,7 +2352,16 @@ channel_flush_from_first_active_circuit(channel_t *chan, int max) /* Main loop: pick a circuit, send a cell, update the cmux */ while (n_flushed < max) { - circ = circuitmux_get_first_active_circuit(cmux); + circ = circuitmux_get_first_active_circuit(cmux, &destroy_queue); + if (destroy_queue) { + /* this code is duplicated from some of the logic below. Ugly! XXXX */ + tor_assert(destroy_queue->n > 0); + cell = cell_queue_pop(destroy_queue); + channel_write_packed_cell(chan, cell); + cell = NULL; + ++n_flushed; + continue; + } /* If it returns NULL, no cells left to send */ if (!circ) break; assert_cmux_ok_paranoid(chan); @@ -2474,7 +2483,7 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, streams_blocked = circ->streams_blocked_on_p_chan; } - cell_queue_append_packed_copy(queue, cell, chan->wide_circ_ids); + cell_queue_append_packed_copy(queue, cell, chan->wide_circ_ids, 1); /* If we have too many cells on the circuit, we should stop reading from * the edge streams for a while. */ diff --git a/src/or/relay.h b/src/or/relay.h index 7e59838..c4cb935 100644 --- a/src/or/relay.h +++ b/src/or/relay.h @@ -47,7 +47,7 @@ void packed_cell_free(packed_cell_t *cell); void cell_queue_clear(cell_queue_t *queue); void cell_queue_append(cell_queue_t *queue, packed_cell_t *cell); void cell_queue_append_packed_copy(cell_queue_t *queue, const cell_t *cell, - int wide_circ_ids); + int wide_circ_ids, int use_stats); void append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, cell_t *cell, cell_direction_t direction,

1 0

[tor/master] Add destroy balance tracking and logging to circuitmux
by nickm＠torproject.org 13 Jun '13

13 Jun '13

commit 16f9861b22751bc90666fe1836b8cf740630447a Author: Andrea Shepard <andrea(a)torproject.org> Date: Wed Jun 12 22:22:21 2013 -0700 Add destroy balance tracking and logging to circuitmux --- src/or/channel.c | 1 - src/or/circuitlist.c | 5 +++-- src/or/circuitmux.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/or/circuitmux.h | 1 + src/or/relay.c | 2 ++ 5 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index e327bda..33a3210 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -2652,7 +2652,6 @@ is_destroy_cell(channel_t *chan, return 0; } - /** * Send destroy cell on a channel * diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index deb45b7..e50ab60 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1717,8 +1717,9 @@ assert_circuit_ok(const circuit_t *c) if (or_circ && or_circ->p_chan) { if (or_circ->p_circ_id) { /* ibid */ - circuit_t *c2 = circuit_get_by_circid_channel_impl(or_circ->p_circ_id, - or_circ->p_chan, NULL); + circuit_t *c2 = + circuit_get_by_circid_channel_impl(or_circ->p_circ_id, + or_circ->p_chan, NULL); tor_assert(c == c2); } } diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index 198e518..a6256f8 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -127,6 +127,10 @@ struct circuitmux_s { * cells completely. */ unsigned int last_cell_was_destroy : 1; + /** Destroy counter: increment this when a destroy gets queued, decrement + * when we unqueue it, so we can test to make sure they don't starve. + */ + int64_t destroy_ctr; /* * Circuitmux policy; if this is non-NULL, it can override the built- @@ -206,6 +210,11 @@ static void circuitmux_assert_okay_pass_one(circuitmux_t *cmux); static void circuitmux_assert_okay_pass_two(circuitmux_t *cmux); static void circuitmux_assert_okay_pass_three(circuitmux_t *cmux); +/* Static global variables */ + +/** Count the destroy balance to debug destroy queue logic */ +static int64_t global_destroy_ctr = 0; + /* Function definitions */ /** @@ -521,6 +530,25 @@ circuitmux_free(circuitmux_t *cmux) tor_free(cmux->chanid_circid_map); } + /* + * We're throwing away some destroys; log the counter and + * adjust the global counter by the queue size. + */ + if (cmux->destroy_cell_queue.n > 0) { + cmux->destroy_ctr -= cmux->destroy_cell_queue.n; + global_destroy_ctr -= cmux->destroy_cell_queue.n; + log_debug(LD_CIRC, + "Freeing cmux at %p with %u queued destroys; the last cmux " + "destroy balance was %ld, global is %ld\n", + cmux, cmux->destroy_cell_queue.n, + cmux->destroy_ctr, global_destroy_ctr); + } else { + log_debug(LD_CIRC, + "Freeing cmux at %p with no queued destroys, the cmux destroy " + "balance was %ld, global is %ld\n", + cmux, cmux->destroy_ctr, global_destroy_ctr); + } + cell_queue_clear(&cmux->destroy_cell_queue); tor_free(cmux); @@ -1502,6 +1530,24 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, circuitmux_assert_okay_paranoid(cmux); } +/** + * Notify the circuitmux that a destroy was sent, so we can update + * the counter. + */ + +void +circuitmux_notify_xmit_destroy(circuitmux_t *cmux) +{ + tor_assert(cmux); + + --(cmux->destroy_ctr); + --(global_destroy_ctr); + log_debug(LD_CIRC, + "Cmux at %p sent a destroy, cmux counter is now %ld, " + "global counter is now %ld\n", + cmux, cmux->destroy_ctr, global_destroy_ctr); +} + /* * Circuitmux consistency checking assertions */ @@ -1798,6 +1844,14 @@ circuitmux_append_destroy_cell(channel_t *chan, cell_queue_append_packed_copy(&cmux->destroy_cell_queue, &cell, chan->wide_circ_ids, 0); + /* Destroy entering the queue, update counters */ + ++(cmux->destroy_ctr); + ++global_destroy_ctr; + log_debug(LD_CIRC, + "Cmux at %p queued a destroy for circ %u, " + "cmux counter is now %ld, global counter is now %ld\n", + cmux, circ_id, cmux->destroy_ctr, global_destroy_ctr); + /* XXXX Duplicate code from append_cell_to_circuit_queue */ if (!channel_has_queued_writes(chan)) { /* There is no data at all waiting to be sent on the outbuf. Add a @@ -1808,3 +1862,4 @@ circuitmux_append_destroy_cell(channel_t *chan, channel_flush_from_first_active_circuit(chan, 1); } } + diff --git a/src/or/circuitmux.h b/src/or/circuitmux.h index da62196..9ff29de 100644 --- a/src/or/circuitmux.h +++ b/src/or/circuitmux.h @@ -124,6 +124,7 @@ circuit_t * circuitmux_get_first_active_circuit(circuitmux_t *cmux, cell_queue_t **destroy_queue_out); void circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, unsigned int n_cells); +void circuitmux_notify_xmit_destroy(circuitmux_t *cmux); /* Circuit interface */ void circuitmux_attach_circuit(circuitmux_t *cmux, circuit_t *circ, diff --git a/src/or/relay.c b/src/or/relay.c index ec86026..46bfc44 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2358,6 +2358,8 @@ channel_flush_from_first_active_circuit(channel_t *chan, int max) tor_assert(destroy_queue->n > 0); cell = cell_queue_pop(destroy_queue); channel_write_packed_cell(chan, cell); + /* Update the cmux destroy counter */ + circuitmux_notify_xmit_destroy(cmux); cell = NULL; ++n_flushed; continue;

1 0