> -----Ursprüngliche Nachricht----- > Von: Yann Ylavic > Gesendet: Donnerstag, 12. Juni 2014 13:10 > An: httpd > Betreff: Re: svn commit: r1601877 - > /httpd/httpd/trunk/modules/filters/mod_sed.c > > 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). >
Thinking of it again, I guess cleaning in all cases is fine given the fact that we did not reuse the old brigade previously. Regards Rüdiger
