Thanks for the review.
On Thu, Jun 12, 2014 at 9:13 AM, Ruediger Pluem <[email protected]> wrote:
>
>
> [email protected] wrote:
>> Author: ylavic
>> Date: Wed Jun 11 12:50:29 2014
>> New Revision: 1601877
>>
>> URL: http://svn.apache.org/r1601877
>> Log:
>> mod_sed: Reuse ctx->bb in sed_response_filter() and be safe with its
>> reentrance. The single return point helps to not duplicate cleanup code.
>>
>> Modified:
>> httpd/httpd/trunk/modules/filters/mod_sed.c
>>
>> Modified: httpd/httpd/trunk/modules/filters/mod_sed.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_sed.c?rev=1601877&r1=1601876&r2=1601877&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/filters/mod_sed.c (original)
>> +++ httpd/httpd/trunk/modules/filters/mod_sed.c Wed Jun 11 12:50:29 2014
>
>> @@ -351,20 +349,19 @@ static apr_status_t sed_response_filter(
>> status = sed_eval_buffer(&ctx->eval, buf, bytes, ctx);
>> }
>> if (status != APR_SUCCESS) {
>> - clear_ctxpool(ctx);
>> - return status;
>> + break;
>> }
>> }
>> apr_bucket_delete(b);
>> }
>> }
>> - status = flush_output_buffer(ctx);
>> - if (status != APR_SUCCESS) {
>> - clear_ctxpool(ctx);
>> - return status;
>> + if (status == APR_SUCCESS) {
>> + status = flush_output_buffer(ctx);
>> }
>> if (!APR_BRIGADE_EMPTY(ctx->bb)) {
>> - status = ap_pass_brigade(f->next, ctx->bb);
>> + if (status == APR_SUCCESS) {
>> + status = ap_pass_brigade(f->next, ctx->bb);
>> + }
>> apr_brigade_cleanup(ctx->bb);
>> }
>> clear_ctxpool(ctx);
>>
>>
>>
>
> We have a small change in logic here. Maybe it makes sense. Previously we did
> not cleanup ctx->bb if flush_output_buffer
> returned != APR_SUCCESS. Now we do (only if not empty of course).
Previously we created a new brigade for each call, now we reuse it :
@@ -293,9 +293,9 @@ static apr_status_t sed_response_filter(
return status;
ctx = f->ctx;
apr_table_unset(f->r->headers_out, "Content-Length");
- }
- ctx->bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
+ ctx->bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
+ }
Either we have to make sure the inner buckets have the correct
lifetime (eg. the ones simply moved from the given brigade to ctx->bb,
the transient ones from append_bucket() since the ctxpool is finally
cleared), or cleanup in any case (which is semantically equivalent to
the previous code, and what I did).
Should the sed output filter have the ability to recover from an error
(with a setaside brigade), if recalled?
It didn't before, so this looks a bit overkill, but I could do the
hard work if required (I thinks other filters are in this case
though).
The only legitimate case I can see is EAGAIN, but we have already
passed the brigade for this to happen (and need to cleanup it).
Moreover, EAGAIN is handled directly by the core output filter and
mpm_event, without the whole output filters chain being flushed (IIRC
there was a discussion on dev@ about this some time ago).
Regards,
Yann.
