Separate generation of a dirreq-stats string from writing it to disk.

This patch separates the generation of a dirreq-stats string from
actually writing it to disk.  The new geoip_format_dirreq_stats()
generates a dirreq-stats string that geoip_dirreq_stats_write() writes
to disk.  All the state changing (e.g., resetting the dirreq-stats
history and initializing the next measurement interval) takes place in
geoip_dirreq_stats_write().  That allows us to finally test the
dirreq-stats code better.

src/or/geoip.c

@@ -930,10 +930,9 @@ geoip_dirreq_stats_init(time_t now)
   start_of_dirreq_stats_interval = now;
 }
 
-/** Stop collecting directory request stats in a way that we can re-start
- * doing so in geoip_dirreq_stats_init(). */
+/** Reset counters for dirreq stats. */
 void
-geoip_dirreq_stats_term(void)
+geoip_reset_dirreq_stats(time_t now)
 {
   SMARTLIST_FOREACH(geoip_countries, geoip_country_t *, c, {
       c->n_v2_ns_requests = c->n_v3_ns_requests = 0;
@@ -965,19 +964,24 @@ geoip_dirreq_stats_term(void)
       tor_free(this);
     }
   }
-  start_of_dirreq_stats_interval = 0;
+  start_of_dirreq_stats_interval = now;
 }
 
-/** Write dirreq statistics to $DATADIR/stats/dirreq-stats and return when
- * we would next want to write. */
-time_t
-geoip_dirreq_stats_write(time_t now)
+/** Stop collecting directory request stats in a way that we can re-start
+ * doing so in geoip_dirreq_stats_init(). */
+void
+geoip_dirreq_stats_term(void)
+{
+  geoip_reset_dirreq_stats(0);
+}
+
+/** Return a newly allocated string containing the dirreq statistics
+ * until <b>now</b>, or NULL if we're not collecting dirreq stats. */
+char *
+geoip_format_dirreq_stats(time_t now)
 {
-  char *statsdir = NULL, *filename = NULL;
-  open_file_t *open_file = NULL;
   char t[ISO_TIME_LEN+1];
   double v2_share = 0.0, v3_share = 0.0;
-  FILE *out;
   int i;
   char *v3_ips_string, *v2_ips_string, *v3_reqs_string, *v2_reqs_string,
        *v2_share_string = NULL, *v3_share_string = NULL,
@@ -986,12 +990,7 @@ geoip_dirreq_stats_write(time_t now)
   char *result;
 
   if (!start_of_dirreq_stats_interval)
-    return 0; /* Not initialized. */
-  if (start_of_dirreq_stats_interval + WRITE_STATS_INTERVAL > now)
-    goto done; /* Not ready to write. */
-
-  /* Discard all items in the client history that are too old. */
-  geoip_remove_old_clients(start_of_dirreq_stats_interval);
+    return NULL; /* Not initialized. */
 
   format_iso_time(t, now);
   v2_ips_string = geoip_get_client_history(GEOIP_CLIENT_NETWORKSTATUS_V2);
@@ -1078,34 +1077,47 @@ geoip_dirreq_stats_write(time_t now)
   tor_free(v3_tunneled_dl_string);
   tor_free(v2_tunneled_dl_string);
 
-  /* Reset history to prepare for the next measurement interval. */
-  SMARTLIST_FOREACH(geoip_countries, geoip_country_t *, c, {
-      c->n_v2_ns_requests = c->n_v3_ns_requests = 0;
-  });
-  memset(ns_v2_responses, 0, sizeof(ns_v2_responses));
-  memset(ns_v3_responses, 0, sizeof(ns_v3_responses));
-  start_of_dirreq_stats_interval = now;
+  return result;
+}
+
+/** If 24 hours have passed since the beginning of the current dirreq
+ * stats period, write dirreq stats to $DATADIR/stats/dirreq-stats
+ * (possibly overwriting an existing file) and reset counters.  Return
+ * when we would next want to write dirreq stats or 0 if we never want to
+ * write. */
+time_t
+geoip_dirreq_stats_write(time_t now)
+{
+  char *statsdir = NULL, *filename = NULL, *str = NULL;
+
+  if (!start_of_dirreq_stats_interval)
+    return 0; /* Not initialized. */
+  if (start_of_dirreq_stats_interval + WRITE_STATS_INTERVAL > now)
+    goto done; /* Not ready to write. */
+
+  /* Discard all items in the client history that are too old. */
+  geoip_remove_old_clients(start_of_dirreq_stats_interval);
+
+  /* Generate history string .*/
+  str = geoip_format_dirreq_stats(now);
 
   /* Write dirreq-stats string to disk. */
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
+    log_warn(LD_HIST, "Unable to create stats/ directory!");
     goto done;
+  }
   filename = get_datadir_fname2("stats", "dirreq-stats");
