Re: [GSoC PATCH] reftable: return proper error code from block_writer_add()

[Patrick Steinhardt <ps@pks.im>]

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