On 09/13/2014 07:05 PM, Graham Leggett wrote: > On 12 Sep 2014, at 7:56 AM, Ruediger Pluem <rpl...@apache.org> wrote: > >>> Agreed - as rpluem suggested a pool cleanup should be registered for this, >>> I’ve added it. >> >> I know that I proposed this, but rethinking I see one risk with the cleanup. >> Take the following code >> with long_living_apr_bucket_brigade_ptr being NULL in the beginning: >> >> apr_bucket_brigade *stack_variable; >> >> stack_variable = long_living_apr_bucket_brigade_ptr; >> ap_save_brigade(f, &stack_variable, &bb, *deferred_write_pool); >> long_living_apr_bucket_brigade_ptr = stack_variable; >> >> I guess in this case this could cause unpredictable behaviour as the stack >> of the calling function is likely to be >> repurposed when the pool dies. Do we see that as something that we need to >> consider or do we see this as a "bug" on >> caller side? If we see it as a "bug" on caller side we should probably >> document our expectations. > > Hmmm… > > It does somewhat limit the caller if we force them to put the pointer > somewhere not-on-the-stack. > > Maybe we should remove the cleanup and document the expectation that the > caller must clean it up. In most cases the caller is allocating the space > from the parent pool anyway, so it all gets cleaned up regardless. >
How about to require that the caller of ap_filter_setaside_brigade just hands over a non NULL bucket_brigade as buffered_bb (so changing apr_bucket_brigade ** to apr_bucket_brigade *) and that it should handle *deferred_write_pool as opaque structure and that it should not allocate from it? Or we request the caller to provide a non NULL deferred_write_pool as well (so changing apr_pool_t ** to apr_pool_t *) and warn the caller that the pool might be cleared during the call of ap_filter_setaside_brigade. Hence solving all lifetime issues would be in the responsibility of the caller. Regards Rüdiger
