[PATCHES BUG 13865] memcache tracking of talloc object sizes

[Jeremy Allison]

On Fri, Mar 29, 2019 at 12:09:53PM -0700, Christof Schmitt wrote:
> On Fri, Mar 29, 2019 at 11:35:30AM -0700, Jeremy Allison via samba-technical wrote:
> > On Thu, Mar 28, 2019 at 11:28:47AM -0700, Christof Schmitt via samba-technical wrote:
> > > On Thu, Mar 28, 2019 at 11:28:14AM -0700, Christof Schmitt wrote:
> > > > This is a fix for memcache to also add the talloc object size to the
> > > > amount of used space. The objects become part of the memcache, so the
> > > > sizes should be tracked as well.
> > > > 
> > > > The third patch is a clarification to the description of the 'max stat
> > > > cache size' parameter.
> > > > 
> > > > Pipeline is running at:
> > > > https://gitlab.com/samba-team/devel/samba/pipelines/54120627
> > > > 
> > > > Christof
> > > 
> > > And here are the actual patches.
> > 
> > Not quite complete I think. In the original memcache_add()
> > code we have a corner case where a talloc'ed key value is
> > being replaced with a non-talloc'ed value (yeah I know,
> > goes into the "don't do that catagory" but we currently
> > cope with it :-).
> > 
> > The code goes:
> > 
> > 271         e = memcache_find(cache, n, key);
> > 272 
> > 273         if (e != NULL) {
> > 274                 memcache_element_parse(e, &cache_key, &cache_value);
> > 275 
> > 276                 if (value.length <= cache_value.length) {
> > 277                         if (memcache_is_talloc(e->n)) {
> > 278                                 void *ptr;
> > 279                                 SMB_ASSERT(cache_value.length == sizeof(ptr));
> > 280                                 memcpy(&ptr, cache_value.data, sizeof(ptr));
> > 
> > I think you need to add:
> > 
> > +				    cache->size -= talloc_total_size(ptr);
> > 
> > here to keep the size consistent in this edge case.
> 
> Thanks for pointing this out.

Actually I'm wrong - we can't mix talloc/non-talloc
stores here (it crashes, I tested :-). But we still
need to take the talloc'ed size into account.

> > 
> > 281                                 TALLOC_FREE(ptr);
> > 282                         }
> > 283                         /*
> > 284                          * We can reuse the existing record
> > 285                          */
> > 286                         memcpy(cache_value.data, value.data, value.length);
> > 287                         e->valuelength = value.length;
> > 288                         return;
> > 289                 }
> > 
> > Probably need to add a test for this too...
> 
> Yes, will do. I will be busy with something else this afternoon, but i
> should have an update for this early next week.

I'm nearly done with the fix and test. If I can
get it working I'll post for your review.