On Sun, Jan 28, 2018 at 08:57:21PM +0100, Patryk Obara wrote:
> 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.
The approach I took was to keep the code as similar as possible to
what's there already. If our hash implementation wants to use pointers,
it's okay for it to define git_SHA1_CTX to a pointer type, and
everything should still work. We treat the type as fully opaque anyway
(outside of the actual hash implementation).
> > +/* 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.
Yeah, that does make more sense. I'll make that change.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
Attachment:
signature.asc
Description: PGP signature
