On Thu, Sep 11, 2014 at 5:20 PM, Graham Leggett <minf...@sharp.fm> wrote:
> On 11 Sep 2014, at 2:12 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
>
> I would still do the flush check anyway - the ap_filter_reinstate_brigade()
> has
> no idea what happened outside this function, we may have flushed buckets,
> we may not have and setaside those flush buckets for whatever reason, and
> we want the behaviour to remain predictable.
Makes sense, however ap_core_output_filter() and
ssl_io_filter_output() *know* that their buffered_bb does not contain
such bucket (exclusively filled by ap_filter_setaside_brigade()), in
that the original code (still in svn) is more optimal since it does
not walk through the same brigade multiple times (doing an immediate
nonblocking write with what it has and returning).
To avoid that and the buffered_bb lifetime issue discussed with
Rüdiger, I'm thinking of an opaque type (a struct containing the
buffered_bb and the deferred_pool) that could be passed to both
ap_filter_setaside_brigade() and ap_filter_reinstate_brigade().
That way no user could modify the content of the brigade or play with
the pool (opaque), so that ap_filter_setaside_brigade() could
create/clear the deferred_pool as it wish, and
ap_filter_reinstate_brigade() could be sure buffered_bb contains only
suitable buckets.
Something like :
# util_filter.h
typedef struct ap_filter_aside ap_filter_aside_t;
AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f,
apr_filter_aside_t **aside,
apr_bucket_brigade *bb);
AP_DECLARE(int) ap_filter_reinstate_brigade(ap_filter_t *f,
apr_filter_aside_t *aside,
apr_bucket_brigade *bb,
apr_bucket **flush_upto);
# util_filter.c
struct ap_filter_aside
{
apr_bucket_brigade *bb;
apr_pool_t *pool;
};
AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f,
ap_filter_aside_t **aside,
apr_pool_t
**deferred_write_pool,
apr_bucket_brigade *bb)
{
if (!APR_BRIGADE_EMPTY(bb)) {
if (!*aside) {
*aside = apr_palloc(f->c->pool, sizeof **aside);
apr_pool_create(&(*aside)->pool, f->c->pool);
apr_pool_tag((*aside)->pool, "deferred_write");
(*aside)->bb = NULL;
}
if (!(*aside)->bb || APR_BRIGADE_EMPTY((*aside)->bb))) {
f->c->data_in_output_filters++;
}
if (bb != (*aside)->bb) {
return ap_save_brigade(f, &(*aside)->bb, &bb, (*aside)->pool);
}
}
else if ((*aside)->pool) {
/*
* There are no more requests in the pipeline. We can just clear the
* pool.
*/
((*aside)->bb = NULL;
apr_pool_clear((*aside)->pool);
}
return APR_SUCCESS;
}
AP_DECLARE(int) ap_filter_reinstate_brigade(ap_filter_t *f,
ap_filter_aside_t *aside,
apr_bucket_brigade *bb,
apr_bucket **flush_upto)
{
apr_bucket *bucket, *next;
apr_size_t bytes_in_brigade, non_file_bytes_in_brigade;
int eor_buckets_in_brigade, morphing_bucket_in_brigade;
int loglevel = ap_get_conn_module_loglevel(f->c, APLOG_MODULE_INDEX);
int empty = APR_BRIGADE_EMPTY(bb);
if ((*aside)->bb && !APR_BRIGADE_EMPTY((*aside)->bb)) {
APR_BRIGADE_PREPEND(bb, (*aside)->bb);
f->c->data_in_output_filters--;
if (empty) {
return 1;
}
}
...
}
# core_filters and mod_ssl
use a ap_filter_aside_t* instead of buffered_bb and deferred_write_pool.
WDYT?
