Refactor circuit_build_times_parse_state

Remove the msg parameter to pass an error message out. This
wasn't needed and made it harder to detect a memory leak.

4 files changed, 21 insertions, 24 deletions

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 9749a56fec..8d6d9f47ed 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -605,11 +605,11 @@ circuit_build_times_filter_timeouts(circuit_build_times_t *cbt)
  * after we do so. Use this result to estimate parameters and
  * calculate the timeout.
  *
- * Returns -1 and sets msg on error. Msg must be freed by the caller.
+ * Return -1 on error.
  */
 int
 circuit_build_times_parse_state(circuit_build_times_t *cbt,
-                                or_state_t *state, char **msg)
+                                or_state_t *state)
 {
   int tot_values = 0;
   uint32_t loaded_cnt = 0, N = 0;
@@ -617,7 +617,7 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
   unsigned int i;
   build_time_t *loaded_times;
   circuit_build_times_init(cbt);
-  *msg = NULL;
+  int err = 0;
 
   if (circuit_build_times_disabled()) {
     return 0;
@@ -631,8 +631,9 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
     smartlist_split_string(args, line->value, " ",
                            SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0);
     if (smartlist_len(args) < 2) {
-      *msg = tor_strdup("Unable to parse circuit build times: "
-                        "Too few arguments to CircuitBuildTime");
+      log_warn(LD_GENERAL, "Unable to parse circuit build times: "
+                           "Too few arguments to CircuitBuildTime");
+      err = 1;
       SMARTLIST_FOREACH(args, char*, cp, tor_free(cp));
       smartlist_free(args);
       break;
@@ -645,8 +646,9 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
       ms = (build_time_t)tor_parse_ulong(ms_str, 0, 0,
                                          CBT_BUILD_TIME_MAX, &ok, NULL);
       if (!ok) {
-        *msg = tor_strdup("Unable to parse circuit build times: "
-                          "Unparsable bin number");
+        log_warn(LD_GENERAL, "Unable to parse circuit build times: "
+                             "Unparsable bin number");
+        err = 1;
         SMARTLIST_FOREACH(args, char*, cp, tor_free(cp));
         smartlist_free(args);
         break;
@@ -654,8 +656,9 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
       count = (uint32_t)tor_parse_ulong(count_str, 0, 0,
                                         UINT32_MAX, &ok, NULL);
       if (!ok) {
-        *msg = tor_strdup("Unable to parse circuit build times: "
-                          "Unparsable bin count");
+        log_warn(LD_GENERAL, "Unable to parse circuit build times: "
+                             "Unparsable bin count");
+        err = 1;
         SMARTLIST_FOREACH(args, char*, cp, tor_free(cp));
         smartlist_free(args);
         break;
@@ -692,11 +695,9 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
             "Corrupt state file? Build times count mismatch. "
             "Read %d times, but file says %d", loaded_cnt,
             state->TotalBuildTimes);
-    if (!*msg)
-      *msg = tor_strdup("Build times count mismatch.");
+    err = 1;
     circuit_build_times_reset(cbt);
-    tor_free(loaded_times);
-    return -1;
+    goto done;
   }
 
   circuit_build_times_shuffle_and_store_array(cbt, loaded_times, loaded_cnt);
@@ -717,11 +718,9 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
             "Corrupt state file? Shuffled build times mismatch. "
             "Read %d times, but file says %d", tot_values,
             state->TotalBuildTimes);
-    if (!*msg)
-      *msg = tor_strdup("Build times count mismatch.");
+    err = 1;
     circuit_build_times_reset(cbt);
-    tor_free(loaded_times);
-    return -1;
+    goto done;
   }
 
   circuit_build_times_set_timeout(cbt);
@@ -730,8 +729,9 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt,
     circuit_build_times_filter_timeouts(cbt);
   }
 
+done:
   tor_free(loaded_times);
-  return *msg ? -1 : 0;
+  return err ? -1 : 0;
 }
 
 /**
diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h
index 3a02f04202..d6aaef2fe9 100644
--- a/src/or/circuitbuild.h
+++ b/src/or/circuitbuild.h
@@ -81,7 +81,7 @@ extern circuit_build_times_t circ_times;
 void circuit_build_times_update_state(circuit_build_times_t *cbt,
                                       or_state_t *state);
 int circuit_build_times_parse_state(circuit_build_times_t *cbt,
-                                    or_state_t *state, char **msg);
+                                    or_state_t *state);
 void circuit_build_times_count_timeout(circuit_build_times_t *cbt,
                                        int did_onehop);
 int circuit_build_times_count_close(circuit_build_times_t *cbt,
diff --git a/src/or/config.c b/src/or/config.c
index ef2b2ddeab..e65d1329ce 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -4971,9 +4971,7 @@ or_state_set(or_state_t *new_state)
     tor_free(err);
     ret = -1;
   }
-  if (circuit_build_times_parse_state(&circ_times, global_state, &err) < 0) {
-    log_warn(LD_GENERAL,"%s",err);
-    tor_free(err);
+  if (circuit_build_times_parse_state(&circ_times, global_state) < 0) {
     ret = -1;
   }
   return ret;
diff --git a/src/test/test.c b/src/test/test.c
index 9948ecf992..6e5abcb97d 100644
--- a/src/test/test.c
+++ b/src/test/test.c
@@ -478,7 +478,6 @@ test_circuit_timeout(void)
   circuit_build_times_t final;
   double timeout1, timeout2;
   or_state_t state;
-  char *msg;
   int i, runs;
   double close_ms;
   circuit_build_times_init(&initial);
@@ -518,7 +517,7 @@ test_circuit_timeout(void)
   test_assert(estimate.total_build_times <= CBT_NCIRCUITS_TO_OBSERVE);
 
   circuit_build_times_update_state(&estimate, &state);
-  test_assert(circuit_build_times_parse_state(&final, &state, &msg) == 0);
+  test_assert(circuit_build_times_parse_state(&final, &state) == 0);
 
   circuit_build_times_update_alpha(&final);
   timeout2 = circuit_build_times_calculate_timeout(&final,