Merge branch 'decouple_controller_events_squashed'

changes/decouple_control_events

+  o Code simplification and refactoring:
+    - When generating an event to send to the controller, we no longer
+      put the event over the network immediately.  Instead, we queue
+      these events, and use a Libevent callback to deliver them.
+      This change simplifies Tor's callgraph by reducing the number
+      of functions from which all other Tor functions are reachable.
+      Closes ticket 16695.
+

src/common/compat_pthreads.c

@@ -275,6 +275,33 @@ tor_cond_signal_all(tor_cond_t *cond)
   pthread_cond_broadcast(&cond->cond);
 }
 
+int
+tor_threadlocal_init(tor_threadlocal_t *threadlocal)
+{
+  int err = pthread_key_create(&threadlocal->key, NULL);
+  return err ? -1 : 0;
+}
+
+void
+tor_threadlocal_destroy(tor_threadlocal_t *threadlocal)
+{
+  pthread_key_delete(threadlocal->key);
+  memset(threadlocal, 0, sizeof(tor_threadlocal_t));
+}
+
+void *
+tor_threadlocal_get(tor_threadlocal_t *threadlocal)
+{
+  return pthread_getspecific(threadlocal->key);
+}
+
+void
+tor_threadlocal_set(tor_threadlocal_t *threadlocal, void *value)
+{
+  int err = pthread_setspecific(threadlocal->key, value);
+  tor_assert(err == 0);
+}
+
 /** Set up common structures for use by threading. */
 void
 tor_threads_init(void)

src/common/compat_threads.h

@@ -111,5 +111,41 @@ typedef struct alert_sockets_s {
 int alert_sockets_create(alert_sockets_t *socks_out, uint32_t flags);
 void alert_sockets_close(alert_sockets_t *socks);
 
+typedef struct tor_threadlocal_s {
+#ifdef _WIN32
+  DWORD index;
+#else
+  pthread_key_t key;
+#endif
+} tor_threadlocal_t;
+
+/** Initialize a thread-local variable.
+ *
+ * After you call this function on a tor_threadlocal_t, you can call
+ * tor_threadlocal_set to change the current value of this variable for the
+ * current thread, and tor_threadlocal_get to retrieve the current value for
+ * the current thread.  Each thread has its own value.
+ **/
+int tor_threadlocal_init(tor_threadlocal_t *threadlocal);
+/**
+ * Release all resource associated with a thread-local variable.
+ */
+void tor_threadlocal_destroy(tor_threadlocal_t *threadlocal);
+/**
+ * Return the current value of a thread-local variable for this thread.
+ *
+ * It's undefined behavior to use this function if the threadlocal hasn't
+ * been initialized, or has been destroyed.
+ */
+void *tor_threadlocal_get(tor_threadlocal_t *threadlocal);
+/**
+ * Change the current value of a thread-local variable for this thread to
+ * <b>value</b>.
+ *
+ * It's undefined behavior to use this function if the threadlocal hasn't
+ * been initialized, or has been destroyed.
+ */
+void tor_threadlocal_set(tor_threadlocal_t *threadlocal, void *value);
+
 #endif
 

src/common/compat_winthreads.c

@@ -124,6 +124,49 @@ tor_cond_signal_all(tor_cond_t *cond)
   tor_cond_signal_impl(cond, 1);
 }
 
