On Thu, Mar 06, 2025 at 05:43:24PM +0530, Meet Soni wrote:
> @@ -115,8 +115,9 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
> int err;
>
> err = reftable_record_key(rec, &w->scratch);
> - if (err < 0)
> + if (err < 0) {
> goto done;
> + }
>
> if (!w->scratch.len) {
> err = REFTABLE_API_ERROR;
This change probably shouldn't be here. Our style guide mentions that we
prefer to not have curly braces around single-line statements.
> @@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
> n = reftable_encode_key(&is_restart, out, last, w->scratch,
> reftable_record_val_type(rec));
> if (n < 0) {
> - err = -1;
> + err = n;
> goto done;
> }
> string_view_consume(&out, n);
>
> n = reftable_record_encode(rec, out, w->hash_size);
> if (n < 0) {
> - err = -1;
> + err = n;
> goto done;
> }
> string_view_consume(&out, n);
Okay. `reftable_encode_key()` right now only knows to return generic
errors, but you fix that further down, and you also adapt
`reftable_record_encode()`.
> diff --git a/reftable/record.c b/reftable/record.c
> index 8919df8a4d..5523804a0c 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> @@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,
> uint64_t suffix_len = key.len - prefix_len;
> int n = put_var_int(&dest, prefix_len);
> if (n < 0)
> - return -1;
> + return REFTABLE_ENTRY_TOO_BIG_ERROR;
> string_view_consume(&dest, n);
>
> *restart = (prefix_len == 0);
>
> n = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);
> if (n < 0)
> - return -1;
> + return REFTABLE_ENTRY_TOO_BIG_ERROR;
> string_view_consume(&dest, n);
>
> if (dest.len < suffix_len)
> - return -1;
> + return REFTABLE_ENTRY_TOO_BIG_ERROR;
> memcpy(dest.buf, key.buf + prefix_len, suffix_len);
> string_view_consume(&dest, suffix_len);
>
Makes sense.
> @@ -1144,14 +1144,20 @@ static struct reftable_record_vtable reftable_index_record_vtable = {
>
> int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest)
> {
> - return reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);
> + int key_len = reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);
> + if (key_len < 0)
> + return REFTABLE_ENTRY_TOO_BIG_ERROR;
> + return key_len;
> }
This here is incorrect. We don't know why the `->key()` function has
failed, so we shouldn't assume `TOO_BIG_ERROR`. We'd have to vet all the
implementations of that function and then should bubble up their
respective error codes.
> int reftable_record_encode(struct reftable_record *rec, struct string_view dest,
> uint32_t hash_size)
> {
> - return reftable_record_vtable(rec)->encode(reftable_record_data(rec),
> + int encode_len = reftable_record_vtable(rec)->encode(reftable_record_data(rec),
> dest, hash_size);
> + if (encode_len < 0)
> + return REFTABLE_ENTRY_TOO_BIG_ERROR;
> + return encode_len;
> }
>
> int reftable_record_copy_from(struct reftable_record *rec,
Same remark here.
Thanks!
Patrick
