On 25/03/31 10:41AM, Patrick Steinhardt wrote:
> The logic to read blocks from a reftable is scattered across both the
> table and the block subsystems. Besides causing somewhat fuzzy
> responsibilities, it also means that we have to awkwardly pass around
> the ownership of blocks between the subsystems.
>
> Refactor the code so that we stop passing the block when initializing a
> reader, but instead by passing in the block source plus the offset at
> which we we're supposed to read a block. Like this, the ownership of the
s/we we're/we're/
> block itself doesn't need to get handed over as the block reader is the
> one owning the block right from the start.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> reftable/block.c | 87 ++++++++++++++++++++++++++---------------
> reftable/block.h | 8 ++--
> reftable/table.c | 65 +++---------------------------
> t/unit-tests/t-reftable-block.c | 76 ++++++++++++++++++-----------------
> 4 files changed, 107 insertions(+), 129 deletions(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index f2567a8f0fd..2517108b8ef 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -209,31 +209,57 @@ int block_writer_finish(struct block_writer *w)
> return w->next;
> }
>
> -int block_reader_init(struct block_reader *br, struct reftable_block *block,
> - uint32_t header_off, uint32_t table_block_size,
> - uint32_t hash_size)
> +static int read_block(struct reftable_block_source *source,
> + struct reftable_block *dest, uint64_t off,
> + uint32_t sz)
> {
> + size_t size = block_source_size(source);
> + block_source_return_block(dest);
> + if (off >= size)
> + return 0;
> + if (off + sz > size)
> + sz = size - off;
> + return block_source_read_block(source, dest, off, sz);
> +}
> +
> +int block_reader_init(struct block_reader *br,
> + struct reftable_block_source *source,
> + uint32_t offset, uint32_t header_size,
> + uint32_t table_block_size, uint32_t hash_size)
> +{
> + uint32_t guess_block_size = table_block_size ?
> + table_block_size : DEFAULT_BLOCK_SIZE;
Out of curiousity, in what scenarios would the table not know the block
size and we have to rely on the guess?
> uint32_t full_block_size = table_block_size;
> - uint8_t typ = block->data[header_off];
> - uint32_t sz = reftable_get_be24(block->data + header_off + 1);
> uint16_t restart_count;
> uint32_t restart_off;
> + uint32_t block_size;
> + uint8_t block_type;
> int err;
>
> - block_source_return_block(&br->block);
> + err = read_block(source, &br->block, offset, guess_block_size);
> + if (err < 0)
> + goto done;
Ok, so now `block_reader_init()` handles reading the block itself and no
longer relies on the read block being provided to it. This makes block
ownership more self-contained and clear.
> - if (!reftable_is_block_type(typ)) {
> - err = REFTABLE_FORMAT_ERROR;
> + block_type = br->block.data[header_size];
> + if (!reftable_is_block_type(block_type)) {
> + err = REFTABLE_FORMAT_ERROR;
> goto done;
> }
>
> - if (typ == BLOCK_TYPE_LOG) {
> - uint32_t block_header_skip = 4 + header_off;
> - uLong dst_len = sz - block_header_skip;
> - uLong src_len = block->len - block_header_skip;
> + block_size = reftable_get_be24(br->block.data + header_size + 1);
> + if (block_size > guess_block_size) {
> + err = read_block(source, &br->block, offset, block_size);
> + if (err < 0)
> + goto done;
> + }
Instead of relying on `table_init_block_reader()` to determine if
`guess_block_size` was correct and reread the block, this is now handled
as part of the initial `block_reader_init()`. Make sense.
[snip]
