[SSSD] [PATCH] libsss_dbus

Sumit Bose sbose at redhat.com
Wed May 28 16:29:42 UTC 2014


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?

bye,
Sumit

> 
> I'll run the patches through Coverity in the meantime.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list