Re: svn commit: r1640823 - /httpd/httpd/trunk/modules/filters/mod_reqtimeout.c

[Mike Rumph <mi...@oracle.com>]

Hello Yann,

I would have thought that a MIN macro would already be defined somewhere.
If not, perhaps a more central location (some header file) could be 
chosen for it.

Thanks,

Mike

On 11/20/2014 1:38 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Thu Nov 20 21:38:53 2014
> New Revision: 1640823
>
> URL: http://svn.apache.org/r1640823
> Log:
> mod_reqtimeout: revert r1640758.
> Unexpected functional change.
>
> Modified:
>      httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
>
> Modified: httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_reqtimeout.c?rev=1640823&r1=1640822&r2=1640823&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_reqtimeout.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_reqtimeout.c Thu Nov 20 21:38:53 2014
> @@ -164,6 +164,7 @@ static apr_status_t brigade_append(apr_b
>   }
>   
>   
> +#define MIN(x,y) ((x) < (y) ? (x) : (y))
>   static apr_status_t reqtimeout_filter(ap_filter_t *f,
>                                         apr_bucket_brigade *bb,
>                                         ap_input_mode_t mode,
> @@ -227,10 +228,8 @@ static apr_status_t reqtimeout_filter(ap
>       rv = apr_socket_timeout_get(ccfg->socket, &saved_sock_timeout);
>       AP_DEBUG_ASSERT(rv == APR_SUCCESS);
>   
> -    if (time_left < saved_sock_timeout) {
> -        rv = apr_socket_timeout_set(ccfg->socket, time_left);
> -        AP_DEBUG_ASSERT(rv == APR_SUCCESS);
> -    }
> +    rv = apr_socket_timeout_set(ccfg->socket, MIN(time_left, saved_sock_timeout));
> +    AP_DEBUG_ASSERT(rv == APR_SUCCESS);
>   
>       if (mode == AP_MODE_GETLINE) {
>           /*
> @@ -299,10 +298,9 @@ static apr_status_t reqtimeout_filter(ap
>               if (rv != APR_SUCCESS)
>                   break;
>   
> -            if (time_left < saved_sock_timeout) {
> -                rv = apr_socket_timeout_set(ccfg->socket, time_left);
> -                AP_DEBUG_ASSERT(rv == APR_SUCCESS);
> -            }
> +            rv = apr_socket_timeout_set(ccfg->socket,
> +                                   MIN(time_left, saved_sock_timeout));
> +            AP_DEBUG_ASSERT(rv == APR_SUCCESS);
>   
>           } while (1);
>   
> @@ -318,9 +316,7 @@ static apr_status_t reqtimeout_filter(ap
>           }
>       }
>   
> -    if (saved_sock_timeout != time_left) {
> -        apr_socket_timeout_set(ccfg->socket, saved_sock_timeout);
> -    }
> +    apr_socket_timeout_set(ccfg->socket, saved_sock_timeout);
>   
>   out:
>       if (APR_STATUS_IS_TIMEUP(rv)) {
>
>
>
>

Re: svn commit: r1640823 - /httpd/httpd/trunk/modules/filters/mod_reqtimeout.c

[Yann Ylavic <yl...@gmail.com>]

Hi Mike,

On Thu, Nov 20, 2014 at 10:49 PM, Mike Rumph <mi...@oracle.com> wrote:
> I would have thought that a MIN macro would already be defined somewhere.

Yes, me too, it was also a motivation for this change (remove MIN).
Unfortunately the code was wrong, and it would be more complicated
than the original one once fixed (plus the fact that
apr_socket_timeout_set() already checks that new_timeout !=
old_timeout).

> If not, perhaps a more central location (some header file) could be chosen
> for it.

It belongs to APR (along with min/max_t) IMHO.
Maybe this could be proposed there...

Regards,
Yann.