commit 5060007f4b959c5b8cd483817969252c4e4138aa Author: Nick Mathewson nickm@torproject.org Date: Thu Nov 7 17:42:47 2019 -0500
Split log configuration out of options_act_reversible(). --- scripts/maint/practracker/exceptions.txt | 2 +- src/app/config/config.c | 199 ++++++++++++++++++++++--------- src/app/config/config.h | 2 +- 3 files changed, 144 insertions(+), 59 deletions(-)
diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index a70119e8a..1c3bf9cbe 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/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 diff --git a/src/app/config/config.c b/src/app/config/config.c index 1ef23824d..082dff313 100644 --- a/src/app/config/config.c +++ b/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) - goto commit; - - 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."); + log_transaction = options_start_log_transaction(old_options, msg); + if (log_transaction == NULL) 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(); - }
- { - 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); - } + options_commit_log_transaction(log_transaction);
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) { - rollback_log_changes(); - control_adjust_event_log_severity(); - } + options_rollback_log_transaction(log_transaction);
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; diff --git a/src/app/config/config.h b/src/app/config/config.h index eeba9e64d..0af96a0c2 100644 --- a/src/app/config/config.h +++ b/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,