Tweak TB_EMPTY event based on comments by nickm.

- Avoid control_event_refill_global function with 13 arguments and
  increase code reuse factor by moving more code from control.c to
  connection.c.
- Avoid an unsafe uint32_t -> int cast.
- Add TestingEnableTbEmptyEvent option.
- Prepare functions for testing.
- Rename a few functions and improve documentation.

5 files changed, 134 insertions, 140 deletions

diff --git a/src/or/config.c b/src/or/config.c
index 7a17a9f1f..4ca0338d7 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -220,6 +220,7 @@ static config_var_t option_vars_[] = {
   V(DownloadExtraInfo,           BOOL,     "0"),
   V(TestingEnableConnBwEvent,    BOOL,     "0"),
   V(TestingEnableCellStatsEvent, BOOL,     "0"),
+  V(TestingEnableTbEmptyEvent,   BOOL,     "0"),
   V(EnforceDistinctSubnets,      BOOL,     "1"),
   V(EntryNodes,                  ROUTERSET,   NULL),
   V(EntryStatistics,             BOOL,     "0"),
@@ -465,6 +466,7 @@ static const config_var_t testing_tor_network_defaults[] = {
   V(MinUptimeHidServDirectoryV2, INTERVAL, "0 minutes"),
   V(TestingEnableConnBwEvent,    BOOL,     "1"),
   V(TestingEnableCellStatsEvent, BOOL,     "1"),
+  V(TestingEnableTbEmptyEvent,   BOOL,     "1"),
   VAR("___UsingTestNetworkDefaults", BOOL, UsingTestNetworkDefaults_, "1"),
 
   { NULL, CONFIG_TYPE_OBSOLETE, 0, NULL }
@@ -3252,6 +3254,12 @@ options_validate(or_options_t *old_options, or_options_t *options,
            "Tor networks!");
   }
 
+  if (options->TestingEnableTbEmptyEvent &&
+      !options->TestingTorNetwork && !options->UsingTestNetworkDefaults_) {
+    REJECT("TestingEnableTbEmptyEvent may only be changed in testing "
+           "Tor networks!");
+  }
+
   if (options->TestingTorNetwork) {
     log_warn(LD_CONFIG, "TestingTorNetwork is set. This will make your node "
                         "almost unusable in the public Tor network, and is "
diff --git a/src/or/connection.c b/src/or/connection.c
index f7f028b0b..2f2a421fc 100644
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -2541,25 +2541,33 @@ record_num_bytes_transferred(connection_t *conn,
 #endif
 
 #ifndef USE_BUFFEREVENTS
-/** Last emptied global or relay buckets in msec since midnight; only used
- * in TestingTorNetwork mode. */
+/** Last time at which the global or relay buckets were emptied in msec
+ * since midnight. */
 static uint32_t global_relayed_read_emptied = 0,
                 global_relayed_write_emptied = 0,
                 global_read_emptied = 0,
                 global_write_emptied = 0;
 
-/** Check if a bucket has just run out of tokens, and if so, note the
- * timestamp for TB_EMPTY events; only used in TestingTorNetwork mode. */
+/** Helper: convert given <b>tvnow</b> time value to milliseconds since
+ * midnight. */
+static uint32_t
+msec_since_midnight(struct timeval tvnow)
+{
+  return (uint32_t)(((tvnow.tv_sec % 86400L) * 1000L) +
+         ((uint32_t)tvnow.tv_usec / (uint32_t)1000L));
+}
+
+/** Check if a bucket which had <b>tokens_before</b> tokens and which got
+ * <b>tokens_removed</b> tokens removed at timestamp <b>tvnow</b> has run
+ * out of tokens, and if so, note the milliseconds since midnight in
+ * <b>timestamp_var</b> for the next TB_EMPTY event. */
 static void
-connection_buckets_empty_ts(uint32_t *timestamp_var, int tokens_before,
-                            size_t tokens_removed)
+connection_buckets_note_empty_ts(uint32_t *timestamp_var,
+                                 int tokens_before, size_t tokens_removed,
+                                 struct timeval tvnow)
 {
-  if (tokens_before > 0 && tokens_before - (int)tokens_removed <= 0) {
-    struct timeval tvnow;
-    tor_gettimeofday_cached(&tvnow);
-    *timestamp_var = (uint32_t)(((tvnow.tv_sec % 86400L) * 1000L) +
-        ((uint32_t)tvnow.tv_usec / (uint32_t)1000L));
-  }
+  if (tokens_before > 0 && (uint32_t)tokens_before <= tokens_removed)
+    *timestamp_var = msec_since_midnight(tvnow);
 }
 
 /** We just read <b>num_read</b> and wrote <b>num_written</b> bytes
@@ -2586,23 +2594,25 @@ connection_buckets_decrement(connection_t *conn, time_t now,
 
   /* If one or more of our token buckets ran dry just now, note the
    * timestamp for TB_EMPTY events. */
-  if (get_options()->TestingTorNetwork) {
+  if (get_options()->TestingEnableTbEmptyEvent) {
+    struct timeval tvnow;
+    tor_gettimeofday_cached(&tvnow);
     if (connection_counts_as_relayed_traffic(conn, now)) {
-      connection_buckets_empty_ts(&global_relayed_read_emptied,
-                                  global_relayed_read_bucket, num_read);
-      connection_buckets_empty_ts(&global_relayed_write_emptied,
-                                  global_relayed_write_bucket, num_written);
+      connection_buckets_note_empty_ts(&global_relayed_read_emptied,
+                         global_relayed_read_bucket, num_read, tvnow);
+      connection_buckets_note_empty_ts(&global_relayed_write_emptied,
+                         global_relayed_write_bucket, num_written, tvnow);
     }
-    connection_buckets_empty_ts(&global_read_emptied, global_read_bucket,
-                                num_read);
-    connection_buckets_empty_ts(&global_write_emptied, global_write_bucket,
-                                num_written);
+    connection_buckets_note_empty_ts(&global_read_emptied,
+                       global_read_bucket, num_read, tvnow);
+    connection_buckets_note_empty_ts(&global_write_emptied,
+                       global_write_bucket, num_written, tvnow);
     if (connection_speaks_cells(conn) && conn->state == OR_CONN_STATE_OPEN) {
       or_connection_t *or_conn = TO_OR_CONN(conn);
-      connection_buckets_empty_ts(&or_conn->read_emptied_time,
-                                  or_conn->read_bucket, num_read);
-      connection_buckets_empty_ts(&or_conn->write_emptied_time,
-                                  or_conn->write_bucket, num_written);
+      connection_buckets_note_empty_ts(&or_conn->read_emptied_time,
+                         or_conn->read_bucket, num_read, tvnow);
+      connection_buckets_note_empty_ts(&or_conn->write_emptied_time,
+                         or_conn->write_bucket, num_written, tvnow);
     }
   }
 
@@ -2712,6 +2722,28 @@ connection_bucket_refill_helper(int *bucket, int rate, int burst,
   }
 }
 
+/** Helper: return the time in milliseconds since <b>last_empty_time</b>
+ * when a bucket ran empty that previously had <b>tokens_before</b> tokens
+ * now has <b>tokens_after</b> tokens after refilling at timestamp
+ * <b>tvnow</b>, capped at <b>milliseconds_elapsed</b> milliseconds since
+ * last refilling that bucket.  Return 0 if the bucket has not been empty
+ * since the last refill or has not been refilled. */
+static uint32_t
+bucket_millis_empty(int tokens_before, uint32_t last_empty_time,
+                    int tokens_after, int milliseconds_elapsed,
+                    struct timeval tvnow)
+{
+  uint32_t result = 0, refilled;
+  if (tokens_before <= 0 && tokens_after > tokens_before) {
+    refilled = msec_since_midnight(tvnow);
+    result = (uint32_t)((refilled + 86400L * 1000L - last_empty_time) %
+             (86400L * 1000L));
+    if (result > (uint32_t)milliseconds_elapsed)
+      result = (uint32_t)milliseconds_elapsed;
+  }
+  return result;
+}
+
 /** Time has passed; increment buckets appropriately. */
 void
 connection_bucket_refill(int milliseconds_elapsed, time_t now)
@@ -2724,6 +2756,7 @@ connection_bucket_refill(int milliseconds_elapsed, time_t now)
   int prev_global_write = global_write_bucket;
   int prev_relay_read = global_relayed_read_bucket;
   int prev_relay_write = global_relayed_write_bucket;
+  struct timeval tvnow; /*< Only used if TB_EMPTY events are enabled. */
 
   bandwidthrate = (int)options->BandwidthRate;
   bandwidthburst = (int)options->BandwidthBurst;
@@ -2759,14 +2792,31 @@ connection_bucket_refill(int milliseconds_elapsed, time_t now)
                                   milliseconds_elapsed,
                                   "global_relayed_write_bucket");
 
-  control_event_refill_global(global_read_bucket, prev_global_read,
-                              global_read_emptied, global_write_bucket,
-                              prev_global_write, global_write_emptied,
-                              global_relayed_read_bucket, prev_relay_read,
-                              global_relayed_read_emptied,
-                              global_relayed_write_bucket, prev_relay_write,
-                              global_relayed_write_emptied,
-                              milliseconds_elapsed);
+  /* If buckets were empty before and have now been refilled, tell any
+   * interested controllers. */
+  if (get_options()->TestingEnableTbEmptyEvent) {
+    uint32_t global_read_empty_time, global_write_empty_time,
+             relay_read_empty_time, relay_write_empty_time;
+    tor_gettimeofday_cached(&tvnow);
+    global_read_empty_time = bucket_millis_empty(prev_global_read,
+                             global_read_emptied, global_read_bucket,
+                             milliseconds_elapsed, tvnow);
+    global_write_empty_time = bucket_millis_empty(prev_global_write,
+                              global_write_emptied, global_write_bucket,
+                              milliseconds_elapsed, tvnow);
+    control_event_tb_empty("GLOBAL", global_read_empty_time,
+                           global_write_empty_time, milliseconds_elapsed);
+    relay_read_empty_time = bucket_millis_empty(prev_relay_read,
+                            global_relayed_read_emptied,
+                            global_relayed_read_bucket,
+                            milliseconds_elapsed, tvnow);
+    relay_write_empty_time = bucket_millis_empty(prev_relay_write,
+                             global_relayed_write_emptied,
+                             global_relayed_write_bucket,
+                             milliseconds_elapsed, tvnow);
+    control_event_tb_empty("RELAY", relay_read_empty_time,
+                           relay_write_empty_time, milliseconds_elapsed);
+  }
 
   /* refill the per-connection buckets */
   SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn) {
@@ -2793,8 +2843,26 @@ connection_bucket_refill(int milliseconds_elapsed, time_t now)
                                         "or_conn->write_bucket");
       }
 
-      control_event_refill_conn(or_conn, prev_conn_read, prev_conn_write,
-                                (uint32_t)milliseconds_elapsed);
+      /* If buckets were empty before and have now been refilled, tell any
+       * interested controllers. */
+      if (get_options()->TestingEnableTbEmptyEvent) {
+        char *bucket;
+        uint32_t conn_read_empty_time, conn_write_empty_time;
+        tor_asprintf(&bucket, "ORCONN ID="U64_FORMAT,
+                     U64_PRINTF_ARG(or_conn->base_.global_identifier));
+        conn_read_empty_time = bucket_millis_empty(prev_conn_read,
+                               or_conn->read_emptied_time,
+                               or_conn->read_bucket,
+                               milliseconds_elapsed, tvnow);
+        conn_write_empty_time = bucket_millis_empty(prev_conn_write,
+                                or_conn->write_emptied_time,
+                                or_conn->write_bucket,
+                                milliseconds_elapsed, tvnow);
+        control_event_tb_empty(bucket, conn_read_empty_time,
+                               conn_write_empty_time,
+                               milliseconds_elapsed);
+        tor_free(bucket);
+      }
     }
 
     if (conn->read_blocked_on_bw == 1 /* marked to turn reading back on now */
diff --git a/src/or/control.c b/src/or/control.c
index c06a91182..ac7be8d59 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -4155,99 +4155,23 @@ control_event_circuit_cell_stats(void)
   return 0;
 }
 
