On Wed, Oct 30, 2013 at 05:12:56PM +0100, Jakub Hrozek wrote: > On Wed, Oct 30, 2013 at 04:43:54PM +0100, Sumit Bose wrote: > > On Wed, Oct 30, 2013 at 04:32:58PM +0100, Jakub Hrozek wrote: > > > On Wed, Oct 30, 2013 at 11:26:21AM -0400, Stephen Gallagher wrote: > > > > -----BEGIN PGP SIGNED MESSAGE----- > > > > Hash: SHA1 > > > > > > > > On 10/30/2013 10:29 AM, Pavel Březina wrote: > > > > > > > > > > > > > > > > > Nack. This happens to work by coincidence (we're initializing the > > > > idmap with a talloc allocator), but the real problem here is that we > > > > don't have a proper free() function to deal with sss_idmap_unix_to_*() > > > > functions (and friends). We allow the sss_idmap_init() to take a > > > > custom allocator, which means we need a custom free as well. The only > > > > one we offer right now is for freeing the entire idmap_ctx. > > > > > > > > Please add an appropriate sss_idmap_free_data() function and use that. > > > > > > Pavel filed https://fedorahosted.org/sssd/ticket/2133 to track that > > > effort. > > > > > > I'm not sure it makes too much sense to accept this patch for 1.11.2. > > > Since we're already relying on talloc being the allocator, we can also > > > rely on its hierarchical nature and let it do its job to free the SID.. > > > > I'm sorry but this won't work because the TALLOC context used inside the > > idmap code is a long living one because idmapping is needed as long as > > the backend lives. I took this idea for multiple allocators from the > > dhash code but here the hashes might have a shorter lifetime. > > > > bye, > > Sumit > > Ah, you're right (and so is Pavel), sorry. > > ACK
btw I only acked the patch to make sure the leak is gone for 1.11.2. For 1.11.3 and master we'll introduce a real sss_idmap_free() function. Pushed to master and sssd-1-11 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
