[SSSD] [PATCH] libsss_dbus

Lukas Slebodnik lslebodn at redhat.com
Wed May 28 19:45:21 UTC 2014


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



More information about the sssd-devel mailing list