-/** Helper: return the time in millis that a given bucket was empty,
- * capped at the time in millis since last refilling that bucket.  Return
- * 0 if the bucket was not empty during the last refill period. */
-static uint32_t
-bucket_millis_empty(int prev_tokens, uint32_t last_empty_time,
-                    uint32_t milliseconds_elapsed)
-{
-  uint32_t result = 0, refilled;
-  if (prev_tokens <= 0) {
-    struct timeval tvnow;
-    tor_gettimeofday_cached(&tvnow);
-    refilled = (uint32_t)((tvnow.tv_sec % 86400L) * 1000L +
-               (uint32_t)tvnow.tv_usec / (uint32_t)1000L);
-    result = (uint32_t)((refilled + 86400L * 1000L - last_empty_time) %
-             (86400L * 1000L));
-    if (result > milliseconds_elapsed)
-      result = milliseconds_elapsed;
-  }
-  return result;
-}
-
-/** Token buckets have been refilled: tell any interested control
- * connections how global and relay token buckets have changed. */
+/** Tokens in <b>bucket</b> have been refilled: the read bucket was empty
+ * for <b>read_empty_time</b> millis, the write bucket was empty for
+ * <b>write_empty_time</b> millis, and buckets were last refilled
+ * <b>milliseconds_elapsed</b> millis ago.  Only emit TB_EMPTY event if
+ * either read or write bucket have been empty before. */
 int
