Re: [PATCH v2 07/10] reftable/stack: adapt `format_name()` to handle allocation failures

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 14, 2024, 22:41 UTC
Message-ID: <Zw2eIbeWUygH9+hn@nand.local>
In-Reply-To: <1f08163009b87596188165863b89b62c5f521e00.1728910727.git.ps@pks.im>

On Mon, Oct 14, 2024 at 03:02:37PM +0200, Patrick Steinhardt wrote:

Show 12 quoted lines

> @@ -846,7 +846,10 @@ int reftable_addition_add(struct reftable_addition *add,
>  	int tab_fd;
>
>  	reftable_buf_reset(&next_name);
> -	format_name(&next_name, add->next_update_index, add->next_update_index);
> +
> +	err = format_name(&next_name, add->next_update_index, add->next_update_index);
> +	if (err < 0)
> +		goto done;
>
>  	stack_filename(&temp_tab_file_name, add->stack, next_name.buf);
>  	reftable_buf_addstr(&temp_tab_file_name, ".temp.XXXXXX");

The conversion to store the return value of 'format_name()' here makes sense. I was going to ask why this call to reftable_buf_addstr() does not have its own return value checked similarly, but I see that it is handled specially a few commits later on.

I think that what you wrote here is fine, but there are a couple of alternatives IMHO that may be worth considering in the future:

  - You could do these conversions function by function, where each
    patch handles all potential allocation failures.

    This generates more patches, but makes each individual patch a
    little easier to review in isolation, since the reviewer does not
    have to page in and out the context of what different functions do,
    etc.

  - Alternatively, you could mention something along the lines of "this
    step does not make any of these functions entirely resilient against
    allocation failures, but future commits will address the remaining
    components" to avoid temporary confusion on the reader's part
    wondering why only part of the code appears to handle allocation
    failures.

Thanks, Taylor