httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: [patch] reduce conversions of apr_time_t
Date Wed, 12 Jun 2002 22:09:58 GMT
At 01:29 PM 6/12/2002, Cliff Woolley wrote:
>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.

Good suggestion, adopted.

> >      /** Seconds we'll wait for another request */
>
>This comment needs to change as well.

Done.

> > --- 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);
>
>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.

Well, that's stolen from mod_status.  Actually, timeouts in APR can be whatever
resolution the platform supports.  That said;

> > -    cmd->server->keep_alive_timeout = atoi(arg);
> > +    cmd->server->keep_alive_timeout = APR_TIME_FROM_SEC(atoi(arg));
>
>(This being the reason for that.)

Of course I recognized this, too.  We don't [in config syntax] support timeouts
with any granularity other than seconds.  That doesn't affect how they are 
used,
but it could affect how they are displayed.

I'm fine with the suggestion, went to an (int) cast in the mod_info patch :-)


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

Exactly.  It's been observed on list that seconds shouldn't be restricted to
64 bits, so the cast allows us to deliberately toss the resolution.


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


Thanks.  Revised patch to follow.


Mime
View raw message