From 1bdb391ed0df979bc29ed1b62e7c0f3c9494a8d7 Mon Sep 17 00:00:00 2001 From: Kevin Butler Date: Sun, 1 Sep 2013 00:24:07 +0100 Subject: [PATCH 1/3] Added no_tempfile parameter to write_chunks_to_file to do non-atomic writes. Implements #1376. --- changes/bug1376 | 2 ++ src/common/util.c | 13 ++++++++++--- src/common/util.h | 2 +- src/or/dirvote.c | 2 +- src/or/routerlist.c | 4 ++-- 5 files changed, 16 insertions(+), 7 deletions(-) create mode 100644 changes/bug1376 diff --git a/changes/bug1376 b/changes/bug1376 new file mode 100644 index 0000000000..631f2af563 --- /dev/null +++ b/changes/bug1376 @@ -0,0 +1,2 @@ + o Minor bugfixes: + - Added additional argument to write_chunks_to_file to optionally skip using a temp file to do non-atomic writes. Implements ticket #1376. \ No newline at end of file diff --git a/src/common/util.c b/src/common/util.c index 6e14a58dd1..4e84d942e8 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2190,12 +2190,19 @@ write_chunks_to_file_impl(const char *fname, const smartlist_t *chunks, return -1; } -/** Given a smartlist of sized_chunk_t, write them atomically to a file - * fname, overwriting or creating the file as necessary. */ +/** Given a smartlist of sized_chunk_t, write them to a file + * fname, overwriting or creating the file as necessary. + * If no_tempfile is 0 then the file will be written + * atomically. */ int -write_chunks_to_file(const char *fname, const smartlist_t *chunks, int bin) +write_chunks_to_file(const char *fname, const smartlist_t *chunks, int bin, int no_tempfile) { int flags = OPEN_FLAGS_REPLACE|(bin?O_BINARY:O_TEXT); + + if (no_tempfile) { + // O_APPEND stops write_chunks_to_file from using tempfiles + flags |= O_APPEND; + } return write_chunks_to_file_impl(fname, chunks, flags); } diff --git a/src/common/util.h b/src/common/util.h index 090243ea29..24428ad9dc 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -365,7 +365,7 @@ typedef struct sized_chunk_t { size_t len; } sized_chunk_t; int write_chunks_to_file(const char *fname, const struct smartlist_t *chunks, - int bin); + int bin, int no_tempfile); int append_bytes_to_file(const char *fname, const char *str, size_t len, int bin); int write_bytes_to_new_file(const char *fname, const char *str, size_t len, diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 12ceba8549..456a033ec9 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -3143,7 +3143,7 @@ dirvote_compute_consensuses(void) }); votefile = get_datadir_fname("v3-status-votes"); - write_chunks_to_file(votefile, votestrings, 0); + write_chunks_to_file(votefile, votestrings, 0, 0); tor_free(votefile); SMARTLIST_FOREACH(votestrings, sized_chunk_t *, c, tor_free(c)); smartlist_free(votestrings); diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 46da17e03b..3e8b9fbc01 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -428,7 +428,7 @@ trusted_dirs_flush_certs_to_disk(void) } DIGESTMAP_FOREACH_END; filename = get_datadir_fname("cached-certs"); - if (write_chunks_to_file(filename, chunks, 0)) { + if (write_chunks_to_file(filename, chunks, 0, 0)) { log_warn(LD_FS, "Error writing certificates to disk."); } tor_free(filename); @@ -1048,7 +1048,7 @@ router_rebuild_store(int flags, desc_store_t *store) smartlist_add(chunk_list, c); } SMARTLIST_FOREACH_END(sd); - if (write_chunks_to_file(fname_tmp, chunk_list, 1)<0) { + if (write_chunks_to_file(fname_tmp, chunk_list, 1, 1)<0) { log_warn(LD_FS, "Error writing router store to disk."); goto done; } From 0f070e7858d4270983141a701116de9353fb3fb6 Mon Sep 17 00:00:00 2001 From: Kevin Butler Date: Wed, 4 Sep 2013 23:25:41 +0100 Subject: [PATCH 2/3] Added test for new write_chunks_to_file behaviour in #1376. --- src/common/util.c | 5 +- src/test/test_util.c | 116 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 4e84d942e8..695d0486c4 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2191,11 +2191,12 @@ write_chunks_to_file_impl(const char *fname, const smartlist_t *chunks, } /** Given a smartlist of sized_chunk_t, write them to a file - * fname, overwriting or creating the file as necessary. + * fname, overwriting or creating the file as necessary. * If no_tempfile is 0 then the file will be written * atomically. */ int -write_chunks_to_file(const char *fname, const smartlist_t *chunks, int bin, int no_tempfile) +write_chunks_to_file(const char *fname, const smartlist_t *chunks, int bin, + int no_tempfile) { int flags = OPEN_FLAGS_REPLACE|(bin?O_BINARY:O_TEXT); diff --git a/src/test/test_util.c b/src/test/test_util.c index 05d28d7877..6f2bdddb51 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -102,6 +102,121 @@ test_util_read_file_eof_zero_bytes(void *arg) test_util_read_until_eof_impl("tor_test_fifo_empty", 0, 10000); } +/* Test the basic expected behaviour for write_chunks_to_file. +* NOTE: This will need to be updated if we ever change the tempfile location +* or extension */ +static void +test_util_write_chunks_to_file(void *arg) +{ + char *fname; + char *tempname; + char *str; + char *data_str; + char *temp_str; + int r; + int fd = -1; + size_t sz = 999999; + (void)arg; + + /* These should be two different sizes to ensure the data is different + * between the data file and the temp file's 'known string' */ + int temp_str_len = 1024; + int data_str_len = 512; + temp_str = tor_malloc(temp_str_len); + data_str = tor_malloc(data_str_len); + crypto_rand(temp_str, temp_str_len); + crypto_rand(data_str, data_str_len); + tt_assert(temp_str != NULL); + tt_assert(data_str != NULL); + + // Ensure it can write multiple chunks + smartlist_t *chunks = smartlist_new(); + sized_chunk_t c = {data_str, data_str_len/2}; + sized_chunk_t c2 = {data_str + data_str_len/2, data_str_len/2}; + smartlist_add(chunks, &c); + smartlist_add(chunks, &c2); + + /* + * Check if it writes using a tempfile + */ + fname = tor_strdup("tor_test_write_chunks_to_file_with_tempfile"); + tor_asprintf(&tempname, "%s.tmp", fname); + + // write a known string to a file where the tempfile will be + r = write_bytes_to_file(tempname, temp_str, temp_str_len, 1); + tt_int_op(r, ==, 0); + + // call write_chunks_to_file + r = write_chunks_to_file(fname, chunks, 1, 0); + tt_int_op(r, ==, 0); + + // assert the file has been written (expected size) + fd = open(fname, O_RDONLY|O_BINARY); + tt_int_op(fd, >=, 0); + str = read_file_to_str_until_eof(fd, data_str_len*2, &sz); + tt_assert(str != NULL); + tt_int_op(sz, ==, data_str_len); + test_mem_op(data_str, ==, str, sz); + tor_free(str); + close(fd); + + // assert that the tempfile is removed (should not leave artifacts) + fd = open(tempname, O_RDONLY|O_BINARY); + tt_int_op(fd, <, 0); // This might be untrue + str = read_file_to_str_until_eof(fd, temp_str_len*2, &sz); + tt_assert(str == NULL); + + // Remove old testfile for second test + r = unlink(fname); + tt_int_op(r, ==, 0); + tor_free(fname); + tor_free(tempname); + + /* + * Check if it skips using a tempfile with flags + */ + fname = tor_strdup("tor_test_write_chunks_to_file_with_no_tempfile"); + tor_asprintf(&tempname, "%s.tmp", fname); + + // write a known string to a file where the tempfile will be + r = write_bytes_to_file(tempname, temp_str, temp_str_len, 1); + tt_int_op(r, ==, 0); + + // call write_chunks_to_file with no_tempfile = true + r = write_chunks_to_file(fname, chunks, 1, 1); + tt_int_op(r, ==, 0); + + // assert the file has been written (expected size) + fd = open(fname, O_RDONLY|O_BINARY); + tt_int_op(fd, >=, 0); + str = read_file_to_str_until_eof(fd, data_str_len*2, &sz); + tt_assert(str != NULL); + tt_int_op(sz, ==, data_str_len); + test_mem_op(data_str, ==, str, sz); + tor_free(str); + close(fd); + + // assert the tempfile still contains the known string + fd = open(tempname, O_RDONLY|O_BINARY); + tt_int_op(fd, >=, 0); // This might be untrue + str = read_file_to_str_until_eof(fd, temp_str_len*2, &sz); + tt_assert(str != NULL); + tt_int_op(sz, ==, temp_str_len); + test_mem_op(temp_str, ==, str, sz); + + done: + unlink(fname); + unlink(tempname); + smartlist_free(chunks); + tor_free(fname); + tor_free(tempname); + tor_free(str); + tor_free(data_str); + tor_free(temp_str); + if (fd >= 0) + close(fd); +} + static void test_util_time(void) { @@ -3502,6 +3617,7 @@ struct testcase_t util_tests[] = { UTIL_TEST(read_file_eof_tiny_limit, 0), UTIL_TEST(read_file_eof_two_loops, 0), UTIL_TEST(read_file_eof_zero_bytes, 0), + UTIL_TEST(write_chunks_to_file, 0), UTIL_TEST(mathlog, 0), UTIL_TEST(weak_random, 0), UTIL_TEST(socket, TT_FORK), From 7ef9ecf6b38041e38dd079c2d18b5c8813d1959a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 11 Oct 2013 12:51:07 -0400 Subject: [PATCH 3/3] Fix some whitespace; tighten the tests. --- changes/bug1376 | 4 +- src/common/util.c | 4 +- src/test/test_util.c | 87 ++++++++++++++++++++------------------------ 3 files changed, 44 insertions(+), 51 deletions(-) diff --git a/changes/bug1376 b/changes/bug1376 index 631f2af563..bee42a39a4 100644 --- a/changes/bug1376 +++ b/changes/bug1376 @@ -1,2 +1,4 @@ o Minor bugfixes: - - Added additional argument to write_chunks_to_file to optionally skip using a temp file to do non-atomic writes. Implements ticket #1376. \ No newline at end of file + + - Added additional argument to write_chunks_to_file to optionally skip + using a temp file to do non-atomic writes. Implements ticket #1376. diff --git a/src/common/util.c b/src/common/util.c index 695d0486c4..f31a5d84f3 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2196,12 +2196,12 @@ write_chunks_to_file_impl(const char *fname, const smartlist_t *chunks, * atomically. */ int write_chunks_to_file(const char *fname, const smartlist_t *chunks, int bin, - int no_tempfile) + int no_tempfile) { int flags = OPEN_FLAGS_REPLACE|(bin?O_BINARY:O_TEXT); if (no_tempfile) { - // O_APPEND stops write_chunks_to_file from using tempfiles + /* O_APPEND stops write_chunks_to_file from using tempfiles */ flags |= O_APPEND; } return write_chunks_to_file_impl(fname, chunks, flags); diff --git a/src/test/test_util.c b/src/test/test_util.c index 6f2bdddb51..25d968cffe 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -103,43 +103,42 @@ test_util_read_file_eof_zero_bytes(void *arg) } /* Test the basic expected behaviour for write_chunks_to_file. -* NOTE: This will need to be updated if we ever change the tempfile location -* or extension */ + * NOTE: This will need to be updated if we ever change the tempfile location + * or extension */ static void test_util_write_chunks_to_file(void *arg) { - char *fname; - char *tempname; - char *str; - char *data_str; - char *temp_str; + char *fname = NULL; + char *tempname = NULL; + char *str = NULL; int r; int fd = -1; - size_t sz = 999999; - (void)arg; + struct stat st; /* These should be two different sizes to ensure the data is different - * between the data file and the temp file's 'known string' */ + * between the data file and the temp file's 'known string' */ int temp_str_len = 1024; int data_str_len = 512; - temp_str = tor_malloc(temp_str_len); - data_str = tor_malloc(data_str_len); - crypto_rand(temp_str, temp_str_len); - crypto_rand(data_str, data_str_len); - tt_assert(temp_str != NULL); - tt_assert(data_str != NULL); + char *data_str = tor_malloc(data_str_len); + char *temp_str = tor_malloc(temp_str_len); - // Ensure it can write multiple chunks smartlist_t *chunks = smartlist_new(); sized_chunk_t c = {data_str, data_str_len/2}; sized_chunk_t c2 = {data_str + data_str_len/2, data_str_len/2}; + (void)arg; + + crypto_rand(temp_str, temp_str_len); + crypto_rand(data_str, data_str_len); + + // Ensure it can write multiple chunks + smartlist_add(chunks, &c); smartlist_add(chunks, &c2); /* * Check if it writes using a tempfile */ - fname = tor_strdup("tor_test_write_chunks_to_file_with_tempfile"); + fname = tor_strdup(get_fname("write_chunks_with_tempfile")); tor_asprintf(&tempname, "%s.tmp", fname); // write a known string to a file where the tempfile will be @@ -151,19 +150,15 @@ test_util_write_chunks_to_file(void *arg) tt_int_op(r, ==, 0); // assert the file has been written (expected size) - fd = open(fname, O_RDONLY|O_BINARY); - tt_int_op(fd, >=, 0); - str = read_file_to_str_until_eof(fd, data_str_len*2, &sz); + str = read_file_to_str(fname, RFTS_BIN, &st); tt_assert(str != NULL); - tt_int_op(sz, ==, data_str_len); - test_mem_op(data_str, ==, str, sz); + tt_int_op(st.st_size, ==, data_str_len); + test_mem_op(data_str, ==, str, data_str_len); tor_free(str); close(fd); // assert that the tempfile is removed (should not leave artifacts) - fd = open(tempname, O_RDONLY|O_BINARY); - tt_int_op(fd, <, 0); // This might be untrue - str = read_file_to_str_until_eof(fd, temp_str_len*2, &sz); + str = read_file_to_str(tempname, RFTS_BIN|RFTS_IGNORE_MISSING, &st); tt_assert(str == NULL); // Remove old testfile for second test @@ -175,7 +170,7 @@ test_util_write_chunks_to_file(void *arg) /* * Check if it skips using a tempfile with flags */ - fname = tor_strdup("tor_test_write_chunks_to_file_with_no_tempfile"); + fname = tor_strdup(get_fname("write_chunks_with_no_tempfile")); tor_asprintf(&tempname, "%s.tmp", fname); // write a known string to a file where the tempfile will be @@ -187,34 +182,30 @@ test_util_write_chunks_to_file(void *arg) tt_int_op(r, ==, 0); // assert the file has been written (expected size) - fd = open(fname, O_RDONLY|O_BINARY); - tt_int_op(fd, >=, 0); - str = read_file_to_str_until_eof(fd, data_str_len*2, &sz); + str = read_file_to_str(fname, RFTS_BIN, &st); tt_assert(str != NULL); - tt_int_op(sz, ==, data_str_len); - test_mem_op(data_str, ==, str, sz); + tt_int_op(st.st_size, ==, data_str_len); + test_mem_op(data_str, ==, str, data_str_len); tor_free(str); close(fd); // assert the tempfile still contains the known string - fd = open(tempname, O_RDONLY|O_BINARY); - tt_int_op(fd, >=, 0); // This might be untrue - str = read_file_to_str_until_eof(fd, temp_str_len*2, &sz); + str = read_file_to_str(tempname, RFTS_BIN, &st); tt_assert(str != NULL); - tt_int_op(sz, ==, temp_str_len); - test_mem_op(temp_str, ==, str, sz); + tt_int_op(st.st_size, ==, temp_str_len); + test_mem_op(temp_str, ==, str, temp_str_len); - done: - unlink(fname); - unlink(tempname); - smartlist_free(chunks); - tor_free(fname); - tor_free(tempname); - tor_free(str); - tor_free(data_str); - tor_free(temp_str); - if (fd >= 0) - close(fd); + done: + unlink(fname); + unlink(tempname); + smartlist_free(chunks); + tor_free(fname); + tor_free(tempname); + tor_free(str); + tor_free(data_str); + tor_free(temp_str); + if (fd >= 0) + close(fd); } static void