Fix truncation of bytes/bytes_read/bytes_written stats #156
Conversation
These stats are stored in a uint64_t but we were truncating them to 32 bits.
| @@ -87,15 +87,15 @@ static memcached_return set_data(memcached_stat_st *memc_stat, char *key, char * | |||
| } | |||
| else if (!strcmp("bytes_read", key)) | |||
| { | |||
| memc_stat->bytes_read= (uint32_t) strtoll(value, (char **)NULL, 10); | |||
| memc_stat->bytes_read= (uint64_t) strtoll(value, (char **)NULL, 10); | |||
There was a problem hiding this comment.
Should we be using strtoull so we aren't casting away the sign?
It seems like there might be an edge case if you had a value greater than LLONG_MAX but less than ULLONG_MAX. The manual says the value would be truncated to LLONG_MAX, giving us 63 bits instead of 64.
There was a problem hiding this comment.
Newer versions of libmemcached do use strtoull, so I suspect you're correct. However what I changed the code to here matches the other 64-bit stats (like cmd_get a few lines below the end of this diff). I wanted to make as small a change as possible to fix the immediate issue, which is truncating to 32 bits. It seems unlikely that we would end up with a value greater than 2^63 (8 exabytes) here.
There was a problem hiding this comment.
That said, if a more complete fix is desired I'd be happy to switch to strtoull everywhere.
There was a problem hiding this comment.
Yeah, after I saw the diff I looked at the whole file and saw strtoll being used everywhere for unsigned left hand values and realized why you went this route.
|
hey @aroben , thanks for the PR. |
Yeah, as mentioned above, newer versions of the C client have already fixed this bug. |
|
@aroben , I am looking into update the client here, as it is SUPER old.. but before I have all that done, I need to get CI back to green, and after I have that I will merge this for us.. |
|
The embedded libmemcached has been upgraded to 1.0.18 on master now, which is the newest release, so I think this can be closed now. |
These stats are stored in a
uint64_tbut we were truncating them to 32 bits.