r14832@catbus: nickm | 2007-08-29 15:00:27 -0400

Use (and debug) new file-writing functions in order to simplify code that formerly built big strings in RAM.


svn:r11301

ChangeLog

@@ -18,6 +18,11 @@ Changes in version 0.2.0.7-alpha - 2007-??-??
       router, do not try to include the nickname if it is absent.  Fixes
       bug 467.
 
+  o Code simplifications and refactoring:
+    - Revamp file-writing logic so we don't need to have the entire contents
+      of a file in memory at once before we write to disk.  Tor, meet stdio.
+
+
 Changes in version 0.2.0.6-alpha - 2007-08-26
   o New directory authorities:
     - Set up Tonga as the default bridge directory authority.

doc/TODO

@@ -156,8 +156,8 @@ N     - Design/implement the "local-status" or something like it, from the
        (high expected time to failure) or good guard qualities (high
        fractional uptime).
      - AKA Track uptime as %-of-time-up, as well as time-since-last-down
-    - Should TrackHostExits expire TrackHostExitsExpire seconds after their
-       *last* use, not their *first* use?
+    - Make TrackHostExits expire TrackHostExitsExpire seconds after their
+       *last* use, not their *first* use.
     - Limit to 2 dir, 2 OR, N SOCKS connections per IP.
      - Or maybe close connections from same IP when we get a lot from one.
      - Or maybe block IPs that connect too many times at once.

src/common/util.c

@@ -72,13 +72,6 @@ const char util_c_id[] = "$Id$";
 #include <malloc.h>
 #endif
 
-#ifndef O_BINARY
-#define O_BINARY 0
-#endif
-#ifndef O_TEXT
-#define O_TEXT 0
-#endif
-
 /* =====
  * Memory management
  * ===== */
@@ -1465,6 +1458,7 @@ struct open_file_t {
   char *filename;
   int rename_on_close;
   int fd;
+  FILE *stdio_file;
 };
 
 /** DOCDOC */