-  out = start_writing_to_stdio_file(filename, OPEN_FLAGS_REPLACE | O_TEXT,
-                                    0600, &open_file);
-  if (!out)
-    goto done;
-  if (fprintf(out, "%s", result) < 0)
-    goto done;
-  finish_writing_to_file(open_file);
-  open_file = NULL;
+  if (write_str_to_file(filename, str, 0) < 0)
+    log_warn(LD_HIST, "Unable to write dirreq statistics to disk!");
+
+  /* Reset measurement interval start. */
+  geoip_reset_dirreq_stats(now);
 
  done:
-  if (open_file)
-    abort_writing_to_file(open_file);
   tor_free(statsdir);
   tor_free(filename);
-  tor_free(result);
+  tor_free(str);
   return start_of_dirreq_stats_interval + WRITE_STATS_INTERVAL;
 }
 

src/or/geoip.h

@@ -43,6 +43,8 @@ void geoip_change_dirreq_state(uint64_t dirreq_id, dirreq_type_t type,
                                dirreq_state_t new_state);
 
 void geoip_dirreq_stats_init(time_t now);
+void geoip_reset_dirreq_stats(time_t now);
+char *geoip_format_dirreq_stats(time_t now);
 time_t geoip_dirreq_stats_write(time_t now);
 void geoip_dirreq_stats_term(void);
 void geoip_entry_stats_init(time_t now);

src/test/test.c

