On Thu, 2014-05-22 at 15:22 +0200, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:02:45AM -0400, Simo Sorce wrote:
On Thu, 2014-05-22 at 11:17 +0200, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote: >On 05/16/2014 02:04 PM, Jakub Hrozek wrote: >>On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: >>>On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
I am concerned with all the abstraction around the allocation functions.
I see that by default malloc() is used, and with the use of malloc I can already see a ton of memory leaks throughout the code, look for example at sss_sifp_create_ifp_msg() in your patch, I think it will leak a struct sss_sifp_dbus_message at every invocation.
as said, the patch was meant to illustrate the idea, it certainly needs fixing.
Why this complexity and pluggability of allocators ?
I think it dates back when you asked if dhash can have a pluggable allocator so that talloc can be used directly. After that most the SSSD libraries for external use had this.
Do you think this should change?
It made sense for dhash as it was a very small, contained library with a simple interface, and it made a lot of sense to be able to use it outside of sssd.
This is a sssd specific interface, so it may be ok to just use talloc.
The only 'potential' reason would be if we need to allow for thread safety on the client side (unfortunately talloc is not completely thread safe for the 'NULL' context), but we can always put in work to fix talloc :)
It is always a trade-off in the end, I just wanted to point out the code I've sen has issues though, and that concerned me.
Simo.