> On 20 Nov 2020, at 01:33, Michael Paquier <michael@paquier.xyz> wrote:
>> This seems like a complication which brings little benefit since the only real
>> errorcase is OOM in creating the context? The built-in crypto support is
>> designed to never fail, and reading the OpenSSL code the only failure cases are
>> ENGINE initialization (which we don't do) and memory allocation. Did you
>> consider using EVP_MD_CTX_init instead which place the memory allocation
>> responsibility with the caller? Keeping this a void API and leaving the caller
>> to decide on memory allocation would make the API a lot simpler IMHO.
>
> Yes. That's actually the main take and why EVP callers *have* to let
> OpenSSL do the allocation: we cannot know the size of EVP_MD_CTX in
> advance in newer versions,
Yeah, there is that.
> knowing that we still need to deal with the OOM failure handling
> and pass down the error to the callers playing with SHA2, it feels
> like the most consistent API to me for the frontend and the backend.
For the backend I'd prefer an API where the allocation worked like palloc, if
not then it's a straight exit through the giftshop. But if we want an API for
both the frontend and backend, I guess this is what we'll have to do.
>> +#ifndef _PG_CRYPTOHASH_H_
>> +#define _PG_CRYPTOHASH_H_
>> This should probably be CRYPTOHASH_H to be consistent?
>
> cryptohash.h sounds like a header file we could find somewhere else,
> hence the extra PG_.
Ok, then at least I think we should use PG_CRYPTOHASH_H to be more consistent
with the tree, and since leading underscores in C are problematic spec-wise.
>> +/* Include internals of each implementation here */
>> +#include "sha2.c"
>> Do we really want to implement this by including a .c file? Are there no other
>> options you see?
>
> That was the least intrusive option I could figure out. Two other
> options I have thought about:
> - Compile those fallback implementations independently and publish the
> internals in a separate header, but nobody is going to need that if we
> have a generic entry point.
> - Include the internals of each implementation in cryptohash.c, but
> this bloats the file with more implementations added (HMAC and MD5
> still need to be added on top of SHA2), and it messes up with the
> existing copyright entries.
> So splitting and just including them sounds like the cleanest option
> of the set.
Personally I think the first option of using an internal header seems cleaner,
but MMV so I'll leave it to others to weigh in too.
cheers ./daniel
