httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: [RFC] Guide to writing output filters
Date Sat, 17 Mar 2007 15:30:24 GMT

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/%3c44C3DFDA.9080306@apache.org%3e

- 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



Mime
View raw message