You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2009/01/05 11:26:16 UTC
Re: svn commit: r731358 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_ext_filter.xml
modules/filters/mod_ext_filter.c
On 01/04/2009 09:52 PM, niq@apache.org wrote:
> Author: niq
> Date: Sun Jan 4 12:52:41 2009
> New Revision: 731358
>
> URL: http://svn.apache.org/viewvc?rev=731358&view=rev
> Log:
> Fix mod_ext_filter to detect failure to start the external program,
> and add configuration option to abort or continue.
> PR 41120
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/manual/mod/mod_ext_filter.xml
> httpd/httpd/trunk/modules/filters/mod_ext_filter.c
>
> @@ -855,7 +869,19 @@
>
> if (!ctx) {
> if ((rv = init_filter_instance(f)) != APR_SUCCESS) {
> - return rv;
> + ctx = f->ctx;
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
> + "can't initialise output filter %s: %s",
> + f->frec->name,
> + ctx->dc->onfail ? "removing" : "aborting");
> + ap_remove_output_filter(f);
> + if (ctx->dc->onfail) {
Hm. I am slightly confused here. The documentation states that "abort" should be
the default behaviour (that is the else branch), but if nothing is set in the config
onfail defaults to -1 which would cause to get us here (the remove case).
So either the documentation or the code is wrong.
> + return ap_pass_brigade(f->next, bb);
> + }
> + else {
> + f->r->status = HTTP_INTERNAL_SERVER_ERROR;
> + return HTTP_INTERNAL_SERVER_ERROR;
> + }
> }
> ctx = f->ctx;
> }
> @@ -886,7 +912,19 @@
>
> if (!ctx) {
> if ((rv = init_filter_instance(f)) != APR_SUCCESS) {
> - return rv;
> + ctx = f->ctx;
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r,
> + "can't initialise input filter %s: %s",
> + f->frec->name,
> + ctx->dc->onfail ? "removing" : "aborting");
> + ap_remove_input_filter(f);
> + if (ctx->dc->onfail) {
Same as above.
> + return ap_get_brigade(f->next, bb, mode, block, readbytes);
> + }
> + else {
> + f->r->status = HTTP_INTERNAL_SERVER_ERROR;
> + return HTTP_INTERNAL_SERVER_ERROR;
> + }
> }
> ctx = f->ctx;
> }
>
Regards
Rüdiger
Re: svn commit: r731358 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_ext_filter.xml
modules/filters/mod_ext_filter.c
Posted by Nick Kew <ni...@webthing.com>.
Ruediger Pluem wrote:
>
> On 01/05/2009 02:16 PM, Nick Kew wrote:
>> Ruediger Pluem wrote:
>>
>>> Hm. I am slightly confused here. The documentation states that "abort"
>>> should be
>>> the default behaviour (that is the else branch), but if nothing is set
>>> in the config
>>> onfail defaults to -1 which would cause to get us here (the remove case).
>>> So either the documentation or the code is wrong.
>> Fixed in r731388, which is included in the backport proposal.
>> Thanks for reviewing.
>
> IMHO r731388 does not fix my concern. The concern I have is that documentation
> and code are contrary on what is the default behaviour if nothing is set.
Aaargh!
OK, my local copy is changed to testing (ctx->dc->onfail == 1).
It seems that got omitted from my commit, so you're right.
Fixing it now!
(the difference arose because the local copy has an experimental
variant in with #ifdef, and I couldn't be arsed to tidy all that up).
--
Nick Kew
Re: svn commit: r731358 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_ext_filter.xml
modules/filters/mod_ext_filter.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 01/05/2009 02:16 PM, Nick Kew wrote:
> Ruediger Pluem wrote:
>
>> Hm. I am slightly confused here. The documentation states that "abort"
>> should be
>> the default behaviour (that is the else branch), but if nothing is set
>> in the config
>> onfail defaults to -1 which would cause to get us here (the remove case).
>> So either the documentation or the code is wrong.
>
> Fixed in r731388, which is included in the backport proposal.
> Thanks for reviewing.
IMHO r731388 does not fix my concern. The concern I have is that documentation
and code are contrary on what is the default behaviour if nothing is set.
Regards
Rüdiger
Re: svn commit: r731358 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_ext_filter.xml
modules/filters/mod_ext_filter.c
Posted by Nick Kew <ni...@webthing.com>.
Ruediger Pluem wrote:
> Hm. I am slightly confused here. The documentation states that "abort" should be
> the default behaviour (that is the else branch), but if nothing is set in the config
> onfail defaults to -1 which would cause to get us here (the remove case).
> So either the documentation or the code is wrong.
Fixed in r731388, which is included in the backport proposal.
Thanks for reviewing.
--
Nick Kew