> On 1 Dec 2020, at 06:38, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Nov 30, 2020 at 02:29:29PM +0100, Daniel Gustafsson wrote:
>> Yeah, that's along the lines of what I was thinking of.
>
> Hmm. I have looked at that, and thought first about having directly a
> reference to the resowner directly in pg_cryptohash_ctx, but that's
> not a good plan for two reasons:
> - common/cryptohash.h would get knowledge of that, requiring bundling
> in a bunch of dependencies.
> - There is no need for that in the non-OpenSSL case.
> So, instead, I have been thinking about using an extra context layer
> only for cryptohash_openssl.c with a structure saved as
> pg_cryptohash_context->data that stores the information about
> EVP_MD_CTX* and the resource owner. Then, I was thinking about
> storing directly pg_cryptohash_ctx in the resowner EVP array and just
> call pg_cryptohash_free() from resowner.c without the need of an
> extra routine. I have not tested this idea but that should work.
> What's your take?
That sounds like it would work. Since the cryptohash implementation owns and
controls the void *data contents it can store whatever it needs to properly
free it.
> In parallel, I have spent more time today polishing and reviewing 0001
> (indented, adjusted a couple of areas and added also brackets and
> extra comments as you suggested) and tested it on Linux and Windows,
> with and without OpenSSL down to 1.0.1, the oldest version supported
> on HEAD. So I'd like to apply the attached first and sort out the
> resowner stuff in a next step.
+1 on separating the API, EVP migration, resowner patches.
Reading through the v6 patch I see nothing sticking out and all review comments
addressed, +1 on applying that one and then we'll take if from there with the
remaining ones in the patchset.
cheers ./daniel