On (28/05/14 18:29), Sumit Bose wrote: >On Wed, May 28, 2014 at 05:09:20PM +0200, 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#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. > >yes, they look good, so ACK form my side as well. > >I just wonder how we can handle '-version-info 0:0:0' during the beta >phase. Do we have to change it for the release if we do changes to the >library or are we allowed to say that changes may happen during beta >without updating version-info? > Numbers are cheap :-) It will be beter to increment numbers.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
