Re: [PATCH 01/20] chunk-format: note that pair_chunk() is unsafe

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 10, 2023, 23:45 UTC
Message-ID: <ZSXiJZFnmpzAmuwx@nand.local>
In-Reply-To: <20231009205823.GA3282181@coredump.intra.peff.net>

On Mon, Oct 09, 2023 at 04:58:23PM -0400, Jeff King wrote:

> There are no callers of the "safe" version yet, but we'll add some in
> subsequent patches.

Makes sense.

Show 7 quoted lines

> +int pair_chunk_unsafe(struct chunkfile *cf,
> +		      uint32_t chunk_id,
> +		      const unsigned char **p)
>  {
> -	return read_chunk(cf, chunk_id, pair_chunk_fn, p);
> +	size_t dummy;
> +	return pair_chunk(cf, chunk_id, p, &dummy);

I have always disliked functions that require you to pass a non-NULL pointer to some value that you may or may not want to have that function fill out. So I was going to suggest something along the lines of "pair_chunk() should tolerate a NULL fourth argument instead of filling out the size unconditionally".

But that's (a) the whole point of the series ;-), and (b) unnecessary, since this function is going to go away entirely by the end of the series, too.

So I think that the call you made here was the right one. The rest of the patch all looks good to me, let's read on...

Thanks, Taylor