[tor-commits] [tor/master] Unify code in channel_write_*cell()

[nickm at torproject.org]

commit ec4142abdf7fa4efaa281413e31acc93bf22ea4a
Author: Nick Mathewson <nickm at torproject.org>
Date:   Wed Oct 19 17:07:23 2016 -0400

    Unify code in channel_write_*cell()
    
    Patch from pingl; patch for 13827.
---
 changes/bug13827 |  3 ++
 src/or/channel.c | 93 +++++++++++++++++++-------------------------------------
 src/or/channel.h |  3 ++
 3 files changed, 38 insertions(+), 61 deletions(-)

diff --git a/changes/bug13827 b/changes/bug13827
new file mode 100644
index 0000000..2235a3f
--- /dev/null
+++ b/changes/bug13827
@@ -0,0 +1,3 @@
+  o Code simplification and refactoring:
+    - Remove duplicate code in the channel_write_*cell() functions.
+       Closes ticket 13827; patch from Pingl.
diff --git a/src/or/channel.c b/src/or/channel.c
index f547aea..75aab3c 100644
--- a/src/or/channel.c
+++ b/src/or/channel.c
@@ -1838,45 +1838,58 @@ channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q)
   }
 }
 
-/**
- * Write a cell to a channel
+/** Write a generic cell type to a channel
  *
- * Write a fixed-length cell to a channel using the write_cell() method.
- * This is equivalent to the pre-channels connection_or_write_cell_to_buf();
- * it is called by the transport-independent code to deliver a cell to a
- * channel for transmission.
+ * Write a generic cell to a channel. It is called by channel_write_cell(),
+ * channel_write_var_cell() and channel_write_packed_cell() in order to reduce
+ * code duplication. Notice that it takes cell as pointer of type void,
+ * this can be dangerous because no type check is performed.
  */
 
 void
-channel_write_cell(channel_t *chan, cell_t *cell)
+channel_write_cell_generic_(channel_t *chan, const char *cell_type,
+                            void *cell, cell_queue_entry_t *q)
 {
-  cell_queue_entry_t q;
 
   tor_assert(chan);
   tor_assert(cell);
 
   if (CHANNEL_IS_CLOSING(chan)) {
-    log_debug(LD_CHANNEL, "Discarding cell_t %p on closing channel %p with "
-              "global ID "U64_FORMAT, cell, chan,
+    log_debug(LD_CHANNEL, "Discarding %c %p on closing channel %p with "
+              "global ID "U64_FORMAT, *cell_type, cell, chan,
               U64_PRINTF_ARG(chan->global_identifier));
     tor_free(cell);
     return;
   }
-
   log_debug(LD_CHANNEL,
-            "Writing cell_t %p to channel %p with global ID "
-            U64_FORMAT,
+            "Writing %c %p to channel %p with global ID "
+            U64_FORMAT, *cell_type,
             cell, chan, U64_PRINTF_ARG(chan->global_identifier));
 
-  q.type = CELL_QUEUE_FIXED;
-  q.u.fixed.cell = cell;
-  channel_write_cell_queue_entry(chan, &q);
-
+  channel_write_cell_queue_entry(chan, q);
   /* Update the queue size estimate */
   channel_update_xmit_queue_size(chan);
 }
 
 /**
+ * Write a cell to a channel
+ *
+ * Write a fixed-length cell to a channel using the write_cell() method.
+ * This is equivalent to the pre-channels connection_or_write_cell_to_buf();
+ * it is called by the transport-independent code to deliver a cell to a
+ * channel for transmission.
+ */
+
+void
+channel_write_cell(channel_t *chan, cell_t *cell)
+{
+  cell_queue_entry_t q;
+  q.type = CELL_QUEUE_FIXED;
+  q.u.fixed.cell = cell;
+  channel_write_cell_generic_(chan, "cell_t", cell, &q);
+}
+
+/**
  * Write a packed cell to a channel
  *
  * Write a packed cell to a channel using the write_cell() method.  This is
@@ -1888,30 +1901,9 @@ void
 channel_write_packed_cell(channel_t *chan, packed_cell_t *packed_cell)
 {
   cell_queue_entry_t q;
-
-  tor_assert(chan);
-  tor_assert(packed_cell);
-
-  if (CHANNEL_IS_CLOSING(chan)) {
-    log_debug(LD_CHANNEL, "Discarding packed_cell_t %p on closing channel %p "
-              "with global ID "U64_FORMAT, packed_cell, chan,
-              U64_PRINTF_ARG(chan->global_identifier));
-    packed_cell_free(packed_cell);
-    return;
-  }
-
-  log_debug(LD_CHANNEL,
-            "Writing packed_cell_t %p to channel %p with global ID "
-            U64_FORMAT,
-            packed_cell, chan,
-            U64_PRINTF_ARG(chan->global_identifier));
-
   q.type = CELL_QUEUE_PACKED;
   q.u.packed.packed_cell = packed_cell;
-  channel_write_cell_queue_entry(chan, &q);
-
-  /* Update the queue size estimate */
-  channel_update_xmit_queue_size(chan);
+  channel_write_cell_generic_(chan, "packed_cell_t", packed_cell, &q);
 }
 
 /**
@@ -1927,30 +1919,9 @@ void
 channel_write_var_cell(channel_t *chan, var_cell_t *var_cell)
 {
   cell_queue_entry_t q;
-
-  tor_assert(chan);
-  tor_assert(var_cell);
-
-  if (CHANNEL_IS_CLOSING(chan)) {
-    log_debug(LD_CHANNEL, "Discarding var_cell_t %p on closing channel %p "
-              "with global ID "U64_FORMAT, var_cell, chan,
-              U64_PRINTF_ARG(chan->global_identifier));
-    var_cell_free(var_cell);
-    return;
-  }
-
-  log_debug(LD_CHANNEL,
-            "Writing var_cell_t %p to channel %p with global ID "
-            U64_FORMAT,
-            var_cell, chan,
-            U64_PRINTF_ARG(chan->global_identifier));
-
   q.type = CELL_QUEUE_VAR;
   q.u.var.var_cell = var_cell;
-  channel_write_cell_queue_entry(chan, &q);
-
-  /* Update the queue size estimate */
-  channel_update_xmit_queue_size(chan);
+  channel_write_cell_generic_(chan, "var_cell_t", var_cell, &q);
 }
 
 /**
diff --git a/src/or/channel.h b/src/or/channel.h
index a711b56..44a3901 100644
--- a/src/or/channel.h
+++ b/src/or/channel.h
@@ -382,6 +382,9 @@ struct cell_queue_entry_s {
 STATIC int chan_cell_queue_len(const chan_cell_queue_t *queue);
 
 STATIC void cell_queue_entry_free(cell_queue_entry_t *q, int handed_off);
+
+void channel_write_cell_generic_(channel_t *chan, const char *cell_type,
+                                 void *cell, cell_queue_entry_t *q);
 #endif
 
 /* Channel operations for subclasses and internal use only */