Re: [PATCH v2 7/8] csum-file: introduce hashfile_checkpoint_init()
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jan 10, 2025, 21:50 UTC
- Message-ID
- <Z4GWIZkJOUa278VA@nand.local>
- In-Reply-To
- <20250110103756.GA1014709@coredump.intra.peff.net>
On Fri, Jan 10, 2025 at 05:37:56AM -0500, Jeff King wrote:
Show 18 quoted lines
> 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().
Show 17 quoted lines
> 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