diff mbox series

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

Message ID 20250306121324.1315290-1-meetsoni3017@gmail.com (mailing list archive)
State New
Headers show
Series [GSoC] reftable: return proper error code from block_writer_add() | expand

Commit Message

Meet Soni March 6, 2025, 12:13 p.m. UTC 
Previously, block_writer_add() used to return generic -1, which forced
an assumption about the error type.

Replace generic -1 returns in block_writer_add() and related functions
with defined error codes.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
This patch attempts to avoid making an assumption regarding error codes
returned by block_writer_add().
 reftable/block.c  |  9 +++++----
 reftable/record.c | 16 +++++++++++-----
 reftable/writer.c |  8 +-------
 3 files changed, 17 insertions(+), 16 deletions(-)


base-commit: e969bc875963a10890d61ba84eab3a460bd9e535

Comments

Patrick Steinhardt March 6, 2025, 2:43 p.m. UTC | #1 
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
diff mbox series

Patch

diff --git a/reftable/block.c b/reftable/block.c
index b14a8f1259..50fbac801a 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);
@@ -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;
@@ -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);
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);
 
@@ -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;
 }
 
 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,
diff --git a/reftable/writer.c b/reftable/writer.c
index f3ab1035d6..600ba5441b 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -327,16 +327,10 @@  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;
 		goto done;
 	}