libmemcache, various minor questions

[John McCaskey]

Thanks Sean!

On Tue, 2004-12-07 at 11:25 -0800, Sean Chittenden wrote:
> > 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
> 
-- 
John A. McCaskey
Software Development Engineer
Klir Technologies, Inc.
johnm at klir.com
206.902.2027