@@ -1477,6 +1471,9 @@ start_writing_to_file(const char *fname, int open_flags, int mode,
   const char *open_name;
   tor_assert(fname);
   tor_assert(data_out);
+#if (O_BINARY != 0 && O_TEXT != 0)
+  tor_assert((open_flags & (O_BINARY|O_TEXT)) != 0);
+#endif
   new_file->fd = -1;
   tempname_len = strlen(fname)+16;
   tor_assert(tempname_len > strlen(fname)); /*check for overflow*/
@@ -1485,7 +1482,7 @@ start_writing_to_file(const char *fname, int open_flags, int mode,
     open_name = fname;
     new_file->rename_on_close = 0;
   } else {
-    new_file->tempname = tor_malloc(tempname_len);
+    open_name = new_file->tempname = tor_malloc(tempname_len);
     if (tor_snprintf(new_file->tempname, tempname_len, "%s.tmp", fname)<0) {
       log(LOG_WARN, LD_GENERAL, "Failed to generate filename");
       goto err;
@@ -1493,10 +1490,10 @@ start_writing_to_file(const char *fname, int open_flags, int mode,
     new_file->rename_on_close = 1;
   }
 
-  if ((new_file->fd = open(new_file->tempname, open_flags, mode))
+  if ((new_file->fd = open(open_name, open_flags, mode))
       < 0) {
-    log(LOG_WARN, LD_FS, "Couldn't open \"%s\" for writing: %s",
-        new_file->tempname, strerror(errno));
+    log(LOG_WARN, LD_FS, "Couldn't open \"%s\" (%s) for writing: %s",
+        open_name, fname, strerror(errno));
     goto err;
   }
 
@@ -1511,17 +1508,50 @@ start_writing_to_file(const char *fname, int open_flags, int mode,
   return -1;
 }
 
+/** DOCDOC */
+FILE *
+fdopen_file(open_file_t *file_data)
+{
+  tor_assert(file_data);
+  if (file_data->stdio_file)
+    return file_data->stdio_file;
+  tor_assert(file_data->fd >= 0);
+  if (!(file_data->stdio_file = fdopen(file_data->fd, "a"))) {
+    log_warn(LD_FS, "Couldn't fdopen \"%s\": %s", file_data->filename,
+             strerror(errno));
+  }
+  return file_data->stdio_file;
+}
+
+/** DOCDOC */
+FILE *
+start_writing_to_stdio_file(const char *fname, int open_flags, int mode,
+                            open_file_t **data_out)
+{
+  FILE *res;
+  if (start_writing_to_file(fname, open_flags, mode, data_out)<0)
+    return NULL;
+  if (!(res = fdopen_file(*data_out)))
+    abort_writing_to_file(*data_out);
+  return res;
+}
+
 /** DOCDOC */
 static int
 finish_writing_to_file_impl(open_file_t *file_data, int abort_write)
 {
   int r = 0;
   tor_assert(file_data && file_data->filename);
-  if (file_data->fd >= 0 && close(file_data->fd) < 0) {
+  if (file_data->stdio_file) {
+    if (fclose(file_data->stdio_file)) {
+      log_warn(LD_FS, "Error closing \"%s\": %s", file_data->filename,
+               strerror(errno));
+      abort_write = r = -1;
+    }
+  } else if (file_data->fd >= 0 && close(file_data->fd) < 0) {
     log_warn(LD_FS, "Error flushing \"%s\": %s", file_data->filename,
              strerror(errno));
-    abort_write = 1;
-    r = -1;
+    abort_write = r = -1;
   }
 
   if (file_data->rename_on_close) {
@@ -1538,6 +1568,8 @@ finish_writing_to_file_impl(open_file_t *file_data, int abort_write)
     }
   }
 
+  memset(file_data, 0, sizeof(file_data));
+  file_data->fd = -1;
   tor_free(file_data->filename);
   tor_free(file_data->tempname);
   tor_free(file_data);
@@ -1593,7 +1625,7 @@ write_chunks_to_file_impl(const char *fname, const smartlist_t *chunks,
 int
 write_chunks_to_file(const char *fname, const smartlist_t *chunks, int bin)
 {
-  int flags = O_WRONLY|O_CREAT|O_TRUNC|(bin?O_BINARY:O_TEXT);
+  int flags = OPEN_FLAGS_REPLACE|(bin?O_BINARY:O_TEXT);
   return write_chunks_to_file_impl(fname, chunks, flags);
 }
 
@@ -1603,7 +1635,7 @@ int
 write_bytes_to_file(const char *fname, const char *str, size_t len,
                     int bin)
 {
-  int flags = O_WRONLY|O_CREAT|O_TRUNC|(bin?O_BINARY:O_TEXT);
+  int flags = OPEN_FLAGS_REPLACE|(bin?O_BINARY:O_TEXT);
   int r;
   sized_chunk_t c = { str, len };
   smartlist_t *chunks = smartlist_create();
@@ -1619,7 +1651,7 @@ int
 append_bytes_to_file(const char *fname, const char *str, size_t len,
                      int bin)
 {
-  int flags = O_WRONLY|O_CREAT|O_APPEND|(bin?O_BINARY:O_TEXT);
+  int flags = OPEN_FLAGS_APPEND|(bin?O_BINARY:O_TEXT);
   int r;
   sized_chunk_t c = { str, len };
   smartlist_t *chunks = smartlist_create();

src/common/util.h

@@ -18,6 +18,13 @@
 #include <stdio.h>
 #include <stdlib.h>
 
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+#ifndef O_TEXT
+#define O_TEXT 0
+#endif
+
 /* Replace assert() with a variant that sends failures to the log before
  * calling assert() normally.
  */
@@ -213,9 +220,14 @@ file_status_t file_status(const char *filename);
  * directory; see that function's documentation for details. */
 typedef enum { CPD_NONE, CPD_CREATE, CPD_CHECK } cpd_check_t;
 int check_private_dir(const char *dirname, cpd_check_t check);
+#define OPEN_FLAGS_REPLACE (O_WRONLY|O_CREAT|O_TRUNC)
+#define OPEN_FLAGS_APPEND (O_WRONLY|O_CREAT|O_APPEND)
 typedef struct open_file_t open_file_t;
 int start_writing_to_file(const char *fname, int open_flags, int mode,
                           open_file_t **data_out);
+FILE *start_writing_to_stdio_file(const char *fname, int open_flags, int mode,
+                                  open_file_t **data_out);
+FILE *fdopen_file(open_file_t *file_data);
 int finish_writing_to_file(open_file_t *file_data);
 int abort_writing_to_file(open_file_t *file_data);
 int write_str_to_file(const char *fname, const char *str, int bin);

src/or/rephist.c

@@ -563,14 +563,23 @@ get_mtbf_filename(void)
 int
 rep_hist_record_mtbf_data(void)
 {
-  char buf[128];
   char time_buf[ISO_TIME_LEN+1];
-  smartlist_t *lines;
 
   digestmap_iter_t *orhist_it;
   const char *digest;
   void *or_history_p;
   or_history_t *hist;
+  open_file_t *open_file = NULL;
+  FILE *f;
+
+  {
+    char *filename = get_mtbf_filename();
+    f = start_writing_to_stdio_file(filename, OPEN_FLAGS_REPLACE|O_TEXT, 0600,
+                                    &open_file);
+    tor_free(filename);
+    if (!f)
+      return -1;
+  }
 
   /* File format is:
    *   FormatLine *KeywordLine Data
@@ -581,27 +590,24 @@ rep_hist_record_mtbf_data(void)
    *   RouterMTBFLine = Fingerprint SP WeightedRunLen SP
    *           TotalRunWeights [SP S=StartRunTime] NL
    */
+#define PUT(s) STMT_BEGIN if (fputs((s),f)<0) goto err; STMT_END
+#define PRINTF(args) STMT_BEGIN if (fprintf args <0) goto err; STMT_END
 
-  lines = smartlist_create();
-
-  smartlist_add(lines, tor_strdup("format 1\n"));
+  PUT("format 1\n");
 
   format_iso_time(time_buf, time(NULL));
-  tor_snprintf(buf, sizeof(buf), "stored-at %s\n", time_buf);
-  smartlist_add(lines, tor_strdup(buf));
+  PRINTF((f, "stored-at %s\n", time_buf));
 
   if (started_tracking_stability) {
     format_iso_time(time_buf, started_tracking_stability);
-    tor_snprintf(buf, sizeof(buf), "tracked-since %s\n", time_buf);
-    smartlist_add(lines, tor_strdup(buf));
+    PRINTF((f, "tracked-since %s\n", time_buf));
   }
   if (stability_last_downrated) {
     format_iso_time(time_buf, stability_last_downrated);
-    tor_snprintf(buf, sizeof(buf), "last-downrated %s\n", time_buf);
-    smartlist_add(lines, tor_strdup(buf));
+    PRINTF((f, "last-downrated %s\n", time_buf));
   }
 
-  smartlist_add(lines, tor_strdup("data\n"));
+  PUT("data\n");
 
   for (orhist_it = digestmap_iter_init(history_map);
        !digestmap_iter_done(orhist_it);
@@ -616,28 +622,20 @@ rep_hist_record_mtbf_data(void)
       format_iso_time(time_buf, hist->start_of_run);
       t = time_buf;
     }
-    tor_snprintf(buf, sizeof(buf), "%s %lu %.5lf%s%s\n",
-                 dbuf, hist->weighted_run_length, hist->total_run_weights,
-                 t ? " S=" : "", t ? t : "");
-    smartlist_add(lines, tor_strdup(buf));
+    PRINTF((f, "%s %lu %.5lf%s%s\n",
+            dbuf, hist->weighted_run_length, hist->total_run_weights,
+            t ? " S=" : "", t ? t : ""));
   }
 
-  smartlist_add(lines, tor_strdup(".\n"));
+  PUT(".\n");
 
-  {
-    size_t sz;
-    /* XXXX This isn't terribly efficient; line-at-a-time would be
-     * way faster. */
-    char *filename = get_mtbf_filename();
-    char *data = smartlist_join_strings(lines, "", 0, &sz);
-    int r = write_bytes_to_file(filename, data, sz, 0);
+#undef PUT
+#undef PRINTF
 
-    tor_free(data);
-    tor_free(filename);
-    SMARTLIST_FOREACH(lines, char *, cp, tor_free(cp));
-    smartlist_free(lines);
-    return r;
-  }
+  return finish_writing_to_file(open_file);
+ err:
+  abort_writing_to_file(open_file);
+  return -1;
 }
 
 /** Load MTBF data from disk.  Returns 0 on success or recoverable error, -1

src/tools/tor-gencert.c

@@ -151,7 +151,7 @@ load_identity_key(void)
   FILE *f;
 
   if (make_new_id) {
-    int fd;
+    open_file_t *open_file = NULL;
     RSA *key;
     if (status != FN_NOENT) {
       log_err(LD_GENERAL, "--create-identity-key was specified, but %s "
@@ -171,18 +171,10 @@ load_identity_key(void)
       return 1;
     }
 
-    if ((fd = open(identity_key_file, O_CREAT|O_EXCL|O_WRONLY, 0400))<0) {
-      log_err(LD_GENERAL, "Couldn't fdopen %s for writing: %s",
-              identity_key_file, strerror(errno));
+    if (!(f = start_writing_to_stdio_file(identity_key_file,
+                                          OPEN_FLAGS_REPLACE, 0400,
+                                          &open_file)))
       return 1;
-    }
-
-    if (!(f = fdopen(fd, "w"))) {
-      close(fd);
-      log_err(LD_GENERAL, "Couldn't fdopen %s for writing: %s",
-              identity_key_file, strerror(errno));
-      return 1;
-    }
 
     if (!PEM_write_PKCS8PrivateKey_nid(f, identity_key,
                                        NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
@@ -191,9 +183,10 @@ load_identity_key(void)
       log_err(LD_GENERAL, "Couldn't write identity key to %s",
               identity_key_file);
       crypto_log_errors(LOG_ERR, "Writing identity key");
+      abort_writing_to_file(open_file);
       return 1;
     }
-    fclose(f);
+    finish_writing_to_file(open_file);
   } else {
     if (status != FN_FILE) {
       log_err(LD_GENERAL,
@@ -224,7 +217,7 @@ load_identity_key(void)
 static int
 generate_signing_key(void)
 {
-  int fd;
+  open_file_t *open_file;
   FILE *f;
   RSA *key;
   log_notice(LD_GENERAL, "Generating %d-bit RSA signing key.",
@@ -240,26 +233,19 @@ generate_signing_key(void)
     return 1;
   }
 
-  if ((fd = open(signing_key_file, O_CREAT|O_EXCL|O_WRONLY, 0600))<0) {
-    log_err(LD_GENERAL, "Couldn't open %s for writing: %s",
-            signing_key_file, strerror(errno));
+  if (!(f = start_writing_to_stdio_file(signing_key_file,
+                                        OPEN_FLAGS_REPLACE, 0600,
+                                        &open_file)))
     return 1;
-  }
-
-  if (!(f = fdopen(fd, "w"))) {
-    close(fd);
-    log_err(LD_GENERAL, "Couldn't open %s for writing: %s",
-            signing_key_file, strerror(errno));
-    return 1;
-  }
 
   /* Write signing key with no encryption. */
   if (!PEM_write_RSAPrivateKey(f, key, NULL, NULL, 0, NULL, NULL)) {
     crypto_log_errors(LOG_WARN, "writing signing key");
+    abort_writing_to_file(open_file);
     return 1;
   }
 
-  fclose(f);
+  finish_writing_to_file(open_file);
 
   return 0;
 }