libmemcache, various minor questions

Sean Chittenden sean at chittenden.org
Tue Dec 7 11:25:21 PST 2004


> I believe that the following lines in libmemcache.c are causing free on
> delete to always be set when using mc_aget()...
>
> (lines 482, 483)
> if (res->_flags & (MC_RES_FREE_ON_DELETE | MC_RES_NO_FREE_ON_DELETE))
>       mc_res_free_on_delete(res, (res->size > 0 ? 0 : 1));
>
>
> Looks like if either free_on_delete, or no_free_on delete was set, then
> the check based off res->size occurs, res->size seems to always be 0 at
> this point when using mc_aget, so free_on_delete gets reset to be on,
> even though it was explicitly set off just moments ago in the mc_aget
> function.

I'm using (MC_RES_FREE_ON_DELETE | MC_RES_NO_FREE_ON_DELETE) as the 
default value meaning it hasn't been set and the library should try to 
automatically detect if it should free on delete or not.  Imagine the 
following:

> struct memcache_res *res = mc_req_add(req, key3, key3_len);
> res->size = 1024;				/* Allocate our own memory a head of time (useful 
> for loops) */
> res->val = malloc(res->size);
> mc_get(mc, req);
> mc_res_free(req, res);

The 'res->size = 1024' bit tells libmemcache(3) to not worry about the 
allocation/deallocation of res->val.  One can also call

> mc_res_free_on_delete(res, 1);

so that when someone does

> mc_res_free(req, res);

it will free res and res->val.

> Am I right?  I'm not clear to me what should be done about this, 
> because
> I think the check is there to allow the memory pre-allocation that I 
> see
> in some of your examples, but on the other hand it looks like it's
> causing all users of mc_aget to recieve a pointer to already freed
> memory.

The problem that I ran into (and I pushed this out without much 
testing, I apologize), was that I needed a way of detecting successful 
fetches of keys that had zero length responses.  Originally I was 
testing for valid keys based off of res->size, but this doesn't work in 
the previous situation.  So instead I changed the previous 
res->free_on_delete to res->_flags that was a bit options field.  As a 
result, one can now fetch keys with zero length... but apparently I 
didn't do this 100% right.  I don't use mc_aget() anywhere any more in 
my apps and didn't run my little regression test (stupid, yeah, I 
know... normally I do but it was late and I wanted to leave the colo).

> Perhaps rather than trying to autodetect this users who are
> preallocating memory should simply have to call mc_res_free_on_delete
> themselves, and then the value of the flags should never get modified 
> by
> this apparent auto-detection routine?

Well, that's just it.  mc_res_free_on_delete(3) sets the value to 
either one bit or the other, not both.  If both bits are set, however, 
then it means that the library needs to look at res->size to see if it 
should free on delete or not.  The logic is simple, but the coding 
wasn't 100% correct.  If you apply the following:

Index: memcache.c
===================================================================
RCS file: /opt/cvsroot/nexadesic/src/lib/libmemcache/memcache.c,v
retrieving revision 1.1
diff -u -r1.1 memcache.c
--- memcache.c  7 Dec 2004 01:17:11 -0000       1.1
+++ memcache.c  7 Dec 2004 19:08:20 -0000
@@ -493,7 +493,8 @@

      /* While we're looping, might as well see if we should be auto
       * deleting any of these keys. */
-    if (res->_flags & (MC_RES_FREE_ON_DELETE | 
MC_RES_NO_FREE_ON_DELETE))
+    if (res->_flags & (MC_RES_FREE_ON_DELETE | 
MC_RES_NO_FREE_ON_DELETE) ==
+       (MC_RES_FREE_ON_DELETE | MC_RES_FREE_ON_DELETE))
        mc_res_free_on_delete(res, (res->size > 0 ? 0 : 1));
    }
    rv[i].iov_base = str_endl;
@@ -686,7 +687,8 @@
        /* Make sure we are where we think we are. */
        if (mc->start[rb - 2] != '\r' && mc->start[rb - 1] != '\n') {
         warnx("%s:%u\tProtocol Error", __FILE__, __LINE__);
-       if (!(res->_flags & (MC_RES_FREE_ON_DELETE | 
MC_RES_NO_FREE_ON_DELETE)) ||
+       if ((res->_flags & (MC_RES_FREE_ON_DELETE | 
MC_RES_NO_FREE_ON_DELETE) ==
+            (MC_RES_FREE_ON_DELETE | MC_RES_NO_FREE_ON_DELETE)) ||
             (res->_flags & MC_RES_FREE_ON_DELETE)) {
           mcFree(res->val);
           res->val = NULL;
@@ -1072,7 +1074,8 @@
  void
  mc_res_free(struct memcache_req *req, struct memcache_res *res) {
    TAILQ_REMOVE(&req->query, res, entries);
-  if (res->_flags & (MC_RES_FREE_ON_DELETE | MC_RES_NO_FREE_ON_DELETE) 
||
+  if ((res->_flags & (MC_RES_FREE_ON_DELETE | 
MC_RES_NO_FREE_ON_DELETE) ==
+       (MC_RES_FREE_ON_DELETE | MC_RES_NO_FREE_ON_DELETE)) ||
        res->_flags & MC_RES_FREE_ON_DELETE) {
      if (res->size > 0)
        mcFree(res->val);

you should be good to go.  Or, conversely, you could get the 1.0.1 
release which can be found here:

http://people.FreeBSD.org/~seanc/libmemcache/

-sc

-- 
Sean Chittenden



More information about the memcached mailing list