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
