rsaenh

[Juan Lang]

Hi Michael, that was quick!  Okay, more feedback:

1. English note: destruct_handle_table really should be
destroy_handle_table.  I know, "destructor" came into common use, but the
verb is to destroy.  Same with destruct_key, destruct_hash, etc.

2. A CSP isn't allocating a "real" handle, it's sort of a pseudohandle, so
the multiple-of-four business isn't necessary.  When I was testing SSPs, I
found some of them return 0 as a valid handle value.  So I think using the
index directly would be fine.

3. Do you really need to separate all the functions into an impl version
and a non-impl version?  It looks as though the non-impl version does the
locking, while the impl assumes the table is locked.  It seems you could
at least document this, and it also doesn't seem necessary in every case,
if at all--EnterCriticalSection won't deadlock if a thread that's already
entered it calls it again.

4. Some of the OpenSSL checks don't seem valid.  In init_hash_impl, for
example, you fail if libcrypto isn't available, even if it isn't necessary
for the hash.  Please fail only if you really need an OpenSSL func.  E.g.,
if (!pMD2_Init) { SetLastError()... } seems better to me.

5. This may be open to debate, but as far as a "more elegant way": perhaps
you should protect each typedef with the header file that defines it, e.g.
in HASH_CONTEXT, rather than:
#ifdef HAVE_OPENSSL
    MD2_CTX md2;
#endif
you could use:
#ifdef HAVE_OPENSSL_MD2_H
    MD2_CTX md2;
#endif
This way you could support the algorithms you were compiled with, even if
they aren't the entire set.

Sorry I didn't mention these last time..
--Juan


		
__________________________________
Do you Yahoo!?
Yahoo! Mail Address AutoComplete - You start. We finish.
http://promotions.yahoo.com/new_mail