Re: [PATCH v2 0/8] hash: introduce unsafe_hash_algo(), drop unsafe_ variants

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 10, 2025, 21:29 UTC
Message-ID: <Z4GRQvAIsSIeQuYd@nand.local>
In-Reply-To: <20250110104106.GB1014709@coredump.intra.peff.net>

On Fri, Jan 10, 2025 at 05:41:06AM -0500, Jeff King wrote:

Show 24 quoted lines

> On Wed, Jan 08, 2025 at 02:14:29PM -0500, Taylor Blau wrote:
>
> > (This series is rebased on 'master', which is 14650065b7
> > (RelNotes/2.48.0: fix typos etc., 2025-01-07) at the time of writing).
> >
> > The bulk of this series is unchanged since last time, but a new seventh
> > patch that further hardens the hashfile_checkpoint callers on top of
> > Patrick's recent series[1].
>
> I think that new patch is a definite improvement, though I left some
> comments there.
>
> The changes in patch 1 look fine to me (I still think a generic
> "test-tool hash" would make more sense, but I'm OK punting on that for
> now).
>
> I didn't see any response to the review in round 1 about the pointer
> dangers in patch 3. What do you think of using a separate
> git_hash_algo_fns struct, with the one-time conversion I showed in the
> subthread of:
>
>   https://lore.kernel.org/git/20241121093731.GD602681@coredump.intra.peff.net/
>
> ?

Oops. I must have missed those messages; and sure enough when focusing my inbox on this thread they are indeed unread :-).

That said, I am not sure that that direction is one that I'd want to go in. Part of the goal of this series is to make it possible to mix safe and unsafe function calls on the same hash function. So doing something like:

    struct git_hash_algo *algop;

    algop->init_fn(&ctx);

in one part of the code, and then (using the same algop) calling:

    algop->unsafe_final_fn(...);

should be impossible to do to. The benefit of having only a single set of functions implemented on the git_hash_algo type is that it is impossible to mix the two: you'd have to use a different git_hash_algo altogether!

So porting the above example to your and brian's git_hash_algo_fns struct, you'd still be able to do:

    algop->fn.init(&ctx);

in one part of the code and algop->unsafe_fn.final(...) in another part, which doesn't appear to me to be safer than the current situation that this series aims to solve.

But if I am misunderstanding something about your/brian's discussion earlier in this thread, please feel free to correct me.

Thanks, Taylor