On Fri, Jan 10, 2025 at 05:37:56AM -0500, Jeff King wrote:
> So in the new constructor:
>
> > +void hashfile_checkpoint_init(struct hashfile *f,
> > + struct hashfile_checkpoint *checkpoint)
> > +{
> > + memset(checkpoint, 0, sizeof(*checkpoint));
> > + f->algop->init_fn(&checkpoint->ctx);
> > +}
>
> ...should we actually record "f" itself? And then in the existing
> functions:
>
> > void hashfile_checkpoint(struct hashfile *f, struct hashfile_checkpoint *checkpoint)
>
> ...they'd no longer need to take the extra parameter.
>
> It creates a lifetime dependency of the checkpoint struct on the "f" it
> is checkpointing, but I think that is naturally modeling the domain.
Thanks, I really like these suggestions. I adjusted the series
accordingly to do this cleanup in two patches (one for
hashfile_checkpoint(), another for hashfile_truncate()) after the patch
introducing hashfile_checkpoint_init().
> A semi-related thing I wondered about: do we need a destructor/release
> function of some kind? Long ago when this checkpoint code was added, a
> memcpy() of the sha_ctx struct was sufficient. But these days we use
> clone_fn(), which may call openssl_SHA1_Clone(), which does
> EVP_MD_CTX_copy_ex() under the hood. Do we have any promise that this
> doesn't allocate any resources that might need a call to _Final() to
> release (or I guess the more efficient way is directly EVP_MD_CTX_free()
> under the hood).
>
> My reading of the openssl manpages suggests that we should be doing
> that, or we may see leaks. But it may also be the case that it doesn't
> happen to trigger for their implementation.
>
> At any rate, we do not seem to have such a cleanup function. So it is
> certainly an orthogonal issue to your series. I wondered about it here
> because if we did have one, it would be necessary to clean up checkpoint
> before the hashfile due to the lifetime dependency I mentioned above.
I like the idea of a cleanup function, but let's do so in a separate
series.
Thanks,
Taylor
