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 "
> - "keep-alive: %d</tt></dt>",
> - serv->timeout, serv->keep_alive_timeout);
> + "<tt>connection: %.2f "
> + "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
