commit 1c0a2335cd27ed4f4a61f99ce19b5ba08639eeff Author: Nick Mathewson <nickm@torproject.org> Date: Sun Mar 26 19:33:44 2017 +0200 Extract channel_do_open_actions() from non-open _change_state cases This reduces the size of the largest SCC in the callgraph by 30 functions, from 58 to 28. --- changes/ticket22608 | 6 ++++++ src/or/channel.c | 47 +++++++++++++++++++++++++++++++++-------------- src/or/channel.h | 1 + src/or/channeltls.c | 2 +- src/test/test_channel.c | 30 +++++++++++++++--------------- src/test/test_scheduler.c | 4 ++-- 6 files changed, 58 insertions(+), 32 deletions(-) diff --git a/changes/ticket22608 b/changes/ticket22608 new file mode 100644 index 0000000..5aa9db2 --- /dev/null +++ b/changes/ticket22608 @@ -0,0 +1,6 @@ + o Code simplification and refactoring: + - Extract the code for handling newly-open channels into a separate + function from the general code to handle channel state transitions. + This change simplifies our callgraph, reducing the size of the largest + strongly connected component by roughly a factor of two. + Closes ticket 22608 diff --git a/src/or/channel.c b/src/or/channel.c index e79fc07..68ebc4d 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -1972,8 +1972,8 @@ channel_write_var_cell(channel_t *chan, var_cell_t *var_cell) * are appropriate to the state transition in question. */ -void -channel_change_state(channel_t *chan, channel_state_t to_state) +static void +channel_change_state_(channel_t *chan, channel_state_t to_state) { channel_state_t from_state; unsigned char was_active, is_active; @@ -2092,18 +2092,8 @@ channel_change_state(channel_t *chan, channel_state_t to_state) estimated_total_queue_size += chan->bytes_in_queue; } - /* Tell circuits if we opened and stuff */ - if (to_state == CHANNEL_STATE_OPEN) { - channel_do_open_actions(chan); - chan->has_been_open = 1; - - /* Check for queued cells to process */ - if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)) - channel_process_cells(chan); - if (! TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue)) - channel_flush_cells(chan); - } else if (to_state == CHANNEL_STATE_CLOSED || - to_state == CHANNEL_STATE_ERROR) { + if (to_state == CHANNEL_STATE_CLOSED || + to_state == CHANNEL_STATE_ERROR) { /* Assert that all queues are empty */ tor_assert(TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)); tor_assert(TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue)); @@ -2111,6 +2101,35 @@ channel_change_state(channel_t *chan, channel_state_t to_state) } /** + * As channel_change_state_, but change the state to any state but open. + */ +void +channel_change_state(channel_t *chan, channel_state_t to_state) +{ + tor_assert(to_state != CHANNEL_STATE_OPEN); + channel_change_state_(chan, to_state); +} + +/** + * As channel_change_state, but change the state to open. + */ +void +channel_change_state_open(channel_t *chan) +{ + channel_change_state_(chan, CHANNEL_STATE_OPEN); + + /* Tell circuits if we opened and stuff */ + channel_do_open_actions(chan); + chan->has_been_open = 1; + + /* Check for queued cells to process */ + if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)) + channel_process_cells(chan); + if (! TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue)) + channel_flush_cells(chan); +} + +/** * Change channel listener state * * This internal and subclass use only function is used to change channel diff --git a/src/or/channel.h b/src/or/channel.h index 33dceb1..803ab6c 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -436,6 +436,7 @@ void channel_listener_free(channel_listener_t *chan_l); /* State/metadata setters */ void channel_change_state(channel_t *chan, channel_state_t to_state); +void channel_change_state_open(channel_t *chan); void channel_clear_identity_digest(channel_t *chan); void channel_clear_remote_end(channel_t *chan); void channel_mark_local(channel_t *chan); diff --git a/src/or/channeltls.c b/src/or/channeltls.c index 9d9e744..300304f 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -979,7 +979,7 @@ channel_tls_handle_state_change_on_orconn(channel_tls_t *chan, * We can go to CHANNEL_STATE_OPEN from CHANNEL_STATE_OPENING or * CHANNEL_STATE_MAINT on this. */ - channel_change_state(base_chan, CHANNEL_STATE_OPEN); + channel_change_state_open(base_chan); /* We might have just become writeable; check and tell the scheduler */ if (connection_or_num_cells_writeable(conn) > 0) { scheduler_channel_wants_writes(base_chan); diff --git a/src/test/test_channel.c b/src/test/test_channel.c index f5999b8..347aca7 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -811,7 +811,7 @@ test_channel_incoming(void *arg) tt_assert(ch->registered); /* Open it */ - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_int_op(ch->state, ==, CHANNEL_STATE_OPEN); /* Receive a fixed cell */ @@ -899,7 +899,7 @@ test_channel_lifecycle(void *arg) tt_int_op(old_count, ==, test_cells_written); /* Move it to OPEN and flush */ - channel_change_state(ch1, CHANNEL_STATE_OPEN); + channel_change_state_open(ch1); /* Queue should drain */ tt_int_op(old_count + 1, ==, test_cells_written); @@ -925,13 +925,13 @@ test_channel_lifecycle(void *arg) tt_int_op(test_releases_count, ==, init_releases_count); /* Move ch2 to OPEN */ - channel_change_state(ch2, CHANNEL_STATE_OPEN); + channel_change_state_open(ch2); tt_int_op(test_doesnt_want_writes_count, ==, init_doesnt_want_writes_count + 1); tt_int_op(test_releases_count, ==, init_releases_count); /* Move ch1 back to OPEN */ - channel_change_state(ch1, CHANNEL_STATE_OPEN); + channel_change_state_open(ch1); tt_int_op(test_doesnt_want_writes_count, ==, init_doesnt_want_writes_count + 1); tt_int_op(test_releases_count, ==, init_releases_count); @@ -1018,7 +1018,7 @@ test_channel_lifecycle_2(void *arg) tt_assert(ch->registered); /* Finish opening it */ - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); /* Error exit from lower layer */ chan_test_error(ch); @@ -1037,7 +1037,7 @@ test_channel_lifecycle_2(void *arg) tt_assert(ch->registered); /* Finish opening it */ - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_int_op(ch->state, ==, CHANNEL_STATE_OPEN); /* Go to maintenance state */ @@ -1066,7 +1066,7 @@ test_channel_lifecycle_2(void *arg) tt_assert(ch->registered); /* Finish opening it */ - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_int_op(ch->state, ==, CHANNEL_STATE_OPEN); /* Go to maintenance state */ @@ -1092,7 +1092,7 @@ test_channel_lifecycle_2(void *arg) tt_assert(ch->registered); /* Finish opening it */ - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_int_op(ch->state, ==, CHANNEL_STATE_OPEN); /* Go to maintenance state */ @@ -1322,7 +1322,7 @@ test_channel_queue_impossible(void *arg) * gets thrown away properly. */ test_chan_accept_cells = 1; - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_assert(test_cells_written == old_count); tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), ==, 0); @@ -1350,7 +1350,7 @@ test_channel_queue_impossible(void *arg) /* Let it drain and check that the bad entry is discarded */ test_chan_accept_cells = 1; - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_assert(test_cells_written == old_count); tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), ==, 0); @@ -1378,7 +1378,7 @@ test_channel_queue_impossible(void *arg) /* Let it drain and check that the bad entry is discarded */ test_chan_accept_cells = 1; - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_assert(test_cells_written == old_count); tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), ==, 0); @@ -1406,7 +1406,7 @@ test_channel_queue_impossible(void *arg) /* Let it drain and check that the bad entry is discarded */ test_chan_accept_cells = 1; tor_capture_bugs_(1); - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_assert(test_cells_written == old_count); tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), ==, 0); @@ -1463,7 +1463,7 @@ test_channel_queue_incoming(void *arg) tt_assert(ch->registered); /* Open it */ - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_int_op(ch->state, ==, CHANNEL_STATE_OPEN); /* Assert that the incoming queue is empty */ @@ -1603,7 +1603,7 @@ test_channel_queue_size(void *arg) /* Go to open */ old_count = test_cells_written; - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); /* * It should try to write, but we aren't accepting cells right now, so @@ -1706,7 +1706,7 @@ test_channel_write(void *arg) * gets drained from the queue. */ test_chan_accept_cells = 1; - channel_change_state(ch, CHANNEL_STATE_OPEN); + channel_change_state_open(ch); tt_assert(test_cells_written == old_count + 1); /* diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index 4c536b0..a2e77a4 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -567,7 +567,7 @@ test_scheduler_loop(void *arg) channel_register(ch1); tt_assert(ch1->registered); /* Finish opening it */ - channel_change_state(ch1, CHANNEL_STATE_OPEN); + channel_change_state_open(ch1); /* It should start off in SCHED_CHAN_IDLE */ tt_int_op(ch1->scheduler_state, ==, SCHED_CHAN_IDLE); @@ -636,7 +636,7 @@ test_scheduler_loop(void *arg) tt_int_op(smartlist_len(channels_pending), ==, 0); /* Now, finish opening ch2, and get both back to pending */ - channel_change_state(ch2, CHANNEL_STATE_OPEN); + channel_change_state_open(ch2); scheduler_channel_wants_writes(ch1); scheduler_channel_wants_writes(ch2); scheduler_channel_has_waiting_cells(ch1);