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