[SSSD] [PATCH] libsss_dbus

Pavel Březina pbrezina at redhat.com
Tue May 27 20:49:28 UTC 2014


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

>
> 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.

New patches are attached.


> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tgz
Type: application/x-compressed-tar
Size: 43055 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140527/52b0d1b0/attachment.bin>


More information about the sssd-devel mailing list