Allow controllers a more up-to-date view of bridge usage.

Instead of answering GETINFO requests about our geoip usage only after
running for 24 hours, this patch makes us answer GETINFO requests
immediately.  We still round and quantize as before.

Implements bug2711.

Also, refactor the heck out of the bridge usage formatting code.  No
longer should we need to do a generate-parse-and-regenerate cycle to
get the controller string, and that lets us simplify the code a lot.

changes/feature2711

@@ -0,0 +1,4 @@
+  o Minor features
+    - Export GeoIP information on usage to bridge controller even if we have
+      not yet been running for 24 hours.
+

src/or/control.c

@@ -1785,12 +1785,12 @@ getinfo_helper_events(control_connection_t *control_conn,
                  "information", question);
       }
     } else if (!strcmp(question, "status/clients-seen")) {
-      const char *bridge_stats = geoip_get_bridge_stats_controller(time(NULL));
+      char *bridge_stats = geoip_get_bridge_stats_controller(time(NULL));
       if (!bridge_stats) {
         *errmsg = "No bridge-client stats available";
         return -1;
       }
-      *answer = tor_strdup(bridge_stats);
+      *answer = bridge_stats;
     } else {
       return 0;
     }

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

src/or/geoip.h

@@ -51,7 +51,7 @@ void geoip_bridge_stats_init(time_t now);
 time_t geoip_bridge_stats_write(time_t now);
 void geoip_bridge_stats_term(void);
 const char *geoip_get_bridge_stats_extrainfo(time_t);
-const char *geoip_get_bridge_stats_controller(time_t);
+char *geoip_get_bridge_stats_controller(time_t);
 
 #endif