[SSSD] [PATCH] libsss_dbus

Jakub Hrozek jhrozek at redhat.com
Tue May 27 13:44:55 UTC 2014


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.

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.



More information about the sssd-devel mailing list