Re: [PATCH 05/16] reftable/table: move reading block into block reader
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Apr 2, 2025, 20:13 UTC
- Message-ID
- <umaolgemnks4g4cgwgnurl7alic2a3pzrfx2ws3s7uxypiedia@7mwqxvhw2zxg>
- In-Reply-To
- <20250331-pks-reftable-polishing-v1-5-ebed5247434c@pks.im>
On 25/03/31 10:41AM, Patrick Steinhardt wrote:
Show 8 quoted lines
> 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/
Show 42 quoted lines
> 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@pks.im>
> ---
> 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?
Show 13 quoted lines
> 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.
Show 18 quoted lines
> - 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]