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