From: Patryk Obara <patryk.obara@gmail.com>
Date: Sun, 28 Jan 2018 19:57:21 GMT
Subject: Re: [PATCH 02/12] hash: create union for hash context allocation
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:
> 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.

> +/* 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