> On 20 Nov 2020, at 01:33, Michael Paquier <mich...@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
