On 05/28/2014 05:09 PM, Jakub Hrozek wrote:
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#ga8d6431...
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.
https://fedorahosted.org/sssd/ticket/2342
I think it may be only test related, since the message is never send there. Maybe it hits some bug in dbus.
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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel