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
> > []
> >>
> >> +/* 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.
