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.
> }
>
> 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.
