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
Junio C Hamano March 6, 2025, 5:04 p.m. UTC | #2 
Meet Soni <meetsoni3017@gmail.com> writes:

> 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.

What's missing from this proposed log message is an audit of the
callers to tell readers that this change is safe and expected by the
callers.  IOW, are there callers that start to behave differently
when they see ENTRY_TOO_BIG instead of -1, for example?

> 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(-)
>
> 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;

So this makes block_writer_register_restart() to return -11 instead
of -1; the sole caller of the function is block_writer_add() that
begins like so:

        /* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
           success. Returns REFTABLE_API_ERROR if attempting to write a record with
           empty key. */
        int block_writer_add(struct block_writer *w, struct reftable_record *rec)
        {

Needless to say, the comment before the function needs to be
adjusted together with the above hunk (and others).  But more
importantly, are existing callers of this function now expected to
adjust to the change in behaviour when they receive the return value
of this function?  It used to be sufficient for them to deal with
-1, 0 or API_ERROR, but now they are required to handle other errors
(like the one that comes back from reftable_encode_key().  Do they
already handle these new error codes just fine?  Have you traced the
code paths to see how they react to them?

> @@ -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;
> +	}

This is unwarranted, isn't it?

> @@ -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;
>  	}

Here, block_writer_add() starts returning new error codes to its
callers that it never returned.

>  	string_view_consume(&out, n);
>  
>  	n = reftable_record_encode(rec, out, w->hash_size);
>  	if (n < 0) {
> -		err = -1;
> +		err = n;
>  		goto done;
>  	}

Ditto.

Note that I am not saying that it is a bad idea to make the error
codes more specific so that the callers can tell them apart.  I am
only saying that the patch that makes such a change must also make
sure that the callers are prepared to handle error coes that they
have never seen from the current callee.

The same applies to the remainder of the patch.

Thanks.
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;
 	}