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#ga8d6...
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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel