diff options
Diffstat (limited to 'reftable')
| -rw-r--r-- | reftable/blocksource.c | 20 | ||||
| -rw-r--r-- | reftable/blocksource.h | 2 | ||||
| -rw-r--r-- | reftable/reader.c | 149 | ||||
| -rw-r--r-- | reftable/reader.h | 5 | ||||
| -rw-r--r-- | reftable/reftable-reader.h | 19 | ||||
| -rw-r--r-- | reftable/stack.c | 90 | ||||
| -rw-r--r-- | reftable/stack_test.c | 116 |
7 files changed, 260 insertions, 141 deletions
diff --git a/reftable/blocksource.c b/reftable/blocksource.c index abce4bb2e1..e93cac9bb6 100644 --- a/reftable/blocksource.c +++ b/reftable/blocksource.c @@ -55,26 +55,6 @@ void block_source_from_strbuf(struct reftable_block_source *bs, bs->arg = buf; } -static void malloc_return_block(void *b UNUSED, struct reftable_block *dest) -{ - if (dest->len) - memset(dest->data, 0xff, dest->len); - reftable_free(dest->data); -} - -static struct reftable_block_source_vtable malloc_vtable = { - .return_block = &malloc_return_block, -}; - -static struct reftable_block_source malloc_block_source_instance = { - .ops = &malloc_vtable, -}; - -struct reftable_block_source malloc_block_source(void) -{ - return malloc_block_source_instance; -} - struct file_block_source { uint64_t size; unsigned char *data; diff --git a/reftable/blocksource.h b/reftable/blocksource.h index 072e2727ad..659a27b406 100644 --- a/reftable/blocksource.h +++ b/reftable/blocksource.h @@ -17,6 +17,4 @@ struct reftable_block_source; void block_source_from_strbuf(struct reftable_block_source *bs, struct strbuf *buf); -struct reftable_block_source malloc_block_source(void); - #endif diff --git a/reftable/reader.c b/reftable/reader.c index 082cf00b60..f877099087 100644 --- a/reftable/reader.c +++ b/reftable/reader.c @@ -162,58 +162,6 @@ done: return err; } -int init_reader(struct reftable_reader *r, struct reftable_block_source *source, - const char *name) -{ - struct reftable_block footer = { NULL }; - struct reftable_block header = { NULL }; - int err = 0; - uint64_t file_size = block_source_size(source); - - /* Need +1 to read type of first block. */ - uint32_t read_size = header_size(2) + 1; /* read v2 because it's larger. */ - memset(r, 0, sizeof(struct reftable_reader)); - - if (read_size > file_size) { - err = REFTABLE_FORMAT_ERROR; - goto done; - } - - err = block_source_read_block(source, &header, 0, read_size); - if (err != read_size) { - err = REFTABLE_IO_ERROR; - goto done; - } - - if (memcmp(header.data, "REFT", 4)) { - err = REFTABLE_FORMAT_ERROR; - goto done; - } - r->version = header.data[4]; - if (r->version != 1 && r->version != 2) { - err = REFTABLE_FORMAT_ERROR; - goto done; - } - - r->size = file_size - footer_size(r->version); - r->source = *source; - r->name = xstrdup(name); - r->hash_id = 0; - - err = block_source_read_block(source, &footer, r->size, - footer_size(r->version)); - if (err != footer_size(r->version)) { - err = REFTABLE_IO_ERROR; - goto done; - } - - err = parse_footer(r, footer.data, header.data); -done: - reftable_block_done(&footer); - reftable_block_done(&header); - return err; -} - struct table_iter { struct reftable_reader *r; uint8_t typ; @@ -227,6 +175,7 @@ static int table_iter_init(struct table_iter *ti, struct reftable_reader *r) { struct block_iter bi = BLOCK_ITER_INIT; memset(ti, 0, sizeof(*ti)); + reftable_reader_incref(r); ti->r = r; ti->bi = bi; return 0; @@ -314,6 +263,7 @@ static void table_iter_close(struct table_iter *ti) { table_iter_block_done(ti); block_iter_close(&ti->bi); + reftable_reader_decref(ti->r); } static int table_iter_next_block(struct table_iter *ti) @@ -631,31 +581,90 @@ void reftable_reader_init_log_iterator(struct reftable_reader *r, reader_init_iter(r, it, BLOCK_TYPE_LOG); } -void reader_close(struct reftable_reader *r) +int reftable_reader_new(struct reftable_reader **out, + struct reftable_block_source *source, char const *name) { - block_source_close(&r->source); - FREE_AND_NULL(r->name); -} + struct reftable_block footer = { 0 }; + struct reftable_block header = { 0 }; + struct reftable_reader *r; + uint64_t file_size = block_source_size(source); + uint32_t read_size; + int err; -int reftable_new_reader(struct reftable_reader **p, - struct reftable_block_source *src, char const *name) -{ - struct reftable_reader *rd = reftable_calloc(1, sizeof(*rd)); - int err = init_reader(rd, src, name); - if (err == 0) { - *p = rd; - } else { - block_source_close(src); - reftable_free(rd); + REFTABLE_CALLOC_ARRAY(r, 1); + + /* + * We need one extra byte to read the type of first block. We also + * pretend to always be reading v2 of the format because it is larger. + */ + read_size = header_size(2) + 1; + if (read_size > file_size) { + err = REFTABLE_FORMAT_ERROR; + goto done; + } + + err = block_source_read_block(source, &header, 0, read_size); + if (err != read_size) { + err = REFTABLE_IO_ERROR; + goto done; + } + + if (memcmp(header.data, "REFT", 4)) { + err = REFTABLE_FORMAT_ERROR; + goto done; + } + r->version = header.data[4]; + if (r->version != 1 && r->version != 2) { + err = REFTABLE_FORMAT_ERROR; + goto done; + } + + r->size = file_size - footer_size(r->version); + r->source = *source; + r->name = xstrdup(name); + r->hash_id = 0; + r->refcount = 1; + + err = block_source_read_block(source, &footer, r->size, + footer_size(r->version)); + if (err != footer_size(r->version)) { + err = REFTABLE_IO_ERROR; + goto done; + } + + err = parse_footer(r, footer.data, header.data); + if (err) + goto done; + + *out = r; + +done: + reftable_block_done(&footer); + reftable_block_done(&header); + if (err) { + reftable_free(r); + block_source_close(source); } return err; } -void reftable_reader_free(struct reftable_reader *r) +void reftable_reader_incref(struct reftable_reader *r) +{ + if (!r->refcount) + BUG("cannot increment ref counter of dead reader"); + r->refcount++; +} + +void reftable_reader_decref(struct reftable_reader *r) { if (!r) return; - reader_close(r); + if (!r->refcount) + BUG("cannot decrement ref counter of dead reader"); + if (--r->refcount) + return; + block_source_close(&r->source); + FREE_AND_NULL(r->name); reftable_free(r); } @@ -786,7 +795,7 @@ int reftable_reader_print_blocks(const char *tablename) if (err < 0) goto done; - err = reftable_new_reader(&r, &src, tablename); + err = reftable_reader_new(&r, &src, tablename); if (err < 0) goto done; @@ -817,7 +826,7 @@ int reftable_reader_print_blocks(const char *tablename) } done: - reftable_reader_free(r); + reftable_reader_decref(r); table_iter_close(&ti); return err; } diff --git a/reftable/reader.h b/reftable/reader.h index a2c204d523..3710ee09b4 100644 --- a/reftable/reader.h +++ b/reftable/reader.h @@ -50,11 +50,10 @@ struct reftable_reader { struct reftable_reader_offsets ref_offsets; struct reftable_reader_offsets obj_offsets; struct reftable_reader_offsets log_offsets; + + uint64_t refcount; }; -int init_reader(struct reftable_reader *r, struct reftable_block_source *source, - const char *name); -void reader_close(struct reftable_reader *r); const char *reader_name(struct reftable_reader *r); void reader_init_iter(struct reftable_reader *r, diff --git a/reftable/reftable-reader.h b/reftable/reftable-reader.h index 69621c5b0f..a600452b56 100644 --- a/reftable/reftable-reader.h +++ b/reftable/reftable-reader.h @@ -23,16 +23,28 @@ /* The reader struct is a handle to an open reftable file. */ struct reftable_reader; -/* reftable_new_reader opens a reftable for reading. If successful, +/* reftable_reader_new opens a reftable for reading. If successful, * returns 0 code and sets pp. The name is used for creating a * stack. Typically, it is the basename of the file. The block source * `src` is owned by the reader, and is closed on calling * reftable_reader_destroy(). On error, the block source `src` is * closed as well. */ -int reftable_new_reader(struct reftable_reader **pp, +int reftable_reader_new(struct reftable_reader **pp, struct reftable_block_source *src, const char *name); +/* + * Manage the reference count of the reftable reader. A newly initialized + * reader starts with a refcount of 1 and will be deleted once the refcount has + * reached 0. + * + * This is required because readers may have longer lifetimes than the stack + * they belong to. The stack may for example be reloaded while the old tables + * are still being accessed by an iterator. + */ +void reftable_reader_incref(struct reftable_reader *reader); +void reftable_reader_decref(struct reftable_reader *reader); + /* Initialize a reftable iterator for reading refs. */ void reftable_reader_init_ref_iterator(struct reftable_reader *r, struct reftable_iterator *it); @@ -44,9 +56,6 @@ void reftable_reader_init_log_iterator(struct reftable_reader *r, /* returns the hash ID used in this table. */ uint32_t reftable_reader_hash_id(struct reftable_reader *r); -/* closes and deallocates a reader. */ -void reftable_reader_free(struct reftable_reader *); - /* return an iterator for the refs pointing to `oid`. */ int reftable_reader_refs_for(struct reftable_reader *r, struct reftable_iterator *it, uint8_t *oid); diff --git a/reftable/stack.c b/reftable/stack.c index d3a95d2f1d..ce0a35216b 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -186,7 +186,7 @@ void reftable_stack_destroy(struct reftable_stack *st) if (names && !has_name(names, name)) { stack_filename(&filename, st, name); } - reftable_reader_free(st->readers[i]); + reftable_reader_decref(st->readers[i]); if (filename.len) { /* On Windows, can only unlink after closing. */ @@ -226,6 +226,8 @@ static int reftable_stack_reload_once(struct reftable_stack *st, { size_t cur_len = !st->merged ? 0 : st->merged->readers_len; struct reftable_reader **cur = stack_copy_readers(st, cur_len); + struct reftable_reader **reused = NULL; + size_t reused_len = 0, reused_alloc = 0; size_t names_len = names_length(names); struct reftable_reader **new_readers = reftable_calloc(names_len, sizeof(*new_readers)); @@ -245,6 +247,18 @@ static int reftable_stack_reload_once(struct reftable_stack *st, if (cur[i] && 0 == strcmp(cur[i]->name, name)) { rd = cur[i]; cur[i] = NULL; + + /* + * When reloading the stack fails, we end up + * releasing all new readers. This also + * includes the reused readers, even though + * they are still in used by the old stack. We + * thus need to keep them alive here, which we + * do by bumping their refcount. + */ + REFTABLE_ALLOC_GROW(reused, reused_len + 1, reused_alloc); + reused[reused_len++] = rd; + reftable_reader_incref(rd); break; } } @@ -258,7 +272,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, if (err < 0) goto done; - err = reftable_new_reader(&rd, &src, name); + err = reftable_reader_new(&rd, &src, name); if (err < 0) goto done; } @@ -273,37 +287,47 @@ static int reftable_stack_reload_once(struct reftable_stack *st, if (err < 0) goto done; - st->readers_len = new_readers_len; - if (st->merged) - reftable_merged_table_free(st->merged); - if (st->readers) { - reftable_free(st->readers); - } - st->readers = new_readers; - new_readers = NULL; - new_readers_len = 0; - - new_merged->suppress_deletions = 1; - st->merged = new_merged; + /* + * Close the old, non-reused readers and proactively try to unlink + * them. This is done for systems like Windows, where the underlying + * file of such an open reader wouldn't have been possible to be + * unlinked by the compacting process. + */ for (i = 0; i < cur_len; i++) { if (cur[i]) { const char *name = reader_name(cur[i]); stack_filename(&table_path, st, name); - - reader_close(cur[i]); - reftable_reader_free(cur[i]); - - /* On Windows, can only unlink after closing. */ + reftable_reader_decref(cur[i]); unlink(table_path.buf); } } + /* Update the stack to point to the new tables. */ + if (st->merged) + reftable_merged_table_free(st->merged); + new_merged->suppress_deletions = 1; + st->merged = new_merged; + + if (st->readers) + reftable_free(st->readers); + st->readers = new_readers; + st->readers_len = new_readers_len; + new_readers = NULL; + new_readers_len = 0; + + /* + * Decrement the refcount of reused readers again. This only needs to + * happen on the successful case, because on the unsuccessful one we + * decrement their refcount via `new_readers`. + */ + for (i = 0; i < reused_len; i++) + reftable_reader_decref(reused[i]); + done: - for (i = 0; i < new_readers_len; i++) { - reader_close(new_readers[i]); - reftable_reader_free(new_readers[i]); - } + for (i = 0; i < new_readers_len; i++) + reftable_reader_decref(new_readers[i]); reftable_free(new_readers); + reftable_free(reused); reftable_free(cur); strbuf_release(&table_path); return err; @@ -1328,17 +1352,9 @@ done: strbuf_release(&table_name); free_names(names); - return err; -} - -static int stack_compact_range_stats(struct reftable_stack *st, - size_t first, size_t last, - struct reftable_log_expiry_config *config, - unsigned int flags) -{ - int err = stack_compact_range(st, first, last, config, flags); if (err == REFTABLE_LOCK_ERROR) st->stats.failures++; + return err; } @@ -1346,7 +1362,7 @@ int reftable_stack_compact_all(struct reftable_stack *st, struct reftable_log_expiry_config *config) { size_t last = st->merged->readers_len ? st->merged->readers_len - 1 : 0; - return stack_compact_range_stats(st, 0, last, config, 0); + return stack_compact_range(st, 0, last, config, 0); } static int segment_size(struct segment *s) @@ -1452,8 +1468,8 @@ int reftable_stack_auto_compact(struct reftable_stack *st) st->opts.auto_compaction_factor); reftable_free(sizes); if (segment_size(&seg) > 0) - return stack_compact_range_stats(st, seg.start, seg.end - 1, - NULL, STACK_COMPACT_RANGE_BEST_EFFORT); + return stack_compact_range(st, seg.start, seg.end - 1, + NULL, STACK_COMPACT_RANGE_BEST_EFFORT); return 0; } @@ -1540,12 +1556,12 @@ static void remove_maybe_stale_table(struct reftable_stack *st, uint64_t max, if (err < 0) goto done; - err = reftable_new_reader(&rd, &src, name); + err = reftable_reader_new(&rd, &src, name); if (err < 0) goto done; update_idx = reftable_reader_max_update_index(rd); - reftable_reader_free(rd); + reftable_reader_decref(rd); if (update_idx <= max) { unlink(table_path.buf); diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 311ad759d2..89cb2be19f 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -10,6 +10,7 @@ https://developers.google.com/open-source/licenses/bsd #include "system.h" +#include "copy.h" #include "reftable-reader.h" #include "merged.h" #include "basics.h" @@ -125,6 +126,7 @@ static void write_n_ref_tables(struct reftable_stack *st, .value_type = REFTABLE_REF_VAL1, }; + strbuf_reset(&buf); strbuf_addf(&buf, "refs/heads/branch-%04u", (unsigned) i); ref.refname = buf.buf; set_test_hash(ref.value.val1, i); @@ -1035,10 +1037,8 @@ static void test_reftable_stack_compaction_concurrent(void) static void unclean_stack_close(struct reftable_stack *st) { /* break abstraction boundary to simulate unclean shutdown. */ - int i = 0; - for (; i < st->readers_len; i++) { - reftable_reader_free(st->readers[i]); - } + for (size_t i = 0; i < st->readers_len; i++) + reftable_reader_decref(st->readers[i]); st->readers_len = 0; FREE_AND_NULL(st->readers); } @@ -1077,6 +1077,112 @@ static void test_reftable_stack_compaction_concurrent_clean(void) clear_dir(dir); } +static void test_reftable_stack_read_across_reload(void) +{ + struct reftable_write_options opts = { 0 }; + struct reftable_stack *st1 = NULL, *st2 = NULL; + struct reftable_ref_record rec = { 0 }; + struct reftable_iterator it = { 0 }; + char *dir = get_tmp_dir(__LINE__); + int err; + + /* Create a first stack and set up an iterator for it. */ + err = reftable_new_stack(&st1, dir, &opts); + EXPECT_ERR(err); + write_n_ref_tables(st1, 2); + EXPECT(st1->merged->readers_len == 2); + reftable_stack_init_ref_iterator(st1, &it); + err = reftable_iterator_seek_ref(&it, ""); + EXPECT_ERR(err); + + /* Set up a second stack for the same directory and compact it. */ + err = reftable_new_stack(&st2, dir, &opts); + EXPECT_ERR(err); + EXPECT(st2->merged->readers_len == 2); + err = reftable_stack_compact_all(st2, NULL); + EXPECT_ERR(err); + EXPECT(st2->merged->readers_len == 1); + + /* + * Verify that we can continue to use the old iterator even after we + * have reloaded its stack. + */ + err = reftable_stack_reload(st1); + EXPECT_ERR(err); + EXPECT(st1->merged->readers_len == 1); + err = reftable_iterator_next_ref(&it, &rec); + EXPECT_ERR(err); + EXPECT(!strcmp(rec.refname, "refs/heads/branch-0000")); + err = reftable_iterator_next_ref(&it, &rec); + EXPECT_ERR(err); + EXPECT(!strcmp(rec.refname, "refs/heads/branch-0001")); + err = reftable_iterator_next_ref(&it, &rec); + EXPECT(err > 0); + + reftable_ref_record_release(&rec); + reftable_iterator_destroy(&it); + reftable_stack_destroy(st1); + reftable_stack_destroy(st2); + clear_dir(dir); +} + +static void test_reftable_stack_reload_with_missing_table(void) +{ + struct reftable_write_options opts = { 0 }; + struct reftable_stack *st = NULL; + struct reftable_ref_record rec = { 0 }; + struct reftable_iterator it = { 0 }; + struct strbuf table_path = STRBUF_INIT, content = STRBUF_INIT; + char *dir = get_tmp_dir(__LINE__); + int err; + + /* Create a first stack and set up an iterator for it. */ + err = reftable_new_stack(&st, dir, &opts); + EXPECT_ERR(err); + write_n_ref_tables(st, 2); + EXPECT(st->merged->readers_len == 2); + reftable_stack_init_ref_iterator(st, &it); + err = reftable_iterator_seek_ref(&it, ""); + EXPECT_ERR(err); + + /* + * Update the tables.list file with some garbage data, while reusing + * our old readers. This should trigger a partial reload of the stack, + * where we try to reuse our old readers. + */ + strbuf_addf(&content, "%s\n", st->readers[0]->name); + strbuf_addf(&content, "%s\n", st->readers[1]->name); + strbuf_addstr(&content, "garbage\n"); + strbuf_addf(&table_path, "%s.lock", st->list_file); + write_file_buf(table_path.buf, content.buf, content.len); + err = rename(table_path.buf, st->list_file); + EXPECT_ERR(err); + + err = reftable_stack_reload(st); + EXPECT(err == -4); + EXPECT(st->merged->readers_len == 2); + + /* + * Even though the reload has failed, we should be able to continue + * using the iterator. + */ + err = reftable_iterator_next_ref(&it, &rec); + EXPECT_ERR(err); + EXPECT(!strcmp(rec.refname, "refs/heads/branch-0000")); + err = reftable_iterator_next_ref(&it, &rec); + EXPECT_ERR(err); + EXPECT(!strcmp(rec.refname, "refs/heads/branch-0001")); + err = reftable_iterator_next_ref(&it, &rec); + EXPECT(err > 0); + + reftable_ref_record_release(&rec); + reftable_iterator_destroy(&it); + reftable_stack_destroy(st); + strbuf_release(&table_path); + strbuf_release(&content); + clear_dir(dir); +} + int stack_test_main(int argc UNUSED, const char *argv[] UNUSED) { RUN_TEST(test_empty_add); @@ -1099,6 +1205,8 @@ int stack_test_main(int argc UNUSED, const char *argv[] UNUSED) RUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully); RUN_TEST(test_reftable_stack_update_index_check); RUN_TEST(test_reftable_stack_uptodate); + RUN_TEST(test_reftable_stack_read_across_reload); + RUN_TEST(test_reftable_stack_reload_with_missing_table); RUN_TEST(test_suggest_compaction_segment); RUN_TEST(test_suggest_compaction_segment_nothing); return 0; |