On Mon, Feb 25, 2008 at 09:49:55PM +0100, Ruediger Pluem wrote:
> On 02/25/2008 09:09 PM, [EMAIL PROTECTED] wrote:
>> Author: jorton
>> Date: Mon Feb 25 12:09:38 2008
>> New Revision: 630974
>>
>> URL: http://svn.apache.org/viewvc?rev=630974&view=rev
>> Log:
>> Session cache interface redesign, Part 4:
...
>> --- httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c (original)
>> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c Mon Feb 25 12:09:38 2008
...
>> +static const char *ssl_scache_dbm_create(void **context, const char *arg,
>> + apr_pool_t *tmp, apr_pool_t *p)
>> +{
>> + struct context *ctx;
>> +
>> + *context = ctx = apr_pcalloc(p, sizeof *ctx);
>
> Hm. Don't we need to set ctx->pool to p or create a subpool of p and
> assign it to ctx->pool?
Yes indeed, I already found this out with a little trip through gdb.
Fixed in r630990.
>> @@ -274,12 +291,12 @@
>> /* and delete it from the DBM file */
>> ssl_mutex_on(s);
>
> Any particular reason why we do not clear ctx->pool here?
I missed that, and it wasn't even using ctx->pool later; thanks!
...
>> --- httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c (original)
>> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c Mon Feb 25 12:09:38 2008
>> @@ -49,22 +49,28 @@
>> */
>> struct context {
>> + /* Configured target server: */
>> + const char *target;
>> + /* distcache client context: */
>> DC_CTX *dc;
>> };
>> -static apr_status_t ssl_scache_dc_init(server_rec *s, void **context,
>> apr_pool_t *p)
>> +static const char *ssl_scache_dc_create(void **context, const char *arg,
>> + apr_pool_t *tmp, apr_pool_t *p)
>> {
>> - DC_CTX *dc;
>> - SSLModConfigRec *mc = myModConfig(s);
>> struct context *ctx;
>> - /*
>> - * Create a session context
>> - */
>> - if (mc->szSessionCacheDataFile == NULL) {
>> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "SSLSessionCache
>> required");
>> - return APR_EINVAL;
>> - }
>
> Why don't we check any if arg == NULL as a replacement for the if statement
> above?
Sorry, I should have mentioned this in the changelog message. In both
cases of this, the code will always be passed a non-NULL arg parameter
unless a pstrdup fails (which by policy will be ignored anyway). I
think these checks may have just been a copy'n'paste legacy from the
shmcb code, which has genuine config argument parsing failure cases.
Again, thanks for the detailed review!
Regards,
joe
