httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Plüm, Rüdiger, Vodafone Group <ruediger.pl...@vodafone.com>
Subject AW: svn commit: r1601877 - /httpd/httpd/trunk/modules/filters/mod_sed.c
Date Thu, 12 Jun 2014 11:14:05 GMT


> -----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 <rpluem@apache.org>
> wrote:
> >
> >
> > ylavic@apache.org 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
Mime
View raw message