On Sat, Mar 08, 2025 at 07:03:49PM +0530, Meet Soni wrote:
> diff --git a/reftable/block.c b/reftable/block.c
> index b14a8f1259..89ab8bbc57 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
> if (is_restart)
> rlen++;
> if (2 + 3 * rlen + n > w->block_size - w->next)
> - return -1;
> + return REFTABLE_ENTRY_TOO_BIG_ERROR;
> if (is_restart) {
> REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
> w->restart_cap);
> @@ -97,9 +97,9 @@ uint8_t block_writer_type(struct block_writer *bw)
> return bw->block[bw->header_off];
> }
>
> -/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
> - success. Returns REFTABLE_API_ERROR if attempting to write a record with
> - empty key. */
> +/* Adds the reftable_record to the block. Returns 0 on success and
> + * appropriate error codes on failure.
> + */
> int block_writer_add(struct block_writer *w, struct reftable_record *rec)
> {
> struct reftable_buf empty = REFTABLE_BUF_INIT;
I'm in favor of touching up the comment's formatting while at it, but if
we do so we should use the correct style, which has the opening and
closing parts on their own line:
/*
* Yadda yadda.
*/
> diff --git a/reftable/block.h b/reftable/block.h
> index bef2b8a4c5..0e7c680cf6 100644
> --- a/reftable/block.h
> +++ b/reftable/block.h
> @@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
> /* returns the block type (eg. 'r' for ref records. */
> uint8_t block_writer_type(struct block_writer *bw);
>
> -/* appends the record, or -1 if it doesn't fit. */
> +/* attempts to append the record. returns 0 on success or error code on failure. */
> int block_writer_add(struct block_writer *w, struct reftable_record *rec);
>
> /* appends the key restarts, and compress the block if necessary. */
We might also touch up this comment to start with an upper-case "A"
while at it.
> diff --git a/reftable/record.c b/reftable/record.c
> index 8919df8a4d..d9fba8ff38 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> diff --git a/reftable/writer.c b/reftable/writer.c
> index f3ab1035d6..5cb9d0bf85 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -327,18 +327,11 @@ static int writer_add_record(struct reftable_writer *w,
> goto done;
>
> /*
> - * Try to add the record to the writer again. If this still fails then
> - * the record does not fit into the block size.
> - * TODO: it would be great to have `block_writer_add()` return proper
> - * error codes so that we don't have to second-guess the failure
> - * mode here.
> + * Try to add the record to the writer again.
> */
> err = block_writer_add(w->block_writer, rec);
> - if (err) {
> - err = REFTABLE_ENTRY_TOO_BIG_ERROR;
> + if (err)
> goto done;
> - }
Let's not drop the second sentence of the comment, as it is important to
give context. Also, let's take a step back here and figure out what this
function is doing:
1. We compute the record data and try to append it to the current
block. If this succeeds, we can return immediately and are done.
2. If appending to the current block fails we assume that we have
failed because the block is full. This is because reftable blocks
have a specific maximum length that we cannot exceed. We thus
flush the block and start writing a new one.
3. We now try to add the same record to the new block again and hope
that we can now write the record successfully.
The important part that the TODO comment refers to is in (2), indicated
by "assume": we don't actually check what the error is that we've got
from `block_writer_add()`, but simply pretend as if it was
`REFTABLE_ENTRY_TOO_BIG_ERROR`. This means that we'd even re-try writing
the record in case we had for example a memory allocation failure, or an
I/O error, and that is plain wrong.
With your changes we have now started to plumb proper errors through from
`block_writer_add()`. But that doesn't mean we can just drop the comment
and bubble up the error. Instead, we should also be adapting the code in
(2) to do the right thing: we shouldn't _assume_ that the current block
is full, but instead check the error code returned by the first call to
`block_writer_add()`:
- If it is `REFTABLE_ENTRY_TOO_BIG_ERROR` we indeed should flush the
current block and try to write a new one.
- Otherwise we bail out and bubble up the error.
And once we do that, it is fine to remove the comment indeed. It's
somewhat funny because from my point of view the comment is in the wrong
spot: it does correctly point out that _this_ particular callsite is
doing the wrong thing, but it didn't mention that the other callsite
also has the same problem. And that other callsite is more important
from my perspective.
So I'd recommend to split up this commit into two commits:
- The first commit prepares all transitively called functions as you
already do.
- The second commit adapts "reftable/writer.c" and fixes both
callsites of `block_writer_add()` to do proper error handling.
Patrick
