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: r592951 - in /httpd/httpd/trunk: ./ docs/manual/mod/ include/ modules/filters/ modules/http/ server/
Date Sat, 20 Sep 2008 05:26:41 GMT
Ruediger Pluem wrote:
> On 02.12.2007 02:10, Graham Leggett wrote:
>> Nick Kew wrote:
>>
>>> You created a new kept_body input filter, which seems exactly right
>>> to me.  But you're storing the kept body on the request_rec itself,
>>> which looks like extra complexity.
>>>
>>> Why not make the new filter into its own module, and keep the kept_body
>>> brigade on the filter's own config?  That way the complexity of dealing
>>> with top-level vs other requests is kept in one place.  It's not clear
>>> to me from the changes in http_filter.h that this patch deals with
>>> that distinction (or is kept_body supposed to be able to originate
>>> in non-origin requests)?
>>
>> The kept body stuff is formed of two parts, the bit that sets aside 
>> the body (in ap_discard_request_body()), and the filter that puts it 
>> back again. It didn't make sense to separate the two functions, which 
>> is why they are both in the same source file, the http module 
>> http_filters.c.
>>
>>> On principle, we should be reducing the request_rec, not adding to it!
>>
>> If we can find a way to avoid it, certainly.
>>
>> The kept body filter is only added on subrequests, and only where 
>> r->kept_body is not-NULL, and subrequests as I recall have their own 
>> filter stack separate from the main filter stack. So as I understand 
>> it, linking it to the subrequest filter stack may not work, as the 
>> subrequest filter stack doesn't exist when ap_discard_request_body() 
>> is called.
> 
> First of all sorry for commenting on this that late. I should have paid 
> more attention back in December.
> 
> While it is true that the filter stack branches under certain conditions 
> for
> output filters, the same is not true for input filters. Given Joe's 
> recent true
> comment that ap_discard_request_body pulls all *through* the input 
> filter chain
> the correct (or more correct) solution for the problem seems to be to
> 
> 1. Move the saving of the request body from ap_discard_request_body to a
>    separate input filter that gets implemented by a module.
> 2. Use the insert filter_hook to insert this filter whenever configured.
> 3. I guess the filter can decide on its own what to do: If it is a 
> subrequest
>    pulling from it, then deliver from the buffered brigade otherwise 
> buffer it
>    and pass it on. Of course it has to paid attention in this case for
>    incomplete request bodies and how to handle this.
> 
> IMHO this would make it possible to remove this stuff from the core and 
> from
> the request_rec where it does not really belong.

+1, this revision r592951, and the derivate r647263 need to be 
rethought.  IMO is that this doesn't belong in the reqeust_rec.

I'm only realizing this went in 10 months after the first commit, and 5 
months after it was rewritten in r647263, a sign of my inactivity in 
httpd land, so I'm not gonna 'veto' it, but damn I really don't like 
having this stuff on the reqeust_rec.

-Paul

Mime
View raw message