@@ -1481,8 +1481,64 @@ static void
 test_geoip(void)
 {
   int i, j;
-  time_t now = time(NULL);
+  time_t now = 1281533250; /* 2010-08-11 13:27:30 UTC */
   char *s = NULL;
+  const char *dirreq_stats_1 =
+      "dirreq-stats-end 2010-08-12 13:27:30 (86400 s)\n"
+      "dirreq-v3-ips ab=8\n"
+      "dirreq-v2-ips \n"
+      "dirreq-v3-reqs ab=8\n"
+      "dirreq-v2-reqs \n"
+      "dirreq-v3-resp ok=0,not-enough-sigs=0,unavailable=0,not-found=0,"
+          "not-modified=0,busy=0\n"
+      "dirreq-v2-resp ok=0,unavailable=0,not-found=0,not-modified=0,"
+          "busy=0\n"
+      "dirreq-v3-direct-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v2-direct-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v3-tunneled-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v2-tunneled-dl complete=0,timeout=0,running=0\n",
+  *dirreq_stats_2 =
+      "dirreq-stats-end 2010-08-12 13:27:30 (86400 s)\n"
+      "dirreq-v3-ips \n"
+      "dirreq-v2-ips \n"
+      "dirreq-v3-reqs \n"
+      "dirreq-v2-reqs \n"
+      "dirreq-v3-resp ok=0,not-enough-sigs=0,unavailable=0,not-found=0,"
+          "not-modified=0,busy=0\n"
+      "dirreq-v2-resp ok=0,unavailable=0,not-found=0,not-modified=0,"
+          "busy=0\n"
+      "dirreq-v3-direct-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v2-direct-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v3-tunneled-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v2-tunneled-dl complete=0,timeout=0,running=0\n",
+  *dirreq_stats_3 =
+      "dirreq-stats-end 2010-08-12 13:27:30 (86400 s)\n"
+      "dirreq-v3-ips \n"
+      "dirreq-v2-ips \n"
+      "dirreq-v3-reqs \n"
+      "dirreq-v2-reqs \n"
+      "dirreq-v3-resp ok=8,not-enough-sigs=0,unavailable=0,not-found=0,"
+          "not-modified=0,busy=0\n"
+      "dirreq-v2-resp ok=0,unavailable=0,not-found=0,not-modified=0,"
+          "busy=0\n"
+      "dirreq-v3-direct-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v2-direct-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v3-tunneled-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v2-tunneled-dl complete=0,timeout=0,running=0\n",
+  *dirreq_stats_4 =
+      "dirreq-stats-end 2010-08-12 13:27:30 (86400 s)\n"
+      "dirreq-v3-ips \n"
+      "dirreq-v2-ips \n"
+      "dirreq-v3-reqs \n"
+      "dirreq-v2-reqs \n"
+      "dirreq-v3-resp ok=8,not-enough-sigs=0,unavailable=0,not-found=0,"
+          "not-modified=0,busy=0\n"
+      "dirreq-v2-resp ok=0,unavailable=0,not-found=0,not-modified=0,"
+          "busy=0\n"
+      "dirreq-v3-direct-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v2-direct-dl complete=0,timeout=0,running=0\n"
+      "dirreq-v3-tunneled-dl complete=0,timeout=0,running=4\n"
+      "dirreq-v2-tunneled-dl complete=0,timeout=0,running=0\n";
 
   /* Populate the DB a bit.  Add these in order, since we can't do the final
    * 'sort' step.  These aren't very good IP addresses, but they're perfectly
@@ -1532,6 +1588,55 @@ test_geoip(void)
   test_assert(s);
   test_streq("zz=24,xy=8", s);
 
+  /* Stop being a bridge and start being a directory mirror that gathers
+   * directory request statistics. */
+  get_options_mutable()->BridgeRelay = 0;
+  get_options_mutable()->BridgeRecordUsageByCountry = 0;
+  get_options_mutable()->DirReqStatistics = 1;
+
+  /* Start testing dirreq statistics by making sure that we don't collect
+   * dirreq stats without initializing them. */
+  geoip_note_client_seen(GEOIP_CLIENT_NETWORKSTATUS, 100, now);
+  s = geoip_format_dirreq_stats(now + 86400);
+  test_assert(!s);
+
+  /* Initialize stats, note one connecting client, and generate the
+   * dirreq-stats history string. */
+  geoip_dirreq_stats_init(now);
+  geoip_note_client_seen(GEOIP_CLIENT_NETWORKSTATUS, 100, now);
+  s = geoip_format_dirreq_stats(now + 86400);
+  test_streq(dirreq_stats_1, s);
+  tor_free(s);
+
+  /* Stop collecting stats, add another connecting client, and ensure we
+   * don't generate a history string. */
+  geoip_dirreq_stats_term();
+  geoip_note_client_seen(GEOIP_CLIENT_NETWORKSTATUS, 101, now);
+  s = geoip_format_dirreq_stats(now + 86400);
+  test_assert(!s);
+
+  /* Re-start stats, add a connecting client, reset stats, and make sure
+   * that we get an all empty history string. */
+  geoip_dirreq_stats_init(now);
+  geoip_note_client_seen(GEOIP_CLIENT_NETWORKSTATUS, 100, now);
+  geoip_reset_dirreq_stats(now);
+  s = geoip_format_dirreq_stats(now + 86400);
+  test_streq(dirreq_stats_2, s);
+  tor_free(s);
+
+  /* Note a successful network status response and make sure that it
+   * appears in the history string. */
+  geoip_note_ns_response(GEOIP_CLIENT_NETWORKSTATUS, GEOIP_SUCCESS);
+  s = geoip_format_dirreq_stats(now + 86400);
+  test_streq(dirreq_stats_3, s);
+  tor_free(s);
+
+  /* Start a tunneled directory request. */
+  geoip_start_dirreq((uint64_t) 1, 1024, GEOIP_CLIENT_NETWORKSTATUS,
+                     DIRREQ_TUNNELED);
+  s = geoip_format_dirreq_stats(now + 86400);
+  test_streq(dirreq_stats_4, s);
+
  done:
   tor_free(s);
 }