On 03/16/2007 11:55 PM, Joe Orton wrote: > http://people.apache.org/~jorton/output-filters.html > > How does this look? Anything missed out, anything that doesn't make > sense? I think this covers most of the major problems in output filters > which keep coming up.
Thanks for doing this. It looks very good to me, especially as it gives us a set of rules and best practises even though I think there might be a discussion on the details. 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] - 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. I think it is ok to call it and never *reuse* it. So the remark of destroyed brigades containg buckets sounds a bit misleading to me. It can only contain buckets if it is reused after it was destroyed. But I agree that especially on passed brigades it is not always possible to find out whether this is reused and as it does not get us actual advantages apr_brigade_cleanup should be used instead in 99% of all cases. Maybe worth mentioning apr_brigade_cleanup in this context. - 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. - 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. - There is one word too much (use / consume) in "in contrast, the implementation below will use consume a fixed" - I guess it should be APR_BRIGADE_INSERT_HEAD(tmpbb, e); instead of APR_BRIGADE_INSERT_HEAD(tmpbb); - Maintaining state: - f->ctx = state = apr_palloc(sizeof(*state), f->r->pool); instead of f->ctx = state = apr_palloc(sizeof *state, f->r->pool); ? - 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. - Ten rules for output filters - 1. See my comments above - 6. It needs to be apr_brigade_cleanup instead of apr_brigade_clear. > > I'd also like to add a simple buffering filter which "does things right" > and can be used as a reference; all other in-tree filters are either too > complicated (filters/*, http/* etc) or too awful (experimental/*). Any > objections? Sounds good to me. Just did not have the time to review it so far. Regards Rüdiger