-control_event_refill_global(int global_read, int prev_global_read,
-                            uint32_t global_read_emptied_time,
-                            int global_write, int prev_global_write,
-                            uint32_t global_write_emptied_time,
-                            int relay_read, int prev_relay_read,
-                            uint32_t relay_read_emptied_time,
-                            int relay_write, int prev_relay_write,
-                            uint32_t relay_write_emptied_time,
-                            uint32_t milliseconds_elapsed)
+control_event_tb_empty(const char *bucket, uint32_t read_empty_time,
+                       uint32_t write_empty_time,
+                       int milliseconds_elapsed)
 {
-  uint32_t global_read_empty_time, global_write_empty_time,
-      relay_read_empty_time, relay_write_empty_time;
-  if (!get_options()->TestingTorNetwork ||
-      !EVENT_IS_INTERESTING(EVENT_TB_EMPTY))
-    return 0;
-  if (prev_global_read == global_read &&
-      prev_global_write == global_write &&
-      prev_relay_read == relay_read &&
-      prev_relay_write == relay_write)
-    return 0;
-  if (prev_global_read <= 0 && prev_global_write <= 0) {
-    global_read_empty_time = bucket_millis_empty(prev_global_read,
-        global_read_emptied_time, milliseconds_elapsed);
-    global_write_empty_time = bucket_millis_empty(prev_global_write,
-        global_write_emptied_time, milliseconds_elapsed);
+  if (get_options()->TestingEnableTbEmptyEvent &&
+      EVENT_IS_INTERESTING(EVENT_TB_EMPTY) &&
+      (read_empty_time > 0 || write_empty_time > 0)) {
     send_control_event(EVENT_TB_EMPTY, ALL_FORMATS,
-                       "650 TB_EMPTY GLOBAL READ=%d WRITTEN=%d "
+                       "650 TB_EMPTY %s READ=%d WRITTEN=%d "
                        "LAST=%d\r\n",
-                       global_read_empty_time, global_write_empty_time,
-                       milliseconds_elapsed);
-  }
-  if (prev_relay_read <= 0 && prev_relay_write <= 0) {
-    relay_read_empty_time = bucket_millis_empty(prev_relay_read,
-        relay_read_emptied_time, milliseconds_elapsed);
-    relay_write_empty_time = bucket_millis_empty(prev_relay_write,
-        relay_write_emptied_time, milliseconds_elapsed);
-    send_control_event(EVENT_TB_EMPTY, ALL_FORMATS,
-                       "650 TB_EMPTY RELAY READ=%d WRITTEN=%d "
-                       "LAST=%d\r\n",
-                       relay_read_empty_time, relay_write_empty_time,
-                       milliseconds_elapsed);
-  }
-  return 0;
-}
-
-/** Token buckets of a connection have been refilled: tell any interested
- * control connections how per-connection token buckets have changed. */
-int
-control_event_refill_conn(or_connection_t *or_conn,
-                          int prev_read, int prev_write,
-                          uint32_t milliseconds_elapsed)
-{
-  uint32_t read_bucket_empty_time, write_bucket_empty_time;
-  if (!get_options()->TestingTorNetwork ||
-      !EVENT_IS_INTERESTING(EVENT_TB_EMPTY))
-    return 0;
-  if (prev_read == or_conn->read_bucket &&
-      prev_write == or_conn->write_bucket)
-    return 0;
-  if (prev_read <= 0 || prev_write <= 0) {
-    read_bucket_empty_time = bucket_millis_empty(prev_read,
-        or_conn->read_emptied_time, milliseconds_elapsed);
-    write_bucket_empty_time = bucket_millis_empty(prev_write,
-        or_conn->write_emptied_time, milliseconds_elapsed);
-    send_control_event(EVENT_TB_EMPTY, ALL_FORMATS,
-                       "650 TB_EMPTY ORCONN ID="U64_FORMAT" READ=%d "
-                       "WRITTEN=%d LAST=%d\r\n",
-                       U64_PRINTF_ARG(or_conn->base_.global_identifier),
-                       read_bucket_empty_time, write_bucket_empty_time,
+                       bucket, read_empty_time, write_empty_time,
                        milliseconds_elapsed);
   }
   return 0;
diff --git a/src/or/control.h b/src/or/control.h
index 06a384949..c79c432ef 100644
--- a/src/or/control.h
+++ b/src/or/control.h
@@ -54,18 +54,9 @@ int control_event_circ_bandwidth_used(void);
 int control_event_conn_bandwidth(connection_t *conn);
 int control_event_conn_bandwidth_used(void);
 int control_event_circuit_cell_stats(void);
-int control_event_refill_global(int global_read, int prev_global_read,
-                                uint32_t global_read_emptied,
-                                int global_write, int prev_global_write,
-                                uint32_t global_write_emptied,
-                                int relay_read, int prev_relay_read,
-                                uint32_t relay_read_emptied,
-                                int relay_write, int prev_relay_write,
-                                uint32_t relay_write_emptied,
-                                uint32_t milliseconds_elapsed);
-int control_event_refill_conn(or_connection_t *or_conn,
-                              int prev_read, int prev_write,
-                              uint32_t milliseconds_elapsed);
+int control_event_tb_empty(const char *bucket, uint32_t read_empty_time,
+                           uint32_t write_empty_time,
+                           int milliseconds_elapsed);
 void control_event_logmsg(int severity, uint32_t domain, const char *msg);
 int control_event_descriptors_changed(smartlist_t *routers);
 int control_event_address_mapped(const char *from, const char *to,
diff --git a/src/or/or.h b/src/or/or.h
index 3702664a2..21a36c928 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -1479,11 +1479,11 @@ typedef struct or_connection_t {
 
   struct or_connection_t *next_with_same_id; /**< Next connection with same
                                               * identity digest as this one. */
-  /** Last emptied read token bucket in msec since midnight; only used in
-   * TestingTorNetwork mode. */
+  /** Last emptied read token bucket in msec since midnight; only used if
+   * TB_EMPTY events are enabled. */
   uint32_t read_emptied_time;
-  /** Last emptied write token bucket in msec since midnight; only used in
-   * TestingTorNetwork mode. */
+  /** Last emptied write token bucket in msec since midnight; only used if
+   * TB_EMPTY events are enabled. */
   uint32_t write_emptied_time;
 } or_connection_t;
 
@@ -3991,6 +3991,9 @@ typedef struct {
   /** Enable CELL_STATS events.  Only altered on testing networks. */
   int TestingEnableCellStatsEvent;
 
+  /** Enable TB_EMPTY events.  Only altered on testing networks. */
+  int TestingEnableTbEmptyEvent;
+
   /** If true, and we have GeoIP data, and we're a bridge, keep a per-country
    * count of how many client addresses have contacted us so that we can help
    * the bridge authority guess which countries have blocked access to us. */