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