rsaenh

Michael Jung mjung at iss.tu-darmstadt.de
Wed Oct 27 03:25:59 CDT 2004


On Wednesday 27 October 2004 03:33, Juan Lang wrote:
> 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.
Sure, no problem.

> 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.
I would rather keep this, since it isn't really a performance problem. Perhaps 
sometime somebody else will use the code with "real" handles. This handle 
managing code is actually another example where I would love to have a 
library, which would be linked statically by multiple dlls. There are at 
least three distinct handle table implementations in wine. 

> 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.
When I wrote the code I thought a double call to EnterCriticalSection on the 
same mutex might deadlock. I later found out that I doesn't, but thought that 
it might be faster to avoid multiple calls to EnterCriticalSection, since 
those call wineserver. Is this the case? Otherwise, I will be happy to remove 
the _impl functions. 

> 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.
Are you saying that we should consider the case where we actually have a 
libcrypto.so at runtime, but which only exports a subset of the functions 
rsaenh was compiled with?

> 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.
Now, this would mean we are considering the case, where we only have a subset 
of the libssl-dev headers at compile time, right?

Thanks for your comments,
Michael



More information about the wine-devel mailing list