httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Querna <c...@force-elite.com>
Subject Re: svn commit: r545379 - in /httpd/httpd/trunk: CHANGES modules/ssl/config.m4 modules/ssl/ssl_engine_config.c modules/ssl/ssl_private.h modules/ssl/ssl_scache.c modules/ssl/ssl_scache_memcache.c
Date Wed, 13 Jun 2007 02:27:46 GMT
Ruediger Pluem wrote:
>> +#else
>> +        return "SSLSessionCache: distcache support disabled";
> 
> Nitpicking: Should memcache instead of distcache.

Fixed in r545538.


>> Added: httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c?view=auto&rev=545379
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c (added)
>> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c Thu Jun  7 19:48:04 2007
>> @@ -0,0 +1,289 @@
....
>> +        if (port == 0) {
>> +            port = 11211; /* default port */
> 
> Shouldn't we use a define for the default port and refer to it instead of using it directly?
> 

Fixed in r546707.

>> +        /* Should Max Conns be (thread_limit / nservers) ? */
>> +        rv = apr_memcache_server_create(p,
>> +                                        host_str, port,
>> +                                        0,
>> +                                        1,
>> +                                        thread_limit, 
>> +                                        600,
>> +                                        &st);
> 
> Having at least some of the values configurable would be an enhancement (Sorry for wanting
to have
> everything perfect from the start :-))
> Nevertheless shouldn't we use defines for these default values (except thread_limit of
course)
> and refer to them instead of using the values directly?

I've made them over-ridable compile time #defines for now -- making them
accessible from the config file with a reasonable syntax seems like a
painful challenge right now. (SSLSessionCache atm can only be configured
on a signle command/line atm).


>> +
>> +static char *mc_session_id2sz(unsigned char *id, int idlen,
>> +                               char *str, int strsize)
>> +{
>> +    char *cp;
>> +    int n;
>> + 
>> +    cp = apr_cpystrn(str, MC_TAG, MC_TAG_LEN);
>> +    for (n = 0; n < idlen && n < (MC_KEY_LEN - MC_TAG_LEN); n++) {
> 
> Hm. It seems to me that our upper limit for n is (MC_KEY_LEN - MC_TAG_LEN)/2
> and not (MC_KEY_LEN - MC_TAG_LEN) since every element of id consumes 2 bytes in the
> target buffer. Furthermore I would think that using (strsize - MC_TAG_LEN)/2
> as a precalculated value would be even bettter.
> 
>> +        apr_snprintf(cp, strsize - (cp-str), "%02X", id[n]);
> 
> Couldn't we replace
> 
> strsize - (cp-str)
> 
> just with
> 
> 2
> 
> once the loop condition is fixed?

Yup.  Changed in r546708.

....
>> +SSL_SESSION *ssl_scache_mc_retrieve(server_rec *s, UCHAR *id, int idlen)
>> +{
>> +    SSL_SESSION *pSession;
>> +    MODSSL_D2I_SSL_SESSION_CONST unsigned char *pder;
>> +    apr_size_t der_len;
>> +    SSLModConfigRec *mc = myModConfig(s);
>> +    char buf[MC_KEY_LEN];
>> +    char* strkey = NULL;
>> +    apr_status_t rv;
>> +
>> +    strkey = mc_session_id2sz(id, idlen, buf, sizeof(buf));
>> +
>> +    if(!strkey) {
>> +        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
>> +                     "scache_mc: Key generation borked.");
>> +        return NULL;
>> +    }
>> +
>> +    rv = apr_memcache_getp(memctxt,  mc->pPool, strkey,
> 
> Is this the correct pool to use? AFAICT the data stored in *pder
> will only be used locally, because d2i_SSL_SESSION allocates
> new memory to create the pSession object.
> mc->pPool seems to be a pool with a long lifetime to me and I fear
> that we introduce a memory leak here. How about a temporary subpool
> if no other pool is at hand here?

I've re-worked pool allocations for both memcache and DBM backends, in
r545608 and r545610, but there are still some places in the DBM cache
where allocate from the configuration pool; However we should be good
now in the memcache cache backend.

....
>> +    /* ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
>> +        "scache_mc: 'get_session' HIT"); */
> 
> Why the comments above?

Useful for development/debugging, but they would be be hit 100% if they
weren't commented out....

Thank You for reviewing,

-Paul


Mime
View raw message