On Wed, Jun 07, 2023 at 06:19:13PM +0200, Yann Ylavic wrote: > On Wed, Jun 7, 2023 at 4:36 PM Ruediger Pluem <rpl...@apache.org> wrote: > > > > On 6/7/23 1:56 PM, Yann Ylavic wrote: > > > Hi Giovanni; > > > > > > On Wed, Jun 7, 2023 at 12:02 AM <gbec...@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;
signature.asc
Description: PGP signature
