Re: [PATCH 02/12] hash: create union for hash context allocation
- From
Patryk Obara <patryk.obara@gmail.com>
- Date
- Jan 28, 2018, 19:57 UTC
- Message-ID
- <54b29396-d994-7afd-9c18-83fca8e4ad6f@gmail.com>
- In-Reply-To
- <20180128155722.880805-3-sandals@crustytoothpaste.net>
On 28/01/2018 16:57, brian m. carlson wrote:
Show 9 quoted lines
> In various parts of our code, we want to allocate a structure > representing the internal state of a hash algorithm. The original > implementation of the hash algorithm abstraction assumed we would do > that using heap allocations, and added a context size element to struct > git_hash_algo. However, most of the existing code uses stack > allocations and conversion would needlessly complicate various parts of > the code. Add a union for the purpose of allocating hash contexts on > the stack and a typedef for ease of use. Remove the ctxsz element for > struct git_hash_algo, which is no longer very useful.
Overall, I am OK with this approach (it's straightforward change and cleanest way to replace direct calls to git_SHA1_* functions), but just to play devil's advocate: OpenSSL decided to sway users into heap allocated contexts, citing binary compatibility issues if they change the size of context structure. [1]
I think we might need to revisit this design decision in future - perhaps as soon as we'll transition away from calling git_SHA1_* functions directly.
Show 9 quoted lines
> +/* A suitably aligned type for stack allocations of hash contexts. */
> +union git_hash_ctx {
> + git_SHA_CTX sha1;
> +};
> +typedef union git_hash_ctx git_hash_ctx;
> +
> typedef void (*git_hash_init_fn)(void *ctx);
> typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);
> typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);I think it would be appropriate to replace "void *ctx" with "git_hash_ctx *ctx". This way we can avoid unnecessary casting in git_hash_sha1_* functions.
[1] https://wiki.openssl.org/index.php/Manual:EVP_DigestInit(3)#NOTES
-- | ← Ceci n'est pas une pipe Patryk Obara