+int
+tor_threadlocal_init(tor_threadlocal_t *threadlocal)
+{
+  threadlocal->index = TlsAlloc();
+  return (threadlocal->index == TLS_OUT_OF_INDEXES) ? -1 : 0;
+}
+
+void
+tor_threadlocal_destroy(tor_threadlocal_t *threadlocal)
+{
+  TlsFree(threadlocal->index);
+  memset(threadlocal, 0, sizeof(tor_threadlocal_t));
+}
+
+void *
+tor_threadlocal_get(tor_threadlocal_t *threadlocal)
+{
+  void *value = TlsGetValue(threadlocal->index);
+  if (value == NULL) {
+    DWORD err = GetLastError();
+    if (err != ERROR_SUCCESS) {
+      char *msg = format_win32_error(err);
+      log_err(LD_GENERAL, "Error retrieving thread-local value: %s", msg);
+      tor_free(msg);
+      tor_assert(err == ERROR_SUCCESS);
+    }
+  }
+  return value;
+}
+
+void
+tor_threadlocal_set(tor_threadlocal_t *threadlocal, void *value)
+{
+  BOOL ok = TlsSetValue(threadlocal->index, value);
+  if (!ok) {
+    DWORD err = GetLastError();
+    char *msg = format_win32_error(err);
+    log_err(LD_GENERAL, "Error adjusting thread-local value: %s", msg);
+    tor_free(msg);
+    tor_assert(ok);
+  }
+}
+
 int
 tor_cond_wait(tor_cond_t *cond, tor_mutex_t *lock_, const struct timeval *tv)
 {

src/or/config.c

@@ -1106,6 +1106,9 @@ options_act_reversible(const or_options_t *old_options, char **msg)
       init_libevent(options);
       libevent_initialized = 1;
 
+      /* This has to come up after libevent is initialized. */
+      control_initialize_event_queue();
+
       /*
        * Initialize the scheduler - this has to come after
        * options_init_from_torrc() sets up libevent - why yes, that seems

src/or/control.h

@@ -12,6 +12,8 @@
 #ifndef TOR_CONTROL_H
 #define TOR_CONTROL_H
 
+void control_initialize_event_queue(void);
+
 void control_update_global_event_mask(void);
 void control_adjust_event_log_severity(void);
 
@@ -78,6 +80,14 @@ int control_event_client_status(int severity, const char *format, ...)
   CHECK_PRINTF(2,3);
 int control_event_server_status(int severity, const char *format, ...)
   CHECK_PRINTF(2,3);
+
+int control_event_general_error(const char *format, ...)
+  CHECK_PRINTF(1,2);
+int control_event_client_error(const char *format, ...)
+  CHECK_PRINTF(1,2);
+int control_event_server_error(const char *format, ...)
+  CHECK_PRINTF(1,2);
+
 int control_event_guard(const char *nickname, const char *digest,
                         const char *status);
 int control_event_conf_changed(const smartlist_t *elements);
@@ -203,18 +213,13 @@ void control_free_all(void);
 /* Used only by control.c and test.c */
 STATIC size_t write_escaped_data(const char *data, size_t len, char **out);
 STATIC size_t read_escaped_data(const char *data, size_t len, char **out);
-/** Flag for event_format_t.  Indicates that we should use the one standard
-    format.  (Other formats previous existed, and are now deprecated)
- */
-#define ALL_FORMATS 1
-/** Bit field of flags to select how to format a controller event.  Recognized
- * flag is ALL_FORMATS. */
-typedef int event_format_t;
 
 #ifdef TOR_UNIT_TESTS
 MOCK_DECL(STATIC void,
-send_control_event_string,(uint16_t event, event_format_t which,
-                           const char *msg));
+          send_control_event_string,(uint16_t event, const char *msg));
+
+MOCK_DECL(STATIC void,
+          queue_control_event_string,(uint16_t event, char *msg));
 
 void control_testing_set_global_event_mask(uint64_t mask);
 #endif

src/or/main.c

@@ -1007,7 +1007,7 @@ directory_all_unreachable_cb(evutil_socket_t fd, short event, void *arg)
     connection_mark_unattached_ap(entry_conn,
                                   END_STREAM_REASON_NET_UNREACHABLE);
   }
-  control_event_general_status(LOG_ERR, "DIR_ALL_UNREACHABLE");
+  control_event_general_error("DIR_ALL_UNREACHABLE");
 }
 
 static struct event *directory_all_unreachable_cb_event = NULL;

src/test/test_controller_events.c

@@ -395,12 +395,12 @@ test_cntev_event_mask(void *arg)
   { #name, test_cntev_ ## name, flags, 0, NULL }
 
 struct testcase_t controller_event_tests[] = {
-  TEST(bucket_note_empty, 0),
-  TEST(bucket_millis_empty, 0),
-  TEST(sum_up_cell_stats, 0),
-  TEST(append_cell_stats, 0),
-  TEST(format_cell_stats, 0),
-  TEST(event_mask, 0),
+  TEST(bucket_note_empty, TT_FORK),
+  TEST(bucket_millis_empty, TT_FORK),
+  TEST(sum_up_cell_stats, TT_FORK),
+  TEST(append_cell_stats, TT_FORK),
+  TEST(format_cell_stats, TT_FORK),
+  TEST(event_mask, TT_FORK),
   END_OF_TESTCASES
 };
 

src/test/test_hs.c

@@ -102,13 +102,11 @@ static char *received_msg = NULL;
 /** Mock function for send_control_event_string
  */
 static void
-send_control_event_string_replacement(uint16_t event, event_format_t which,
-                                      const char *msg)
+queue_control_event_string_replacement(uint16_t event, char *msg)
 {
   (void) event;
-  (void) which;
   tor_free(received_msg);
-  received_msg = tor_strdup(msg);
+  received_msg = msg;
 }
 
 /** Mock function for node_describe_longname_by_id, it returns either
@@ -141,8 +139,8 @@ test_hs_desc_event(void *arg)
   char desc_id_base32[REND_DESC_ID_V2_LEN_BASE32 + 1];
 
   (void) arg;
-  MOCK(send_control_event_string,
-       send_control_event_string_replacement);
+  MOCK(queue_control_event_string,
+       queue_control_event_string_replacement);
   MOCK(node_describe_longname_by_id,
        node_describe_longname_by_id_replacement);
 
@@ -225,7 +223,7 @@ test_hs_desc_event(void *arg)
   smartlist_free(rend_query.hsdirs_fp);
 
  done:
-  UNMOCK(send_control_event_string);
+  UNMOCK(queue_control_event_string);
   UNMOCK(node_describe_longname_by_id);
   tor_free(received_msg);
 }

src/test/test_pt.c

@@ -333,15 +333,13 @@ static uint16_t controlevent_event = 0;
 static smartlist_t *controlevent_msgs = NULL;
 
 static void
-send_control_event_string_replacement(uint16_t event, event_format_t which,
-                                      const char *msg)
+queue_control_event_string_replacement(uint16_t event, char *msg)
 {
-  (void) which;
   ++controlevent_n;
   controlevent_event = event;
   if (!controlevent_msgs)
     controlevent_msgs = smartlist_new();
-  smartlist_add(controlevent_msgs, tor_strdup(msg));
+  smartlist_add(controlevent_msgs, msg);
 }
 
 /* Test the configure_proxy() function. */
@@ -360,8 +358,8 @@ test_pt_configure_proxy(void *arg)
        tor_process_handle_destroy_replacement);
   MOCK(get_or_state,
        get_or_state_replacement);
