Re: [GSoC PATCH] reftable: return proper error code from block_writer_add()
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 6, 2025, 14:43 UTC
- Message-ID
- <Z8m0rnTr2PrqBUKQ@pks.im>
- In-Reply-To
- <20250306121324.1315290-1-meetsoni3017@gmail.com>
On Thu, Mar 06, 2025 at 05:43:24PM +0530, Meet Soni wrote:
Show 11 quoted lines
> @@ -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.
Show 17 quoted lines
> @@ -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()`.
Show 26 quoted lines
> 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.
Show 10 quoted lines
> @@ -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.
Show 12 quoted lines
> 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