1 files changed, 94 insertions, 78 deletions

diff --git a/src/or/geoip.c b/src/or/geoip.c
index 654241c1c0..28f570a966 100644
--- a/src/or/geoip.c
+++ b/src/or/geoip.c
@@ -1083,14 +1083,14 @@ geoip_bridge_stats_term(void)
   start_of_bridge_stats_interval = 0;
 }
 
-/** Parse the bridge statistics as they are written to extra-info
- * descriptors for being returned to controller clients. Return the
- * controller string if successful, or NULL otherwise. */
-static char *
-parse_bridge_stats_controller(const char *stats_str, time_t now)
+/** Validate a bridge statistics string as it would be written to a
+ * current extra-info descriptor. Return 1 if the string is valid and
+ * recent enough, or 0 otherwise. */
+static int
+validate_bridge_stats(const char *stats_str, time_t now)
 {
   char stats_end_str[ISO_TIME_LEN+1], stats_start_str[ISO_TIME_LEN+1],
-       *controller_str, *eos, *eol, *summary;
+       *eos;
 
   const char *BRIDGE_STATS_END = "bridge-stats-end ";
   const char *BRIDGE_IPS = "bridge-ips ";
@@ -1104,63 +1104,90 @@ parse_bridge_stats_controller(const char *stats_str, time_t now)
     "bridge-stats-end YYYY-MM-DD HH:MM:SS (N s)" */
   tmp = find_str_at_start_of_line(stats_str, BRIDGE_STATS_END);
   if (!tmp)
-    return NULL;
+    return 0;
   tmp += strlen(BRIDGE_STATS_END);
 
   if (strlen(tmp) < ISO_TIME_LEN + 6)
-    return NULL;
+    return 0;
   strlcpy(stats_end_str, tmp, sizeof(stats_end_str));
   if (parse_iso_time(stats_end_str, &stats_end_time) < 0)
-    return NULL;
+    return 0;
   if (stats_end_time < now - (25*60*60) ||
       stats_end_time > now + (1*60*60))
-    return NULL;
+    return 0;
   seconds = (int)strtol(tmp + ISO_TIME_LEN + 2, &eos, 10);
   if (!eos || seconds < 23*60*60)
-    return NULL;
+    return 0;
   format_iso_time(stats_start_str, stats_end_time - seconds);
 
   /* Parse: "bridge-ips CC=N,CC=N,..." */
   tmp = find_str_at_start_of_line(stats_str, BRIDGE_IPS);
-  if (tmp) {
-    tmp += strlen(BRIDGE_IPS);
-    tmp = eat_whitespace_no_nl(tmp);
-    eol = strchr(tmp, '\n');
-    if (eol)
-      summary = tor_strndup(tmp, eol-tmp);
-    else
-      summary = tor_strdup(tmp);
-  } else {
+  if (!tmp) {
     /* Look if there is an empty "bridge-ips" line */
     tmp = find_str_at_start_of_line(stats_str, BRIDGE_IPS_EMPTY_LINE);
     if (!tmp)
-      return NULL;
-    summary = tor_strdup("");
+      return 0;
   }
 
-  tor_asprintf(&controller_str,
-               "TimeStarted=\"%s\" CountrySummary=%s",
-               stats_start_str, summary);
-  tor_free(summary);
-  return controller_str;
+  return 1;
 }
 
 /** Most recent bridge statistics formatted to be written to extra-info
  * descriptors. */
 static char *bridge_stats_extrainfo = NULL;
 
-/** Most recent bridge statistics formatted to be returned to controller
- * clients. */
-static char *bridge_stats_controller = NULL;
+/** Return a newly allocated string holding our bridge usage stats by country
+ * in a format suitable for inclusion in an extrainfo document. Return NULL on
+ * failure.  */
+static char *
+format_bridge_stats_extrainfo(time_t now)
+{
+  char *out = NULL, *data = NULL;
+  long duration = now - start_of_bridge_stats_interval;
+  char written[ISO_TIME_LEN+1];
+
+  if (duration < 0)
+    return NULL;
+
+  format_iso_time(written, now);
+  data = geoip_get_client_history(GEOIP_CLIENT_CONNECT);
+
+  tor_asprintf(&out,
+               "bridge-stats-end %s (%ld s)\n"
+               "bridge-ips %s\n",
+               written, duration,
+               data ? data : "");
+  tor_free(data);
+
+  return out;
+}
+
+/** Return a newly allocated string holding our bridge usage stats by country
+ * in a format suitable for the answer to a controller request. Return NULL on
+ * failure.  */
+static char *
+format_bridge_stats_controller(time_t now)
+{
+  char *out = NULL, *data = NULL;
+  char started[ISO_TIME_LEN+1];
+  (void) now;
+
+  format_iso_time(started, start_of_bridge_stats_interval);
+  data = geoip_get_client_history(GEOIP_CLIENT_CONNECT);
+
+  tor_asprintf(&out,
+               "TimeStarted=\"%s\" CountrySummary=%s",
+               started, data ? data : "");
+  tor_free(data);
+  return out;
+}
 
 /** Write bridge statistics to $DATADIR/stats/bridge-stats and return
  * when we should next try to write statistics. */
 time_t
 geoip_bridge_stats_write(time_t now)
 {
-  char *statsdir = NULL, *filename = NULL, *data = NULL,
-       written[ISO_TIME_LEN+1], *out = NULL, *controller_str;
-  size_t len;
+  char *filename = NULL, *val = NULL, *statsdir = NULL;
 
   /* Check if 24 hours have passed since starting measurements. */
   if (now < start_of_bridge_stats_interval + WRITE_STATS_INTERVAL)
@@ -1169,64 +1196,54 @@ geoip_bridge_stats_write(time_t now)
   /* Discard all items in the client history that are too old. */
   geoip_remove_old_clients(start_of_bridge_stats_interval);
 
+  /* Generate formatted string */
+  val = format_bridge_stats_extrainfo(now);
+  if (val == NULL)
+    goto done;
+
+  /* Update the stored value. */
+  tor_free(bridge_stats_extrainfo);
+  bridge_stats_extrainfo = val;
+  start_of_bridge_stats_interval = now;
+
+  /* Write it to disk. */
   statsdir = get_datadir_fname("stats");
   if (check_private_dir(statsdir, CPD_CREATE) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "bridge-stats");
-  data = geoip_get_client_history(GEOIP_CLIENT_CONNECT);
-  format_iso_time(written, now);
-  len = strlen("bridge-stats-end  (999999 s)\nbridge-ips \n") +
-        ISO_TIME_LEN + (data ? strlen(data) : 0) + 42;
-  out = tor_malloc(len);
-  if (tor_snprintf(out, len, "bridge-stats-end %s (%u s)\nbridge-ips %s\n",
-              written, (unsigned) (now - start_of_bridge_stats_interval),
-              data ? data : "") < 0)
-    goto done;
-  write_str_to_file(filename, out, 0);
-  controller_str = parse_bridge_stats_controller(out, now);
-  if (!controller_str)
-    goto done;
-  start_of_bridge_stats_interval = now;
-  tor_free(bridge_stats_extrainfo);
-  tor_free(bridge_stats_controller);
-  bridge_stats_extrainfo = out;
-  out = NULL;
-  bridge_stats_controller = controller_str;
-  control_event_clients_seen(controller_str);
+
+  write_str_to_file(filename, bridge_stats_extrainfo, 0);
+
+  /* Tell the controller, "hey, there are clients!" */
+  {
+    char *controller_str = format_bridge_stats_controller(now);
+    if (controller_str)
+      control_event_clients_seen(controller_str);
+    tor_free(controller_str);
+  }
  done:
   tor_free(filename);
   tor_free(statsdir);
-  tor_free(data);
-  tor_free(out);
-  return start_of_bridge_stats_interval +
-         WRITE_STATS_INTERVAL;
+
+  return start_of_bridge_stats_interval + WRITE_STATS_INTERVAL;
 }
 
 /** Try to load the most recent bridge statistics from disk, unless we
- * have finished a measurement interval lately. */
+ * have finished a measurement interval lately, and check whether they
+ * are still recent enough. */
 static void
 load_bridge_stats(time_t now)
 {
-  char *statsdir, *fname=NULL, *contents, *controller_str;
+  char *fname, *contents;
   if (bridge_stats_extrainfo)
     return;
-  statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
-    goto done;
+
   fname = get_datadir_fname2("stats", "bridge-stats");
   contents = read_file_to_str(fname, RFTS_IGNORE_MISSING, NULL);
-  if (contents) {
-    controller_str = parse_bridge_stats_controller(contents, now);
-    if (controller_str) {
-      bridge_stats_extrainfo = contents;
-      bridge_stats_controller = controller_str;
-    } else {
-      tor_free(contents);
-    }
-  }
- done:
+  if (contents && validate_bridge_stats(contents, now))
+    bridge_stats_extrainfo = contents;
+
   tor_free(fname);
-  tor_free(statsdir);
 }
 
 /** Return most recent bridge statistics for inclusion in extra-info
@@ -1238,13 +1255,12 @@ geoip_get_bridge_stats_extrainfo(time_t now)
   return bridge_stats_extrainfo;
 }
 
-/** Return most recent bridge statistics to be returned to controller
- * clients, or NULL if we don't have recent bridge statistics. */
-const char *
+/** Return a new string containing the recent bridge statistics to be returned
+ * to controller clients, or NULL if we don't have any bridge statistics. */
+char *
 geoip_get_bridge_stats_controller(time_t now)
 {
-  load_bridge_stats(now);
-  return bridge_stats_controller;
+  return format_bridge_stats_controller(now);
 }
 
 /** Start time of entry stats or 0 if we're not collecting entry