Sorry I haven't sent out this review sooner. Broader issues:
** Having both apr_crypto_init() and apr_crypto_get_driver() seems to be redundant (unnecessary complexity in API and code). The lowest common denominator in initialization is NSS, which requires process-global configuration when initialized. You cannot obtain a "driver" of either crypto toolkit which is configured differently to how it was initialized by apr_crypto_init(). ** Is the caller of this code expected to be crypto-toolkit agnostic or not? I am struggling to imagine in Fedora, why we'd want to build APR(-util) with support for *both* crypto toolkits at run-time. Why not just pick one at build time, like every other project in the entire world does? ** Generally the naming of the object types exposed seems to be highly non-intuitive. A "driver" creates a "factory" -- so APR has a genuine factory-building-factory! Joel Spolsky would be proud. A "factory" creates a "block context". A "block context" can encrypt/decrypt data. So a "block context" is really a "cipher context"? Anybody with a passing knowledge of crypto would understand the latter but not the former, and still be bewildered by the use of "driver" and "factory". Specific issues: ** initialization via "params" array headers everywhere is completely unclear. there's no indication of what those params arrays should contain for either init/get_driver or apr_crypto_factory. ** the structure used to represent the "factory" object doesn't use "factory" in its name, which is obscure. also, why it is not opaque? Likewise the naming of apr_crypto_cleanup without reference to the "factory" is non-intuitive. ** given that a factory has a one-to-one relationship with a driver (right?) why must the poor caller pass both into functions? ** similarly for apr_crypto_block_t * object; a one->one relatioship between the "block" context and the driver, so why pass both any time? ** the description of the "factory" mentions keys, certs, and algorithms, but none of the backends appear to do anything at all with keys, certs, and algorithms in the factory init function. ** enum constants in apr_crypto_block_key_type_e and apr_crypto_block_key_mode_e are not namespace-safe (lack APR_ prefix) ** various code style issues: a) the brace following the function definition should come on the new-line. line-wrapped function definitions are not indented correctly - see the example at http://httpd.apache.org/dev/styleguide.html wrong: static apr_status_t crypto_init(apr_pool_t *pool, const apr_array_header_t *params, int *rc) { correct: static apr_status_t crypto_init(apr_pool_t *pool, const apr_array_header_t *params, int *rc) { b) lots of unnecessary casts from void *. don't do that. c) lots of apr_*alloc return value checks. don't do that. d) also some chunks of commented-out code ** there should be #defines for the driver names, so the consumer of the API doesn't have to pluck them out of the air, if the consumer is expected to use those directly. ** the apu_err_t structure seems to be unnecessary, and pretending this is more general by putting it in apu_errno.h is wrong. It would be equivalent, and better API (no structures exposed, so easier to extend in the future) to have: void apr_crypto_error(const apr_crypto_t *f, const char **reason, const char **message, int *rc); to return whatever is stored in f->result. note also that the OpenSSL backend initializes this structure but never uses it AFAICT. Returning the "int rc" at all is wrong because it's exposing an error code specific to the underlying toolkit, breaking the abstraction, per previous discussion. ** there are a bunch of #defines which are unused: APR_CRYPTO_CERT_BASE64, APR_CRYPTO_CERT_PFX, ...
