httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cliff Woolley <jwool...@virginia.edu>
Subject Re: [patch] reduce conversions of apr_time_t
Date Wed, 12 Jun 2002 18:29:59 GMT
On Wed, 12 Jun 2002, William A. Rowe, Jr. wrote:

> I need a real +1 to proceed [e.g. grep for ->timeout and
> ->keep_alive_timeout and see that you agree I caught all the
> references.] This aught to be straightened up before we experiment with
> binary usec representations of apr_time_t.

+1 with the following comments:

First of all, why apr_time_t (or its eventual equivalent)?  This should be
an apr_interval_time_t (or its eventual equivalent).  apr_time_t is *only*
for time relative to the epoch.

Give or take that little issue, here are some inline comments:



> Index: include/httpd.h
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
> retrieving revision 1.184
> diff -u -r1.184 httpd.h
> --- include/httpd.h	30 May 2002 07:04:45 -0000	1.184
> +++ include/httpd.h	12 Jun 2002 16:40:29 -0000
> @@ -1078,10 +1078,10 @@
>
>      /** I haven't got a clue */

Oh THAT'S nice.  ;)  Not that it has anything to do with this patch, of
course.

>      server_addr_rec *addrs;
> -    /** Timeout, in seconds, before we give up */
> -    int timeout;
> +    /** Timeout, in apr time, before we give up */
> +    apr_time_t timeout;
>      /** Seconds we'll wait for another request */

This comment needs to change as well.

> Index: modules/generators/mod_info.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/generators/mod_info.c,v
> retrieving revision 1.47
> diff -u -r1.47 mod_info.c
> --- modules/generators/mod_info.c	7 May 2002 23:41:36 -0000	1.47
> +++ modules/generators/mod_info.c	12 Jun 2002 16:40:30 -0000
> @@ -393,9 +393,10 @@
>                          "<tt>%s:%u</tt></dt>\n",
>                          ap_get_server_name(r), ap_get_server_port(r));
>              ap_rprintf(r, "<dt><strong>Timeouts:</strong> "
> -                        "<tt>connection: %d &nbsp;&nbsp; "
> -                        "keep-alive: %d</tt></dt>",
> -                        serv->timeout, serv->keep_alive_timeout);
> +                        "<tt>connection: %.2f &nbsp;&nbsp; "
> +                        "keep-alive: %.2f</tt></dt>",
> +                        (double)serv->timeout / APR_USEC_PER_SEC,
> +                        (double)serv->keep_alive_timeout / APR_USEC_PER_SEC);
>              ap_mpm_query(AP_MPMQ_MAX_DAEMON_USED, &max_daemons);
>              ap_mpm_query(AP_MPMQ_IS_THREADED, &threaded);
>              ap_mpm_query(AP_MPMQ_IS_FORKED, &forked);

I don't think this is right.  The timeouts are still seconds, they're just
represented in apr_time_t (with 0 microseconds always).  You should keep
it as %d and use APR_TIME_SEC(serv->timeout) and so on.


> -    cmd->server->keep_alive_timeout = atoi(arg);
> +    cmd->server->keep_alive_timeout = APR_TIME_FROM_SEC(atoi(arg));

(This being the reason for that.)


> Index: modules/http/http_protocol.c
> ===================================================================
> ...
> +                       apr_psprintf(r->pool, "timeout=%d, max=%d",
> +                            (int)APR_TIME_SEC(r->server->keep_alive_timeout),
> +                            left));

Why the cast to int?  Oh, because APR_TIME_SEC() returns a 64 bit
quantity.  This is what made me realize the apr_time_t vs.
apr_interval_time_t thing. ...

You seem to have forgotten this:

Index: config.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/config.c,v
retrieving revision 1.151
diff -u -d -r1.151 config.c
--- config.c    20 May 2002 15:05:43 -0000      1.151
+++ config.c    12 Jun 2002 18:31:43 -0000
@@ -1726,8 +1726,8 @@
     s->limit_req_line = DEFAULT_LIMIT_REQUEST_LINE;
     s->limit_req_fieldsize = DEFAULT_LIMIT_REQUEST_FIELDSIZE;
     s->limit_req_fields = DEFAULT_LIMIT_REQUEST_FIELDS;
-    s->timeout = DEFAULT_TIMEOUT;
-    s->keep_alive_timeout = DEFAULT_KEEPALIVE_TIMEOUT;
+    s->timeout = APR_TIME_FROM_SEC(DEFAULT_TIMEOUT);
+    s->keep_alive_timeout = APR_TIME_FROM_SEC(DEFAULT_KEEPALIVE_TIMEOUT);
     s->keep_alive_max = DEFAULT_KEEPALIVE;
     s->keep_alive = 1;
     s->next = NULL;


Everything else seems okay... I can't find any other uses of this.

--Cliff


Mime
View raw message