httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Akins <brian.ak...@turner.com>
Subject Re: [PATCH] mod_cache. Allow override of some vary headers
Date Wed, 17 Aug 2005 20:06:04 GMT
Justin Erenkrantz wrote:
> The concept is fine.  (And as Joshua pointed out 'CacheVaryOverride' 
> would be a better name.)

I'm not attatched to the name. So that sounds good to me.

> A few issues with the implementation (modulo style nits)...

This was all done in a few hours today, including testing.  Some stuff, 
especially in cache_util, looks really ugly :)


>> +        if((header = apr_table_get(r->subprocess_env, o)) == NULL) {
>> +            header = "-";
>> +        }
>> +    }
> 
> 
> I don't understand what the assignment to "-" is doing here.

Just a default value when the environment is not set.  Need to have a 
default to handle cases when header is not set.  I thought about having 
this default be configurable per override.  In short, "-" was just as 
good a default as any.


>> +    ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
>> +                 "ap_cache_override_header: %s => %s", key, header);
> 
> 
> I don't think its necessary to log in the case where it is a no-op - so 
> I'd stick this in an else branch of the conditional above.

Probably not necessary to log at all.  I left this in here until me/we 
feel confident with it.

>> -static const char* regen_key(apr_pool_t *p, apr_table_t *headers,
>> +static const char* regen_key(request_rec *r,
>>                               apr_array_header_t *varray, const char
>> *oldkey)
> 
> 
> I would prefer that we keep the headers argument explicit rather than 
> hardcoding that it is r->headers_in.  -- justin


Sure.  I just noticed everytime regen_key was called, it was called with 
r->headers_in.


-- 
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Mime
View raw message