Re: [GSoC PATCH] reftable: return proper error code from block_writer_add()
From: Patrick Steinhardt <hidden>
Date: 2025-03-06 14:44:04
On Thu, Mar 06, 2025 at 05:43:24PM +0530, Meet Soni wrote:
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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()`.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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