[SSSD] [PATCH] libsss_dbus

Sumit Bose sbose at redhat.com
Fri May 16 15:09:48 UTC 2014


On Fri, May 16, 2014 at 03:32:53PM +0200, Jakub Hrozek wrote:
> On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
> > On 05/16/2014 02:04 PM, Jakub Hrozek wrote:

....

> > >
> > >* sss_dbus: add shortcuts for common use cases
> > >I wonder if the functions specific to an object could be kept private
> > >to the LMI code. Is the CIFS module going to use these? I see the point
> > >and I see the code can be handy, but I really think we'd be asked to
> > >provide similar wrappers for groups and all other object we're going to
> > >expose on the bus.
> > >
> > >I think fetch_object_by_attr and fetch_object_by_name are fine, I'm not
> > >so sure about list_domains anymore. If you think these functions should
> > >stay, we need a good explanation in the docs on why we are supporting
> > >users and not groups.
> > 
> > Actually I don't use those functions at all so I'm fine with
> > removing them. I didn't want them to be there for this precise
> > reason.
> > 
> > Sumit asked for them.
> 
> Sumit, since Pavel doesn't use the object and user shorthands, would you
> be OK to keep them in your code? Do you agree with not 'leaking' the
> per-object functions to the public interface and keep it as simple as
> possible or do you see it would cause problems for you?

My idea was that this library hides everything D-BUS related from the
caller so that e.g. he does not has to care about D-BUS method names and
types.

I see e.g. sss_dbus_fetch_user_by_name() basically as a way to avoid to
look up the method name, "UserByName" in this case, and the D-BUS type
for the argument, DBUS_TYPE_STRING here. The later would also require to
have D-BUS header files installed.

If we drop this calls we should add references to the docs where method
names and expected parameters can be found and add enum
sss_dbus_attr_type to the public header file and translate them back
into the D-BUS types when needed.

Btw, currently using sss_dbus.h requires that D-BUS headers are
installed because of DBusMessage. I would suggest to use a opaque struct
which only contains a DBusMessage member to remove this dependency.

> 
> > 
> > >There is a new leak after applying the patch:
> > >==1047== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 17
> > >==1047==    at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> > >==1047==    by 0x5B11D65: dbus_message_new_empty_header (dbus-message.c:1140)
> > >==1047==    by 0x5B12B21: dbus_message_new_method_call (dbus-message.c:1271)
> > >==1047==    by 0x40C2EF: sss_dbus_create_message (sss_dbus.c:176)
> > >==1047==    by 0x40C5B7: sss_dbus_create_prop_msg (sss_dbus.c:49)
> > >==1047==    by 0x40C4AE: sss_dbus_fetch_attr (sss_dbus.c:248)
> > >==1047==    by 0x4063BB: test_sss_dbus_fetch_attr (test_sss_dbus.c:1335)
> > >==1047==    by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2)
> > >==1047==    by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2)
> > >==1047==    by 0x408924: main (test_sss_dbus.c:1927)
> > >==1047==
> > >
> > >* Add libsss_dbus.so to dlopen test
> > >ACK
> > >
> > >* Rename libsss_dbus to libsss_simpleifp
> > >Looks OK, but why is it a separate patch, was it too hard to roll into
> > >the previous ones? Please note I'm fine with a separate patch, I'd just
> > >like to see the reason.
> > 
> > I think it would be possible to squash it, I didn't want to do it in
> > a case we change the opinion back during the review :-)
> 
> If it's easy, then please squash it, if not, I'm fine with a separate
> patch.

I think it would be nice to rename the header file (sss_dbus.h ->
sss_sifp.h or sss_simpleifp.h) and the general prefix (sss_dbus_* ->
sss_sifp_*) as well. If we agree that this would make sense I'm willing
to extend the current rename patch accordingly. But I think in this case
it would be easier with a separate patch.

bye,
Sumit



More information about the sssd-devel mailing list