Split log configuration out of options_act_reversible().

scripts/maint/practracker/exceptions.txt

@@ -33,7 +33,7 @@
 #
 # Remember: It is better to fix the problem than to add a new exception!
 
-problem file-size /src/app/config/config.c 7212
+problem file-size /src/app/config/config.c 7400
 problem include-count /src/app/config/config.c 80
 problem function-size /src/app/config/config.c:options_act_reversible() 298
 problem function-size /src/app/config/config.c:options_act() 381

src/app/config/config.c

@@ -862,6 +862,8 @@ static void options_clear_cb(const config_mgr_t *mgr, void *opts);
 static setopt_err_t options_validate_and_set(const or_options_t *old_options,
                                              or_options_t *new_options,
                                              char **msg_out);
+struct log_transaction_t;
+static void options_rollback_log_transaction(struct log_transaction_t *xn);
 
 /** Magic value for or_options_t. */
 #define OR_OPTIONS_MAGIC 9090909
@@ -1566,6 +1568,139 @@ options_create_directories(char **msg_out)
   return 0;
 }
 
+/** Structure to represent an incompleted configuration of a set of logs.
+ *
+ * This structure is generated by options_start_log_transaction(), and is
+ * either committed by options_commit_log_transaction() or rolled back by
+ * options_rollback_log_transaction(). */
+typedef struct log_transaction_t {
+  /** Previous lowest severity of any configured log. */
+  int old_min_log_level;
+  /** True if we have marked the previous logs to be closed */
+  bool logs_marked;
+  /** True if we initialized the new set of logs */
+  bool logs_initialized;
+  /** True if our safelogging configuration is different from what it was
+   * previously (or if we are starting for the first time). */
+  bool safelogging_changed;
+} log_transaction_t;
+
+/**
+ * Start configuring our logs 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 log_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 log_transaction_t *
+options_start_log_transaction(const or_options_t *old_options,
+                              char **msg_out)
+{
+  const or_options_t *options = get_options();
+  const bool running_tor = options->command == CMD_RUN_TOR;
+
+  log_transaction_t *xn = tor_malloc_zero(sizeof(log_transaction_t));
+  xn->old_min_log_level = get_min_log_level();
+
+  if (! running_tor)
+    goto done;
+
+  mark_logs_temp(); /* Close current logs once new logs are open. */
+  xn->logs_marked = true;
+  /* Configure the tor_log(s) */
+  if (options_init_logs(old_options, options, 0)<0) {
+    *msg_out = tor_strdup("Failed to init Log options. See logs for details.");
+    options_rollback_log_transaction(xn);
+    xn = NULL;
+    goto done;
+  }
+
+  xn->safelogging_changed = !old_options ||
+    old_options->SafeLogging_ != options->SafeLogging_;
+
+  xn->logs_initialized = true;
+
+ done:
+  return xn;
+}
+
+/**
+ * Finish configuring the logs that started to get configured with <b>xn</b>.
+ * Frees <b>xn</b>.
+ **/
+static void
+options_commit_log_transaction(log_transaction_t *xn)
+{
+  const or_options_t *options = get_options();
+  tor_assert(xn);
+
+  if (xn->logs_marked) {
+    log_severity_list_t *severity =
+      tor_malloc_zero(sizeof(log_severity_list_t));
+    close_temp_logs();
+    add_callback_log(severity, control_event_logmsg);
+    logs_set_pending_callback_callback(control_event_logmsg_pending);
+    control_adjust_event_log_severity();
+    tor_free(severity);
+    tor_log_update_sigsafe_err_fds();
+  }
+
+  if (xn->logs_initialized) {
+    flush_log_messages_from_startup();
+  }
+
+  {
+    const char *badness = NULL;
+    int bad_safelog = 0, bad_severity = 0, new_badness = 0;
+    if (options->SafeLogging_ != SAFELOG_SCRUB_ALL) {
+      bad_safelog = 1;
+      if (xn->safelogging_changed)
+        new_badness = 1;
+    }
+    if (get_min_log_level() >= LOG_INFO) {
+      bad_severity = 1;
+      if (get_min_log_level() != xn->old_min_log_level)
+        new_badness = 1;
+    }
+    if (bad_safelog && bad_severity)
+      badness = "you disabled SafeLogging, and "
+        "you're logging more than \"notice\"";
+    else if (bad_safelog)
+      badness = "you disabled SafeLogging";
+    else
+      badness = "you're logging more than \"notice\"";
+    if (new_badness)
+      log_warn(LD_GENERAL, "Your log may contain sensitive information - %s. "
+               "Don't log unless it serves an important reason. "
+               "Overwrite the log afterwards.", badness);
+  }
+
+  tor_free(xn);
+}
+
+/**
+ * Revert the log configuration changes that that started to get configured
+ * with <b>xn</b>.  Frees <b>xn</b>.
+ **/
+static void
+options_rollback_log_transaction(log_transaction_t *xn)
+{
+  if (!xn)
+    return;
+
+  if (xn->logs_marked) {
+    rollback_log_changes();
+    control_adjust_event_log_severity();
+  }
+
+  tor_free(xn);
+}
+
 /** Fetch the active option list, and take actions based on it. All of the
  * things we do should survive being done repeatedly.  If present,
  * <b>old_options</b> contains the previous value of the options.
@@ -1587,10 +1722,9 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
   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;
   int r = -1;
-  int logs_marked = 0, logs_initialized = 0;
-  int old_min_log_level = get_min_log_level();
 
   if (options_act_once_on_startup(msg) < 0)
     goto rollback;
@@ -1663,62 +1797,16 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
       goto done;
   }
 
-
   /* Bail out at this point if we're not going to be a client or server:
    * we don't run Tor itself. */
