SK-Yang / memcached

Automatically exported from code.google.com/p/memcached
0 stars 0 forks source link

better types for time #255

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
http://pubs.opengroup.org/onlinepubs/009695399/functions/times.html

and some netbsd/pkgsrc:

--- items.c.orig        2012-02-25 20:16:35.000000000 +0000
+++ items.c
@@ -379,9 +379,9 @@ char *do_item_cachedump(const unsigned i
         /* Copy the key since it may not be null-terminated in the struct */
         strncpy(key_temp, ITEM_key(it), it->nkey);
         key_temp[it->nkey] = 0x00; /* terminate */
-        len = snprintf(temp, sizeof(temp), "ITEM %s [%d b; %lu s]\r\n",
+        len = snprintf(temp, sizeof(temp), "ITEM %s [%d b; %jd s]\r\n",
                        key_temp, it->nbytes - 2,
-                       (unsigned long)it->exptime + process_started);
+                       (intmax_t)it->exptime + process_started);
         if (bufcurr + len + 6 > memlimit)  /* 6 is END\r\n\0 */
             break;
         memcpy(buffer + bufcurr, temp, len);
@@ -419,13 +419,13 @@ void do_item_stats(ADD_STAT add_stats, v
                 continue;
             }
             APPEND_NUM_FMT_STAT(fmt, i, "number", "%u", sizes[i]);
-            APPEND_NUM_FMT_STAT(fmt, i, "age", "%u", current_time - 
tails[i]->time);
+            APPEND_NUM_FMT_STAT(fmt, i, "age", "%jd", current_time - 
tails[i]->time);
             APPEND_NUM_FMT_STAT(fmt, i, "evicted",
                                 "%llu", (unsigned long long)itemstats[i].evicted);
             APPEND_NUM_FMT_STAT(fmt, i, "evicted_nonzero",
                                 "%llu", (unsigned long long)itemstats[i].evicted_nonzero);
             APPEND_NUM_FMT_STAT(fmt, i, "evicted_time",
-                                "%u", itemstats[i].evicted_time);
+                                "%jd", itemstats[i].evicted_time);
             APPEND_NUM_FMT_STAT(fmt, i, "outofmemory",
                                 "%llu", (unsigned long long)itemstats[i].outofmemory);
             APPEND_NUM_FMT_STAT(fmt, i, "tailrepairs",

--- memcached.h.orig    2011-08-10 04:53:38.000000000 +0000
+++ memcached.h
@@ -199,7 +199,7 @@ enum delta_result_type {
 };

 /** Time relative to server start. Smaller than time_t on 64-bit systems. */
-typedef unsigned int rel_time_t;
+typedef intmax_t rel_time_t;

 /** Stats stored per slab (and per thread). */
 struct slab_stats {

Original issue reported on code.google.com by msporleder@gmail.com on 25 Feb 2012 at 8:27

GoogleCodeExporter commented 9 years ago
Does this fix any theoretical or practical bug or is it just for correctness?

Original comment by dorma...@rydia.net on 26 Feb 2012 at 9:36

GoogleCodeExporter commented 9 years ago
 I believe c99 made intmax_t safe for printing/conversions no matter what the platform.  This, I believe, helps handle netbsd's usage of 64bit time_t on all platforms.

As far as a potential bug, I think it would depend on the implementation of 
time_t and be related to settings items to expire way way in the future or 
having a system with many many years of uptime on items that never expire.

Original comment by msporleder@gmail.com on 26 Feb 2012 at 1:42

GoogleCodeExporter commented 9 years ago
Wether this patch is taken or not, 

these lines:
-            APPEND_NUM_FMT_STAT(fmt, i, "age", "%u", current_time - 
tails[i]->time);
-                                "%u", itemstats[i].evicted_time);

should probably be cast

Original comment by msporleder@gmail.com on 26 Feb 2012 at 2:20

GoogleCodeExporter commented 9 years ago
I'm not sure at all what to do with this change...

You're saying that on some platforms time_t is 64bit, so process_started would 
be 64bit.

rel_time_t will always be an unsigned int, if we change that to intmax_t it's 
going to end up being the largest integer available on that platform and will 
eat into the item size. So intmax_t is more useful for casting?

This seems related to a y2038 bug... I'm going to close it though. If you can 
show some test code that fails on some specific platform I can use that to fix 
it, but the patch potentially changes memory usage and has casting that could 
fail on things I don't have access to.

Sorry :/ 2038 bug is something we'll need to fix at some point, but I don't 
think it'll be this way.

Original comment by dorma...@rydia.net on 29 Jul 2012 at 11:36