[SSSD] [PATCH] libsss_dbus

Simo Sorce simo at redhat.com
Thu May 22 13:47:38 UTC 2014


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.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list