[SSSD] [PATCH] libsss_dbus

Jakub Hrozek jhrozek at redhat.com
Wed May 28 15:09:20 UTC 2014


On Tue, May 27, 2014 at 10:49:28PM +0200, Pavel Březina wrote:
> On 05/27/2014 03:44 PM, Jakub Hrozek wrote:
> >On Sun, May 25, 2014 at 09:52:26PM +0200, Pavel Březina wrote:
> >>On 05/23/2014 09:13 PM, Sumit Bose wrote:
> >>>On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
> >>>>On 05/23/2014 07:06 PM, Sumit Bose wrote:
> >>>>>On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
> >>>>>>On 05/22/2014 11:17 AM, 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:
> >>>>>
> >>>>>
> >>>>>Maybe we can split the header file like libcurl does so that the caller can
> >>>>>decided if he prefers sss_sifp_easy.h where dbus includes file are not required
> >>>>>or the more flexible sss_sifp.h ?
> >>>>
> >>>>Ok, we can do that. How about moving the functions that requires dbus
> >>>>headers into sss_sifp_dbus.h?
> >>>>
> >>>
> >>>I'm fine with the name. Do you have time to do this, or shall I prepare
> >>>a patch on top of yours?
> >>>
> >>>bye,
> >>>Sumit
> >>
> >>Hi,
> >>new patches are attached.
> >>
> >
> >I went through the patchset again because it changed a bit from the
> >initial review and in general I think it's too much of code to be
> >reviewed by a single person.
> >
> >I only have some nitpicks, apart from what Sumit said:
> >sss_sifp_send_message_ex - after
> >dbus_connection_send_with_reply_and_block is called, should we also
> >check reply for being non-NULL? Also, if dbus_error is set but also
> >reply is set, we should unref reply.
> 
> No and no. Reply is null if and only if an error is set. At least
> that's what is supposed to be I haven't check the dbus code.
> 
> http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga8d6431f17a9e53c9446d87c2ba8409f0

I found DBus documentation to be inaccurate in the past as well. For
instance, some of the functions that work with typed varargs were
documented to only return false on OOM, but they also return false on
type mismatch. So I learned to be paranoid.

> 
> >
> >In sss_sifp_alloc_zero() you have two spaces:
> >     memset(addr, '\0', size  * num);
> >                            ^^
> >
> >But more importantly, as valgrind reveals, there are some memory leaks,
> >at least in the tests. I don't want to block including the library for
> >the beta because of these leaks, but please file a ticket to clean them
> >up post-beta.
> 
> The only leak I see is:
> 
> ==24809== 664 (184 direct, 480 indirect) bytes in 1 blocks are
> definitely lost in loss record 16 of 16
> ==24809==    at 0x4A0881C: malloc (vg_replace_malloc.c:270)
> ==24809==    by 0x3386A17BB5: ??? (in /usr/lib64/libdbus-1.so.3.5.6)
> ==24809==    by 0x3386A189D2: dbus_message_new_method_call (in
> /usr/lib64/libdbus-1.so.3.5.6)
> ==24809==    by 0x40B7D8: sss_sifp_create_message (sss_sifp_dbus.c:63)
> ==24809==    by 0x40BD75: sss_sifp_create_prop_msg (sss_sifp.c:44)
> ==24809==    by 0x40C0B6: sss_sifp_fetch_attr (sss_sifp.c:160)
> ==24809==    by 0x406065: test_sss_sifp_fetch_attr (test_sss_sifp.c:1336)
> ==24809==    by 0x4C30767: _run_test (cmocka.c:1723)
> ==24809==    by 0x4C30BF8: _run_tests (cmocka.c:1839)
> ==24809==    by 0x40837C: main (test_sss_sifp.c:1928)
> 
> Unfortunately I don't know what's wrong here. It refers to a memory
> leak from a dbus message, however the message is (unless I am blind)
> unrefed. So the leak is not supposed to be there.

I couldn't find the reason either but valgrind is rarely wrong. So
please file a ticket for 1.12.0 stabilisation.

> 
> New patches are attached.

They are looking good to me and I can confirm that Sumit's suggestions
were fixed. ACK from me, it would be nice to see if Sumit is happy with
the latest iteration as well.

I'll run the patches through Coverity in the meantime.



More information about the sssd-devel mailing list