You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Paul J. Reder" <re...@remulak.net> on 2002/10/13 05:26:08 UTC
Re: cvs commit: httpd-2.0/modules/experimental cache_util.c
Okay, this takes care of item 4 from the list below. Thanks Brian, saves
me from having to do the commit. :)
What about the other 3? Should they be fixed by the change from
apr_time_t to apr_int64_t? Apr_time_t is really apr_int64_t under
the covers and I was seeing only the lower 32 bits being set when
the variables were assigned "0" and "-1". The value was correctly
set when it was assigned APR_DATE_BAD (which has an embedded cast)
so it seems that 1-3 still need to be done.
I haven't had a chance to re-run the tests through the debugger,
but I don't think the change from apr_time_t (apr_int64_t under
the covers) to apr_int64_t is going to remove the need for 1-3.
On Fri, 2002-10-11 at 16:04, Paul J. Reder wrote:
> I have run into a problem where the cache code randomly decides that a
> cached entry is stale or that the last modified date is some time in
> the future. I tracked it back to the ap_cache_check_freshness code
> which does a lot of checking of dates.
>
> Some of this date checking code compares and assigns uncast numeric values
> (such as -1) and the output of atoi() to the apr_time_t values (which are
> long longs). The debugger showed me that only the bottom half of the
> apr_time_t was being updated/compared.
>
> I would like to fix the code in the following ways (if there are no
> objections):
> 1) Replace the assignments/comparisons of 0 with APR_DATE_BAD
> 2) Have someone create a "#define APR_DATE_UNASSIGNED ((apr_time_t)-1)"
> value in apr_date.h.
> 3) Replace the assignments/comparisons of -1 with APR_DATE_UNASSIGNED
> 4) Replace the atoi() calls with calls to apr_atoi64().
I can fix 1 and 3 (as shown in the patch I submitted for review) but I
need help from someone with APR commit authority to do number 2 (before
I can do 3).
Thanks,
Paul J. Reder
brianp@apache.org wrote:
> brianp 2002/10/11 23:43:32
>
> Modified: modules/experimental cache_util.c
> Log:
> Fix a mismatch of data types: apr_time_t vs intervals measured
> in seconds. Also use 64-bit atoi conversions to avoid loss of
> precision (thanks to Paul Reder for the atoi fix)
>
> Revision Changes Path
> 1.19 +9 -9 httpd-2.0/modules/experimental/cache_util.c
>
> Index: cache_util.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_util.c,v
> retrieving revision 1.18
> retrieving revision 1.19
> diff -u -r1.18 -r1.19
> --- cache_util.c 26 Aug 2002 16:41:56 -0000 1.18
> +++ cache_util.c 12 Oct 2002 06:43:32 -0000 1.19
> @@ -138,7 +138,7 @@
>
>
> /* do a HTTP/1.1 age calculation */
> -CACHE_DECLARE(apr_time_t) ap_cache_current_age(cache_info *info, const apr_time_t age_value)
> +CACHE_DECLARE(apr_int64_t) ap_cache_current_age(cache_info *info, const apr_time_t age_value)
> {
> apr_time_t apparent_age, corrected_received_age, response_delay, corrected_initial_age,
> resident_time, current_age;
> @@ -152,13 +152,13 @@
> resident_time = apr_time_now() - info->response_time;
> current_age = corrected_initial_age + resident_time;
>
> - return (current_age);
> + return apr_time_sec(current_age);
> }
>
> CACHE_DECLARE(int) ap_cache_check_freshness(cache_request_rec *cache,
> request_rec *r)
> {
> - apr_time_t age, maxage_req, maxage_cresp, maxage, smaxage, maxstale, minfresh;
> + apr_int64_t age, maxage_req, maxage_cresp, maxage, smaxage, maxstale, minfresh;
> const char *cc_cresp, *cc_req, *pragma_cresp;
> const char *agestr = NULL;
> char *val;
> @@ -202,7 +202,7 @@
> /* TODO: pragma_cresp not being used? */
> pragma_cresp = apr_table_get(r->headers_out, "Pragma");
> if ((agestr = apr_table_get(r->headers_out, "Age"))) {
> - age_c = atoi(agestr);
> + age_c = apr_atoi64(agestr);
> }
>
> /* calculate age of object */
> @@ -210,19 +210,19 @@
>
> /* extract s-maxage */
> if (cc_cresp && ap_cache_liststr(cc_cresp, "s-maxage", &val))
> - smaxage = atoi(val);
> + smaxage = apr_atoi64(val);
> else
> smaxage = -1;
>
> /* extract max-age from request */
> if (cc_req && ap_cache_liststr(cc_req, "max-age", &val))
> - maxage_req = atoi(val);
> + maxage_req = apr_atoi64(val);
> else
> maxage_req = -1;
>
> /* extract max-age from response */
> if (cc_cresp && ap_cache_liststr(cc_cresp, "max-age", &val))
> - maxage_cresp = atoi(val);
> + maxage_cresp = apr_atoi64(val);
> else
> maxage_cresp = -1;
>
> @@ -238,13 +238,13 @@
>
> /* extract max-stale */
> if (cc_req && ap_cache_liststr(cc_req, "max-stale", &val))
> - maxstale = atoi(val);
> + maxstale = apr_atoi64(val);
> else
> maxstale = 0;
>
> /* extract min-fresh */
> if (cc_req && ap_cache_liststr(cc_req, "min-fresh", &val))
> - minfresh = atoi(val);
> + minfresh = apr_atoi64(val);
> else
> minfresh = 0;
>
>
>
>
>
>
>
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: cvs commit: httpd-2.0/modules/experimental cache_util.c
Posted by "Paul J. Reder" <re...@remulak.net>.
I spent a little time on Monday and Tuesday looking at this in the
debugger and it looks like the behavior I am seeing is probably
related to compiler optimizations. My testing showed that the fixes
you committed (64 bit atoi and conversion to use seconds) seem to
have fixed the random behavior I was seeing.
Thanks.
Brian Pane wrote:
> On Sat, 2002-10-12 at 20:26, Paul J. Reder wrote:
>
>>Okay, this takes care of item 4 from the list below. Thanks Brian, saves
>>me from having to do the commit. :)
>>
>>What about the other 3? Should they be fixed by the change from
>>apr_time_t to apr_int64_t? Apr_time_t is really apr_int64_t under
>>the covers and I was seeing only the lower 32 bits being set when
>>the variables were assigned "0" and "-1". The value was correctly
>>set when it was assigned APR_DATE_BAD (which has an embedded cast)
>>so it seems that 1-3 still need to be done.
>>
>
> If I remember correctly, the ANSI arithmetic conversion rules
> should cause the 0 or -1 to be sign-extended to long long (or
> whatever other integral type apr_int64_t is typedef'ed to).
>
> Brian
>
>
>
>
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: cvs commit: httpd-2.0/modules/experimental cache_util.c
Posted by Brian Pane <br...@cnet.com>.
On Sat, 2002-10-12 at 20:26, Paul J. Reder wrote:
> Okay, this takes care of item 4 from the list below. Thanks Brian, saves
> me from having to do the commit. :)
>
> What about the other 3? Should they be fixed by the change from
> apr_time_t to apr_int64_t? Apr_time_t is really apr_int64_t under
> the covers and I was seeing only the lower 32 bits being set when
> the variables were assigned "0" and "-1". The value was correctly
> set when it was assigned APR_DATE_BAD (which has an embedded cast)
> so it seems that 1-3 still need to be done.
If I remember correctly, the ANSI arithmetic conversion rules
should cause the 0 or -1 to be sign-extended to long long (or
whatever other integral type apr_int64_t is typedef'ed to).
Brian