httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: svn commit: r630974 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_config.c ssl_private.h ssl_scache.c ssl_scache_dbm.c ssl_scache_dc.c ssl_scache_memcache.c ssl_scache_shmcb.c
Date Mon, 25 Feb 2008 21:45:44 GMT
On Mon, Feb 25, 2008 at 09:49:55PM +0100, Ruediger Pluem wrote:
> On 02/25/2008 09:09 PM, jorton@apache.org 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


Mime
View raw message