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.

6 files changed, 58 insertions, 32 deletions

diff --git a/changes/ticket22608 b/changes/ticket22608
new file mode 100644
index 0000000000..5aa9db27f1
--- /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 e79fc0760b..68ebc4d1e6 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 33dceb1be0..803ab6c4ac 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 9d9e7446ab..300304fad1 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 f5999b8e67..347aca7ecb 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 4c536b0905..a2e77a45d4 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);