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