On Thu, Sep 11, 2014 at 9:51 AM, Yann Ylavic <ylavic....@gmail.com> wrote:
> On Thu, Sep 11, 2014 at 6:39 AM, Graham Leggett <minf...@sharp.fm> wrote:
>> I don’t follow - that means something different as I’m reading it. We must
>> only signal that we have less data in the output filters if we actually
>> originally had data in the output filters, ie buffered_bb existed and wasn’t
>> empty.
>
> You're right, I misread ap_core_output_filter() where I thought
> ap_filter_reinstate_brigade() wasn't called when
> APR_BRIGADE_EMPTY(bb).
>
> Since ap_filter_reinstate_brigade() can be called with an empty bb,
> ap_core_output_filter() can (still) be simplified though, ie :
>
> should_write = ap_filter_reinstate_brigade(f, ctx->buffered_bb, bb,
> &flush_upto);
> if (APR_BRIGADE_EMPTY(bb)) {
> return APR_SUCCESS;
> }
>
> if (flush_upto != NULL) {
> ...
> }
>
> if (should_write) {
> ...
> }
>
> flush_upto will always be NULL with an empty bb.
Again I missed something...
This code is indeed what I'd like ap_core_output_filter() (or any
filter) to be able to do, but it won't work without a slight change in
ap_filter_reinstate_brigade(), which is :
AP_DECLARE(int) ap_filter_reinstate_brigade(ap_filter_t *f,
apr_bucket_brigade *buffered_bb,
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);
*flush_upto = NULL;
if ((buffered_bb != NULL) && !APR_BRIGADE_EMPTY(buffered_bb)) {
int flush = APR_BRIGADE_EMPTY(bb);
APR_BRIGADE_PREPEND(bb, buffered_bb);
f->c->data_in_output_filters--;
if (flush) {
return 1;
}
}
...
}
The new thing is the "flush" check to return 1 immediatly when an
empty bb is given.
In this case the intent of the caller is to use what we have ("push it
thru" would say Jim), we don't need to walk the buffered_bb again.
That's, I think, what the original code did in ap_core_output_filter()
to send_brigade_nonblocking() immediatly in this case, and what the
current patch misses.
Thoughts?
