Re: [PATCH 02/12] hash: create union for hash context allocation

> 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.