You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2023/06/07 11:56:59 UTC
Re: svn commit: r1910267 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/filters/mod_ext_filter.c
Hi Giovanni;
On Wed, Jun 7, 2023 at 12:02 AM <gb...@apache.org> wrote:
>
> Author: gbechis
> Date: Tue Jun 6 22:02:37 2023
> New Revision: 1910267
>
> URL: http://svn.apache.org/viewvc?rev=1910267&view=rev
> Log:
> mod_ext_filter: check exit status of filter processes
[]
>
> +/* check_filter_process_on_eos():
> + *
> + * if we hit end-of-stream, check the exit status of the filter process, and log
> + * an appropriate message if it failed
> + */
> +static apr_status_t check_filter_process_on_eos(ef_ctx_t *ctx, request_rec *r)
> +{
> + if (ctx->hit_eos) {
> + int exitcode;
> + apr_exit_why_e exitwhy;
> + apr_status_t waitret = apr_proc_wait(ctx->proc, &exitcode, &exitwhy,
> + APR_WAIT);
> + if (waitret != APR_CHILD_DONE) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, waitret, r, APLOGNO(10451)
> + "apr_proc_wait() failed, uri=%s", r->uri);
> + return waitret;
> + }
> + else if (exitwhy != APR_PROC_EXIT) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10452)
> + "child process %s killed by signal %d, uri=%s",
> + ctx->filter->command, exitcode, r->uri);
> + return HTTP_INTERNAL_SERVER_ERROR;
> + }
> + else if (exitcode != 0) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10453)
> + "child process %s exited with non-zero status %d, "
> + "uri=%s", ctx->filter->command, exitcode, r->uri);
> + return HTTP_INTERNAL_SERVER_ERROR;
> + }
HTTP_INTERNAL_SERVER_ERROR (like all HTTP_* statuses) is not an
apr_status_t, it shouldn't be returned by a filter (and does not print
well as an ap_log_rerror() error status for instance like below).
Maybe use APR_EGENERAL? The error message could be enough to
distinguish them here.
I wouldn't return waitret for the first case either since it's in the
error message already, no need to forward it specifically to the
caller, so APR_EGENERAL still possibly.
> + }
> +
> + return APR_SUCCESS;
> +}
> +
> /* ef_unified_filter:
> *
> * runs the bucket brigade bb through the filter and puts the result into
> @@ -880,6 +914,11 @@ static apr_status_t ef_output_filter(ap_
> if (rv != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01468)
> "ef_unified_filter() failed");
> + return rv;
> + }
> +
> + if ((rv = check_filter_process_on_eos(ctx, r)) != APR_SUCCESS) {
> + return rv;
Not correct here for a filter.
> }
>
> if ((rv = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
> @@ -939,7 +978,13 @@ static apr_status_t ef_input_filter(ap_f
> }
>
> rv = ef_unified_filter(f, bb);
> - return rv;
> + if (rv != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, APLOGNO(10454)
> + "ef_unified_filter() failed");
> + return rv;
Ditto, for both the error log status and return value.
> + }
> +
> + return check_filter_process_on_eos(ctx, f->r);
> }
Regards;
Yann.
Re: svn commit: r1910267 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/filters/mod_ext_filter.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Jun 8, 2023 at 9:03 AM Giovanni Bechis <gi...@paclan.it> wrote:
>
> This should fix both issues.
+1, thanks!
Regards;
Yann.
Re: svn commit: r1910267 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/filters/mod_ext_filter.c
Posted by Giovanni Bechis <gi...@paclan.it>.
On Wed, Jun 07, 2023 at 06:19:13PM +0200, Yann Ylavic wrote:
> On Wed, Jun 7, 2023 at 4:36 PM Ruediger Pluem <rp...@apache.org> wrote:
> >
> > On 6/7/23 1:56 PM, Yann Ylavic wrote:
> > > Hi Giovanni;
> > >
> > > On Wed, Jun 7, 2023 at 12:02 AM <gb...@apache.org> wrote:
> > >>
> > >> Author: gbechis
> > >> Date: Tue Jun 6 22:02:37 2023
> > >> New Revision: 1910267
> > >>
> > >> URL: http://svn.apache.org/viewvc?rev=1910267&view=rev
> > >> Log:
> > >> mod_ext_filter: check exit status of filter processes
[...]
> > >> + else if (exitcode != 0) {
> > >> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10453)
> > >> + "child process %s exited with non-zero status %d, "
> > >> + "uri=%s", ctx->filter->command, exitcode, r->uri);
> > >> + return HTTP_INTERNAL_SERVER_ERROR;
> > >> + }
> > >
> > > HTTP_INTERNAL_SERVER_ERROR (like all HTTP_* statuses) is not an
> > > apr_status_t, it shouldn't be returned by a filter (and does not print
> > > well as an ap_log_rerror() error status for instance like below).
> > >
> > > Maybe use APR_EGENERAL? The error message could be enough to
> > > distinguish them here.
> > > I wouldn't return waitret for the first case either since it's in the
> > > error message already, no need to forward it specifically to the
> > > caller, so APR_EGENERAL still possibly.
> > >
> > >> + }
> > >> +
> > >> + return APR_SUCCESS;
> > >> +}
> > >> +
> > >> /* ef_unified_filter:
> > >> *
> > >> * runs the bucket brigade bb through the filter and puts the result into
> > >> @@ -880,6 +914,11 @@ static apr_status_t ef_output_filter(ap_
> > >> if (rv != APR_SUCCESS) {
> > >> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01468)
> > >> "ef_unified_filter() failed");
> > >> + return rv;
> > >> + }
> > >> +
> > >> + if ((rv = check_filter_process_on_eos(ctx, r)) != APR_SUCCESS) {
> > >> + return rv;
> > >
> > > Not correct here for a filter.
> >
> > I am a little bit confused. Provided that your comments on the check_filter_process_on_eos are considered and the code is changed
> > accordingly, why would it be incorrect for the filter to return this?
>
> Sorry for not being clear. I meant *if the code is not changed* that's
> where a/some/this filter returns an HTTP_ error code instead of an
> apr_status_t, which may confuse any upper filter or logger (not the
> end of the world though, it should hardly be considered something
> recoverable).
>
> >
> > >
> > >> }
> > >>
> > >> if ((rv = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
> > >> @@ -939,7 +978,13 @@ static apr_status_t ef_input_filter(ap_f
> > >> }
> > >>
> > >> rv = ef_unified_filter(f, bb);
> > >> - return rv;
> > >> + if (rv != APR_SUCCESS) {
> > >> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, APLOGNO(10454)
> > >> + "ef_unified_filter() failed");
> > >> + return rv;
> > >
> > > Ditto, for both the error log status and return value.
> >
> > Same confusion as above: While ef_unified_filter formally returns an int it looks like its content is actually an apr_status_t.
> > Hence why shouldn't it be used in the log message or returned?
>
> Oh I misread ef_unified_filter() as ef_output_filter() and thought the
> error was propagating here too.
> That's not the case, though now that I look at it in the code (rather
> than the diff only..) it probably makes sense for ef_unified_filter()
> to declare an apr_status_t as returned type (which FWICT it actually
> always returns, as you said).
>
This should fix both issues.
Giovanni
Index: modules/filters/mod_ext_filter.c
===================================================================
--- modules/filters/mod_ext_filter.c (revision 1910292)
+++ modules/filters/mod_ext_filter.c (working copy)
@@ -747,13 +747,13 @@
ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10452)
"child process %s killed by signal %d, uri=%s",
ctx->filter->command, exitcode, r->uri);
- return HTTP_INTERNAL_SERVER_ERROR;
+ return APR_EGENERAL;
}
else if (exitcode != 0) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10453)
"child process %s exited with non-zero status %d, "
"uri=%s", ctx->filter->command, exitcode, r->uri);
- return HTTP_INTERNAL_SERVER_ERROR;
+ return APR_EGENERAL;
}
}
@@ -766,7 +766,7 @@
* bb, dropping the previous content of bb (the input)
*/
-static int ef_unified_filter(ap_filter_t *f, apr_bucket_brigade *bb)
+static apr_status_t ef_unified_filter(ap_filter_t *f, apr_bucket_brigade *bb)
{
request_rec *r = f->r;
conn_rec *c = r->connection;
Re: svn commit: r1910267 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/filters/mod_ext_filter.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jun 7, 2023 at 4:36 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 6/7/23 1:56 PM, Yann Ylavic wrote:
> > Hi Giovanni;
> >
> > On Wed, Jun 7, 2023 at 12:02 AM <gb...@apache.org> wrote:
> >>
> >> Author: gbechis
> >> Date: Tue Jun 6 22:02:37 2023
> >> New Revision: 1910267
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1910267&view=rev
> >> Log:
> >> mod_ext_filter: check exit status of filter processes
> > []
> >>
> >> +/* check_filter_process_on_eos():
> >> + *
> >> + * if we hit end-of-stream, check the exit status of the filter process, and log
> >> + * an appropriate message if it failed
> >> + */
> >> +static apr_status_t check_filter_process_on_eos(ef_ctx_t *ctx, request_rec *r)
> >> +{
> >> + if (ctx->hit_eos) {
> >> + int exitcode;
> >> + apr_exit_why_e exitwhy;
> >> + apr_status_t waitret = apr_proc_wait(ctx->proc, &exitcode, &exitwhy,
> >> + APR_WAIT);
> >> + if (waitret != APR_CHILD_DONE) {
> >> + ap_log_rerror(APLOG_MARK, APLOG_ERR, waitret, r, APLOGNO(10451)
> >> + "apr_proc_wait() failed, uri=%s", r->uri);
> >> + return waitret;
> >> + }
> >> + else if (exitwhy != APR_PROC_EXIT) {
> >> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10452)
> >> + "child process %s killed by signal %d, uri=%s",
> >> + ctx->filter->command, exitcode, r->uri);
> >> + return HTTP_INTERNAL_SERVER_ERROR;
> >> + }
> >> + else if (exitcode != 0) {
> >> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10453)
> >> + "child process %s exited with non-zero status %d, "
> >> + "uri=%s", ctx->filter->command, exitcode, r->uri);
> >> + return HTTP_INTERNAL_SERVER_ERROR;
> >> + }
> >
> > HTTP_INTERNAL_SERVER_ERROR (like all HTTP_* statuses) is not an
> > apr_status_t, it shouldn't be returned by a filter (and does not print
> > well as an ap_log_rerror() error status for instance like below).
> >
> > Maybe use APR_EGENERAL? The error message could be enough to
> > distinguish them here.
> > I wouldn't return waitret for the first case either since it's in the
> > error message already, no need to forward it specifically to the
> > caller, so APR_EGENERAL still possibly.
> >
> >> + }
> >> +
> >> + return APR_SUCCESS;
> >> +}
> >> +
> >> /* ef_unified_filter:
> >> *
> >> * runs the bucket brigade bb through the filter and puts the result into
> >> @@ -880,6 +914,11 @@ static apr_status_t ef_output_filter(ap_
> >> if (rv != APR_SUCCESS) {
> >> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01468)
> >> "ef_unified_filter() failed");
> >> + return rv;
> >> + }
> >> +
> >> + if ((rv = check_filter_process_on_eos(ctx, r)) != APR_SUCCESS) {
> >> + return rv;
> >
> > Not correct here for a filter.
>
> I am a little bit confused. Provided that your comments on the check_filter_process_on_eos are considered and the code is changed
> accordingly, why would it be incorrect for the filter to return this?
Sorry for not being clear. I meant *if the code is not changed* that's
where a/some/this filter returns an HTTP_ error code instead of an
apr_status_t, which may confuse any upper filter or logger (not the
end of the world though, it should hardly be considered something
recoverable).
>
> >
> >> }
> >>
> >> if ((rv = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
> >> @@ -939,7 +978,13 @@ static apr_status_t ef_input_filter(ap_f
> >> }
> >>
> >> rv = ef_unified_filter(f, bb);
> >> - return rv;
> >> + if (rv != APR_SUCCESS) {
> >> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, APLOGNO(10454)
> >> + "ef_unified_filter() failed");
> >> + return rv;
> >
> > Ditto, for both the error log status and return value.
>
> Same confusion as above: While ef_unified_filter formally returns an int it looks like its content is actually an apr_status_t.
> Hence why shouldn't it be used in the log message or returned?
Oh I misread ef_unified_filter() as ef_output_filter() and thought the
error was propagating here too.
That's not the case, though now that I look at it in the code (rather
than the diff only..) it probably makes sense for ef_unified_filter()
to declare an apr_status_t as returned type (which FWICT it actually
always returns, as you said).
>
> >
> >> + }
> >> +
> >> + return check_filter_process_on_eos(ctx, f->r);
> >> }
> >
Regards;
Yann.
Re: svn commit: r1910267 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/filters/mod_ext_filter.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/7/23 1:56 PM, Yann Ylavic wrote:
> Hi Giovanni;
>
> On Wed, Jun 7, 2023 at 12:02 AM <gb...@apache.org> wrote:
>>
>> Author: gbechis
>> Date: Tue Jun 6 22:02:37 2023
>> New Revision: 1910267
>>
>> URL: http://svn.apache.org/viewvc?rev=1910267&view=rev
>> Log:
>> mod_ext_filter: check exit status of filter processes
> []
>>
>> +/* check_filter_process_on_eos():
>> + *
>> + * if we hit end-of-stream, check the exit status of the filter process, and log
>> + * an appropriate message if it failed
>> + */
>> +static apr_status_t check_filter_process_on_eos(ef_ctx_t *ctx, request_rec *r)
>> +{
>> + if (ctx->hit_eos) {
>> + int exitcode;
>> + apr_exit_why_e exitwhy;
>> + apr_status_t waitret = apr_proc_wait(ctx->proc, &exitcode, &exitwhy,
>> + APR_WAIT);
>> + if (waitret != APR_CHILD_DONE) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, waitret, r, APLOGNO(10451)
>> + "apr_proc_wait() failed, uri=%s", r->uri);
>> + return waitret;
>> + }
>> + else if (exitwhy != APR_PROC_EXIT) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10452)
>> + "child process %s killed by signal %d, uri=%s",
>> + ctx->filter->command, exitcode, r->uri);
>> + return HTTP_INTERNAL_SERVER_ERROR;
>> + }
>> + else if (exitcode != 0) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_SUCCESS, r, APLOGNO(10453)
>> + "child process %s exited with non-zero status %d, "
>> + "uri=%s", ctx->filter->command, exitcode, r->uri);
>> + return HTTP_INTERNAL_SERVER_ERROR;
>> + }
>
> HTTP_INTERNAL_SERVER_ERROR (like all HTTP_* statuses) is not an
> apr_status_t, it shouldn't be returned by a filter (and does not print
> well as an ap_log_rerror() error status for instance like below).
>
> Maybe use APR_EGENERAL? The error message could be enough to
> distinguish them here.
> I wouldn't return waitret for the first case either since it's in the
> error message already, no need to forward it specifically to the
> caller, so APR_EGENERAL still possibly.
>
>> + }
>> +
>> + return APR_SUCCESS;
>> +}
>> +
>> /* ef_unified_filter:
>> *
>> * runs the bucket brigade bb through the filter and puts the result into
>> @@ -880,6 +914,11 @@ static apr_status_t ef_output_filter(ap_
>> if (rv != APR_SUCCESS) {
>> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01468)
>> "ef_unified_filter() failed");
>> + return rv;
>> + }
>> +
>> + if ((rv = check_filter_process_on_eos(ctx, r)) != APR_SUCCESS) {
>> + return rv;
>
> Not correct here for a filter.
I am a little bit confused. Provided that your comments on the check_filter_process_on_eos are considered and the code is changed
accordingly, why would it be incorrect for the filter to return this?
>
>> }
>>
>> if ((rv = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
>> @@ -939,7 +978,13 @@ static apr_status_t ef_input_filter(ap_f
>> }
>>
>> rv = ef_unified_filter(f, bb);
>> - return rv;
>> + if (rv != APR_SUCCESS) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, APLOGNO(10454)
>> + "ef_unified_filter() failed");
>> + return rv;
>
> Ditto, for both the error log status and return value.
Same confusion as above: While ef_unified_filter formally returns an int it looks like its content is actually an apr_status_t.
Hence why shouldn't it be used in the log message or returned?
>
>> + }
>> +
>> + return check_filter_process_on_eos(ctx, f->r);
>> }
>
Regards
Rüdiger