-  if (!running_tor)
+  log_transaction = options_start_log_transaction(old_options, msg);
-    goto commit;
+  if (log_transaction == NULL)
-
-  mark_logs_temp(); /* Close current logs once new logs are open. */
-  logs_marked = 1;
-  /* Configure the tor_log(s) */
-  if (options_init_logs(old_options, options, 0)<0) {
-    *msg = tor_strdup("Failed to init Log options. See logs for details.");
     goto rollback;
-  }
-  logs_initialized = 1;
 
- commit:
+  // Commit!
   r = 0;
-  if (logs_marked) {
-    log_severity_list_t *severity =
-      tor_malloc_zero(sizeof(log_severity_list_t));
-    close_temp_logs();
-    add_callback_log(severity, control_event_logmsg);
-    logs_set_pending_callback_callback(control_event_logmsg_pending);
-    control_adjust_event_log_severity();
-    tor_free(severity);
-    tor_log_update_sigsafe_err_fds();
-  }
-  if (logs_initialized) {
-    flush_log_messages_from_startup();
-  }
 
-  {
+  options_commit_log_transaction(log_transaction);
-    const char *badness = NULL;
-    int bad_safelog = 0, bad_severity = 0, new_badness = 0;
-    if (options->SafeLogging_ != SAFELOG_SCRUB_ALL) {
-      bad_safelog = 1;
-      if (!old_options || old_options->SafeLogging_ != options->SafeLogging_)
-        new_badness = 1;
-    }
-    if (get_min_log_level() >= LOG_INFO) {
-      bad_severity = 1;
-      if (get_min_log_level() != old_min_log_level)
-        new_badness = 1;
-    }
-    if (bad_safelog && bad_severity)
-      badness = "you disabled SafeLogging, and "
-        "you're logging more than \"notice\"";
-    else if (bad_safelog)
-      badness = "you disabled SafeLogging";
-    else
-      badness = "you're logging more than \"notice\"";
-    if (new_badness)
-      log_warn(LD_GENERAL, "Your log may contain sensitive information - %s. "
-               "Don't log unless it serves an important reason. "
-               "Overwrite the log afterwards.", badness);
-  }
 
   if (set_conn_limit) {
     /*
@@ -1754,10 +1842,7 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
   r = -1;
   tor_assert(*msg);
 
-  if (logs_marked) {
+  options_rollback_log_transaction(log_transaction);
-    rollback_log_changes();
-    control_adjust_event_log_severity();
-  }
 
   if (set_conn_limit && old_options)
     set_max_file_descriptors((unsigned)old_options->ConnLimit,
@@ -4857,7 +4942,7 @@ options_init_log_granularity(const or_options_t *options,
  * Initialize the logs based on the configuration file.
  */
 STATIC int
-options_init_logs(const or_options_t *old_options, or_options_t *options,
+options_init_logs(const or_options_t *old_options, const or_options_t *options,
                   int validate_only)
 {
   config_line_t *opt;

src/app/config/config.h

@@ -301,7 +301,7 @@ STATIC int open_and_add_file_log(const log_severity_list_t *severity,
                                  const char *fname,
                                  int truncate_log);
 STATIC int options_init_logs(const or_options_t *old_options,
-                             or_options_t *options, int validate_only);
+                             const or_options_t *options, int validate_only);
 
 #ifdef TOR_UNIT_TESTS
 int options_validate(const or_options_t *old_options,