> On 1 Dec 2020, at 06:38, Michael Paquier <mich...@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
