Thanks a lot for the review! On Sat, Mar 17, 2007 at 04:30:24PM +0100, Ruediger Pluem wrote: > Some comments from my side: > > - Passing empty brigades: While I agree that a filter should never create > an empty brigade and pass it down the chain, I think it actually should > pass an empty brigade down the chain if it gets one passed instead of > simply returning. For reasons why see > > http://issues.apache.org/bugzilla/show_bug.cgi?id=40090 > http://mail-archives.apache.org/mod_mbox/httpd-dev/200607.mbox/[EMAIL > PROTECTED]
This is interesting, but I don't really understand how the problem described happens. How is ap_finalize_request_protocol() getting called before a response has been sent? (there is some symmetry between that problem and PR 38014) > - apr_brigade_destroy: I think the danger about using apr_brigade_destroy is > to call it and *reuse* this brigade afterwards, because in this case the > removed pool cleanup can cause a memory leak. Right - and that should be true of every brigade which a filter uses (the passed-in brigade should not be destroyed; any brigades the filter itself creates *must* be long-lived and re-used). So there's really no case in which to recommend use of apr_brigade_destroy() for a filter. It's almost always safer to use _cleanup. I'll add a note about using _cleanup() to this section. > - Procesing buckets: I think with mmap enabled a file bucket will morph into > a mmap bucket and the remaining file bucket. I think the heap bucket will > only be created if mmap is turned off. But I agree that this possibly > introduces > too much complexity to the example and distracts the reader from the > important > point. Yeah. It's an implementation detail, and the risk is that if it gets documented, people will rely on it somehow. > - Filtering brigades: > - Although this is not my opinion I know that there had been some > discussions in the > past that no examples should be given on how you should *not* do things. I was wary of doing this too, but it's such a common mistake that I thought it to explain explicitly why it's bad. > - Maintaining state: > - f->ctx = state = apr_palloc(sizeof(*state), f->r->pool); instead of > f->ctx = state = apr_palloc(sizeof *state, f->r->pool); ? I vaguely prefer sizeof without the brackets where valid ;) > - IMHO ap_save_brigade can operate on an existing brigade. So this can > be a brigade created once per invocation. I agree with the warning that > especially on PIPE buckets ap_save_brigade can consume quite a lot of > memory. Plus ap_save_brigade does a blocking read on the bucket. Good points, I'll add some text. joe
