On Thu, November 22, 2007 4:40 pm, Joe Orton wrote: > General issues throughout this code:
(Some initial observations without the code being in front of me) > - don't check for pool allocation failure Can you point out where exactly - I checked for this specifically very thoroughly, but then looking at the same code continuously for a few days means you will not see all of them. > Drop the @fn stuff. It's unnecessary if you have a correct function > prototype. Will drop throughout the file. > Private keys and certificates can be stored in many formats. The format > this function expects needs to be documented in the API. APR makes no claims as to the format the underlying toolkit accepts, this same problem exists in the apr_ssl* code. We also don't (yet) support the idea of certs being in places other than files. >> + * @param cipherName Name of cipher to use for symmetrical encryption > > Again, the format of this name must be documented in the API; "Advanced > Encryption Standard" is the name of a cipher. Will it work? Again, this is the underlying toolkit's problem. Will document better. > as an output parameter only? How long must the buffer be? Or is it > really an input/output parameter? As I understand this, the length of the final buffer is included by the call that tells you the length of the initial buffer. OpenSSL isn't very clear on this, it needs to be better documented. >> +#if HAVE_DECL_EVP_PKEY_CTX_NEW > > That macro doesn't appear to ever be defined anywhere. See the ssl.m4 file (if memory serves). EVP_PKEY_CTX_new is only available in the as-yet-unreleased OpenSSL v0.9.9. >> +#if HAVE_DECL_EVP_PKEY_CTX_NEW >> + /* load certs */ >> + data->sslCtx = SSL_CTX_new(SSLv23_server_method()); > > Again dead code, and weird code - why mess with an SSL_CTX if doing > encryption? See the OpenSSL v0.9.9 asymmetrical keys support, google for EVP_PKEY_CTX_new. Regards, Graham --