-  MOCK(send_control_event_string,
-       send_control_event_string_replacement);
+  MOCK(queue_control_event_string,
+       queue_control_event_string_replacement);
 
   control_testing_set_global_event_mask(EVENT_TRANSPORT_LAUNCHED);
 
@@ -435,7 +433,7 @@ test_pt_configure_proxy(void *arg)
   UNMOCK(tor_get_lines_from_handle);
   UNMOCK(tor_process_handle_destroy);
   UNMOCK(get_or_state);
-  UNMOCK(send_control_event_string);
+  UNMOCK(queue_control_event_string);
   if (controlevent_msgs) {
     SMARTLIST_FOREACH(controlevent_msgs, char *, cp, tor_free(cp));
     smartlist_free(controlevent_msgs);

src/test/test_threads.c

@@ -28,7 +28,7 @@ static unsigned long thread_fn_tid1, thread_fn_tid2;
 static void thread_test_func_(void* _s) ATTR_NORETURN;
 
 /** How many iterations have the threads in the unit test run? */
-static int t1_count = 0, t2_count = 0;
+static tor_threadlocal_t count;
 
 /** Helper function for threading unit tests: This function runs in a
  * subthread. It grabs its own mutex (start1 or start2) to make sure that it
@@ -38,19 +38,19 @@ static void
 thread_test_func_(void* _s)
 {
   char *s = _s;
-  int i, *count;
+  int i;
   tor_mutex_t *m;
   char buf[64];
   char **cp;
+  int *mycount = tor_malloc_zero(sizeof(int));
+  tor_threadlocal_set(&count, mycount);
   if (!strcmp(s, "thread 1")) {
     m = thread_test_start1_;
     cp = &thread1_name_;
-    count = &t1_count;
     thread_fn_tid1 = tor_get_thread_id();
   } else {
     m = thread_test_start2_;
     cp = &thread2_name_;
-    count = &t2_count;
     thread_fn_tid2 = tor_get_thread_id();
   }
 
@@ -62,8 +62,10 @@ thread_test_func_(void* _s)
   for (i=0; i<10000; ++i) {
     tor_mutex_acquire(thread_test_mutex_);
     strmap_set(thread_test_strmap_, "last to run", *cp);
-    ++*count;
     tor_mutex_release(thread_test_mutex_);
+    int *tls_count = tor_threadlocal_get(&count);
+    tor_assert(tls_count == mycount);
+    ++*tls_count;
   }
   tor_mutex_acquire(thread_test_mutex_);
   strmap_set(thread_test_strmap_, s, *cp);
@@ -89,6 +91,7 @@ test_threads_basic(void *arg)
   tv.tv_usec=100*1000;
 #endif
   (void) arg;
+  tt_int_op(tor_threadlocal_init(&count), OP_EQ, 0);
 
   set_main_thread();
 
@@ -128,7 +131,6 @@ test_threads_basic(void *arg)
   tor_mutex_free(thread_test_mutex_);
 
   if (timedout) {
-    printf("\nTimed out: %d %d", t1_count, t2_count);
     tt_assert(strmap_get(thread_test_strmap_, "thread 1"));
     tt_assert(strmap_get(thread_test_strmap_, "thread 2"));
     tt_assert(!timedout);

src/test/testing_common.c

@@ -14,6 +14,7 @@ const char tor_git_revision[] = "";
 
 #include "orconfig.h"
 #include "or.h"
+#include "control.h"
 #include "config.h"
 #include "rephist.h"
 #include "backtrace.h"
@@ -237,6 +238,7 @@ main(int c, const char **v)
   update_approx_time(time(NULL));
   options = options_new();
   tor_threads_init();
+  control_initialize_event_queue();
   init_logging(1);
   configure_backtrace_handler(get_version());