On Mon, Nov 30, 2020 at 01:43:24PM +0100, Daniel Gustafsson wrote:
> Looks good, nothing major sticks out.
Thanks for the review.
> I'm not excited about all the allocations needed here now, but it
> seems the only option.
Yeah, OpenSSL forces a bit our hand here I am afraid..
> Some of these long if-statements omit the {} around the elog, while some (like
> this one) don't. I think it makes sense from a readability POV to use the
> braces.
Agreed.
> + * pg_cryptohash_create
> + *
> + * Allocate a hash context. Returns NULL on failure.
> This comment should perhaps be amended to specify that it returns NULL on
> failure in the frontend, in the backend it won't return on error.
Okay. Let's be precise.
> + case PG_SHA224:
> + pg_sha224_update((pg_sha224_ctx *) ctx->data, data, len);
> + break;
> + case PG_SHA256:
> + pg_sha256_update((pg_sha256_ctx *) ctx->data, data, len);
> + break;
> Would codepaths like these become more readable if pg_cryptohash_ctx used a
> union with shaxxx_ctx's rather than a void pointer for the data part?
Hmm. I am not sure that this would help much, because we'd still need
to cast to the local structures in this case as the pg_shaXXX_ctx are
local to sha2.c, and we still need to keep some void* in
cryptohash.h.
> Since the cryptohash support is now generalized behind an abstraction layer,
> wouldn't it make sense to roll the resource ownership there as well kind of
> like how JIT is handled? It would make it easier to implement TLS backend
> support, and we won't have to inject OpenSSL headers here.
So, you are referring here about using a new API in the abstraction
layer. This makes sense. What about naming that
pg_cryptohash_context_free(void *)?
--
Michael
