Split listener configuration out of options_act_reversible()

src/app/config/config.c

@@ -864,6 +864,9 @@ static setopt_err_t options_validate_and_set(const or_options_t *old_options,
                                              char **msg_out);
 struct log_transaction_t;
 static void options_rollback_log_transaction(struct log_transaction_t *xn);
+struct listener_transaction_t;
+static void options_rollback_listener_transaction(
+                                           struct listener_transaction_t *xn);
 
 /** Magic value for or_options_t. */
 #define OR_OPTIONS_MAGIC 9090909
@@ -1568,6 +1571,180 @@ options_create_directories(char **msg_out)
   return 0;
 }
 
+/** Structure to represent an incomplete configuration of a set of
+ * listeners.
+ *
+ * This structure is generated by options_start_listener_transaction(), and is
+ * either committed by options_commit_listener_transaction() or rolled back by
+ * options_rollback_listener_transaction(). */
+typedef struct listener_transaction_t {
+  bool set_conn_limit; /**< True if we've set the connection limit */
+  unsigned old_conn_limit; /**< If nonzero, previous connlimit value. */
+  smartlist_t *new_listeners; /**< List of new listeners that we opened. */
+} listener_transaction_t;
+
+/**
+ * Start configuring our listeners based on the current value of
+ * get_options().
+ *
+ * The value <b>old_options</b> holds either the previous options object,
+ * or NULL if we're starting for the first time.
+ *
+ * On success, return a listener_transaction_t that we can either roll back or
+ * commit.
+ *
+ * On failure return NULL and write a message into a newly allocated string in
+ * *<b>msg_out</b>.
+ **/
+static listener_transaction_t *
+options_start_listener_transaction(const or_options_t *old_options,
+                                   char **msg_out)
+{
+  listener_transaction_t *xn = tor_malloc_zero(sizeof(listener_transaction_t));
+  xn->new_listeners = smartlist_new();
+  or_options_t *options = get_options_mutable();
+  const bool running_tor = options->command == CMD_RUN_TOR;
+
+  if (! running_tor) {
+    return xn;
+  }
+
+  int n_ports=0;
+  /* We need to set the connection limit before we can open the listeners. */
+  if (! sandbox_is_active()) {
+    if (set_max_file_descriptors((unsigned)options->ConnLimit,
+                                 &options->ConnLimit_) < 0) {
+      *msg_out = tor_strdup("Problem with ConnLimit value. "
+                            "See logs for details.");
+      goto rollback;
+    }
+    xn->set_conn_limit = true;
+    if (old_options)
+      xn->old_conn_limit = (unsigned)old_options->ConnLimit;
+  } else {
+    tor_assert(old_options);
+    options->ConnLimit_ = old_options->ConnLimit_;
+  }
+
+  /* Adjust the port configuration so we can launch listeners. */
+  /* 31851: some ports are relay-only */
+  if (parse_ports(options, 0, msg_out, &n_ports, NULL)) {
+    if (!*msg_out)
+      *msg_out = tor_strdup("Unexpected problem parsing port config");
+    goto rollback;
+  }
+
+  /* Set the hibernation state appropriately.*/
+  consider_hibernation(time(NULL));
+
+  /* Launch the listeners.  (We do this before we setuid, so we can bind to
+   * ports under 1024.)  We don't want to rebind if we're hibernating or
+   * shutting down. If networking is disabled, this will close all but the
+   * control listeners, but disable those. */
+  /* 31851: some listeners are relay-only */
+  if (!we_are_hibernating()) {
+    if (retry_all_listeners(xn->new_listeners,
+                            options->DisableNetwork) < 0) {
+      *msg_out = tor_strdup("Failed to bind one of the listener ports.");
+      goto rollback;
+    }
+  }
+  if (options->DisableNetwork) {
+    /* Aggressively close non-controller stuff, NOW */
+    log_notice(LD_NET, "DisableNetwork is set. Tor will not make or accept "
+               "non-control network connections. Shutting down all existing "
+               "connections.");
+    connection_mark_all_noncontrol_connections();
+    /* We can't complete circuits until the network is re-enabled. */
+    note_that_we_maybe_cant_complete_circuits();
+  }
+
+#if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H)
+  /* Open /dev/pf before (possibly) dropping privileges. */
+  if (options->TransPort_set &&
+      options->TransProxyType_parsed == TPT_DEFAULT) {
+    if (get_pf_socket() < 0) {
+      *msg_out = tor_strdup("Unable to open /dev/pf for transparent proxy.");
+      goto rollback;
+    }
+  }
+#endif /* defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) */
+
+  return xn;
+
+ rollback:
+  options_rollback_listener_transaction(xn);
+  return NULL;
+}
+
+/**
+ * Finish configuring the listeners that started to get configured with
+ * <b>xn</b>.  Frees <b>xn</b>.
+ **/
+static void
+options_commit_listener_transaction(listener_transaction_t *xn)
+{
+  tor_assert(xn);
+  if (xn->set_conn_limit) {
+    or_options_t *options = get_options_mutable();
+    /*
+     * If we adjusted the conn limit, recompute the OOS threshold too
+     *
+     * How many possible sockets to keep in reserve?  If we have lots of
+     * possible sockets, keep this below a limit and set ConnLimit_high_thresh
+     * very close to ConnLimit_, but if ConnLimit_ is low, shrink it in
+     * proportion.
+     *
+     * Somewhat arbitrarily, set socks_in_reserve to 5% of ConnLimit_, but
+     * cap it at 64.
+     */
+    int socks_in_reserve = options->ConnLimit_ / 20;
+    if (socks_in_reserve > 64) socks_in_reserve = 64;
+
+    options->ConnLimit_high_thresh = options->ConnLimit_ - socks_in_reserve;
+    options->ConnLimit_low_thresh = (options->ConnLimit_ / 4) * 3;
+    log_info(LD_GENERAL,
+             "Recomputed OOS thresholds: ConnLimit %d, ConnLimit_ %d, "
+             "ConnLimit_high_thresh %d, ConnLimit_low_thresh %d",
+             options->ConnLimit, options->ConnLimit_,
+             options->ConnLimit_high_thresh,
+             options->ConnLimit_low_thresh);
+
+    /* Give the OOS handler a chance with the new thresholds */
+    connection_check_oos(get_n_open_sockets(), 0);
+  }
+
+  smartlist_free(xn->new_listeners);
+  tor_free(xn);
+}
+
+/**
+ * Revert the listener configuration changes that that started to get
+ * configured with <b>xn</b>.  Frees <b>xn</b>.
+ **/
+static void
+options_rollback_listener_transaction(listener_transaction_t *xn)
+{
+  if (! xn)
+    return;
+
+  or_options_t *options = get_options_mutable();
+
+  if (xn->set_conn_limit && xn->old_conn_limit)
+    set_max_file_descriptors(xn->old_conn_limit, &options->ConnLimit_);
+
+  SMARTLIST_FOREACH(xn->new_listeners, connection_t *, conn,
+  {
+    log_notice(LD_NET, "Closing partially-constructed %s on %s:%d",
+               conn_type_to_string(conn->type), conn->address, conn->port);
+    connection_close_immediate(conn);
+    connection_mark_for_close(conn);
+  });
+
+  smartlist_free(xn->new_listeners);
+  tor_free(xn);
+}
+
 /** Structure to represent an incompleted configuration of a set of logs.
  *
  * This structure is generated by options_start_log_transaction(), and is
@@ -1719,75 +1896,16 @@ MOCK_IMPL(STATIC int,
 options_act_reversible,(const or_options_t *old_options, char **msg))
 {
   const bool first_time = ! have_set_startup_options;
-  smartlist_t *new_listeners = smartlist_new();
-  or_options_t *options = get_options_mutable();
-  int running_tor = options->command == CMD_RUN_TOR;
   log_transaction_t *log_transaction = NULL;
-  int set_conn_limit = 0;
+  listener_transaction_t *listener_transaction = NULL;
   int r = -1;
 
   if (options_act_once_on_startup(msg) < 0)
     goto rollback;
 
-  if (running_tor) {
+  listener_transaction = options_start_listener_transaction(old_options, msg);
-    int n_ports=0;
+  if (listener_transaction == NULL)
-    /* We need to set the connection limit before we can open the listeners. */
-    if (! sandbox_is_active()) {
-      if (set_max_file_descriptors((unsigned)options->ConnLimit,
-                                   &options->ConnLimit_) < 0) {
-        *msg = tor_strdup("Problem with ConnLimit value. "
-                          "See logs for details.");
     goto rollback;
-      }
-      set_conn_limit = 1;
-    } else {
-      tor_assert(old_options);
-      options->ConnLimit_ = old_options->ConnLimit_;
-    }
-
-    /* Adjust the port configuration so we can launch listeners. */
-    /* 31851: some ports are relay-only */
-    if (parse_ports(options, 0, msg, &n_ports, NULL)) {
-      if (!*msg)
-        *msg = tor_strdup("Unexpected problem parsing port config");
-      goto rollback;
-    }
-
-    /* Set the hibernation state appropriately.*/
-    consider_hibernation(time(NULL));
-
-    /* Launch the listeners.  (We do this before we setuid, so we can bind to
-     * ports under 1024.)  We don't want to rebind if we're hibernating or
-     * shutting down. If networking is disabled, this will close all but the
-     * control listeners, but disable those. */
-    /* 31851: some listeners are relay-only */
-    if (!we_are_hibernating()) {
-      if (retry_all_listeners(new_listeners, options->DisableNetwork) < 0) {
-        *msg = tor_strdup("Failed to bind one of the listener ports.");
-        goto rollback;
-      }
-    }
-    if (options->DisableNetwork) {
-      /* Aggressively close non-controller stuff, NOW */
-      log_notice(LD_NET, "DisableNetwork is set. Tor will not make or accept "
-                 "non-control network connections. Shutting down all existing "
-                 "connections.");
-      connection_mark_all_noncontrol_connections();
-      /* We can't complete circuits until the network is re-enabled. */
-      note_that_we_maybe_cant_complete_circuits();
-    }
-  }
-
-#if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H)
-  /* Open /dev/pf before dropping privileges. */
-  if (options->TransPort_set &&
-      options->TransProxyType_parsed == TPT_DEFAULT) {
-    if (get_pf_socket() < 0) {
-      *msg = tor_strdup("Unable to open /dev/pf for transparent proxy.");
-      goto rollback;
-    }
-  }
-#endif /* defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) */
 
   if (first_time) {
     if (options_switch_id(msg) < 0)
@@ -1808,33 +1926,7 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
 
   options_commit_log_transaction(log_transaction);
 
-  if (set_conn_limit) {
+  options_commit_listener_transaction(listener_transaction);
-    /*
-     * If we adjusted the conn limit, recompute the OOS threshold too
-     *
-     * How many possible sockets to keep in reserve?  If we have lots of
-     * possible sockets, keep this below a limit and set ConnLimit_high_thresh
-     * very close to ConnLimit_, but if ConnLimit_ is low, shrink it in
-     * proportion.
-     *
-     * Somewhat arbitrarily, set socks_in_reserve to 5% of ConnLimit_, but
-     * cap it at 64.
-     */
-    int socks_in_reserve = options->ConnLimit_ / 20;
-    if (socks_in_reserve > 64) socks_in_reserve = 64;
-
-    options->ConnLimit_high_thresh = options->ConnLimit_ - socks_in_reserve;
-    options->ConnLimit_low_thresh = (options->ConnLimit_ / 4) * 3;
-    log_info(LD_GENERAL,
-             "Recomputed OOS thresholds: ConnLimit %d, ConnLimit_ %d, "
-             "ConnLimit_high_thresh %d, ConnLimit_low_thresh %d",
-             options->ConnLimit, options->ConnLimit_,
-             options->ConnLimit_high_thresh,
-             options->ConnLimit_low_thresh);
-
-    /* Give the OOS handler a chance with the new thresholds */
-    connection_check_oos(get_n_open_sockets(), 0);
-  }
 
   goto done;
 
@@ -1843,21 +1935,9 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
   tor_assert(*msg);
 
   options_rollback_log_transaction(log_transaction);
-
+  options_rollback_listener_transaction(listener_transaction);
-  if (set_conn_limit && old_options)
-    set_max_file_descriptors((unsigned)old_options->ConnLimit,
-                             &options->ConnLimit_);
-
-  SMARTLIST_FOREACH(new_listeners, connection_t *, conn,
-  {
-    log_notice(LD_NET, "Closing partially-constructed %s on %s:%d",
-               conn_type_to_string(conn->type), conn->address, conn->port);
-    connection_close_immediate(conn);
-    connection_mark_for_close(conn);
-  });
 
  done:
-  smartlist_free(new_listeners);
   return r;
 }