On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
On 05/13/2014 05:43 PM, Pavel Březina wrote:
On 05/13/2014 11:18 AM, Pavel Březina wrote:
On 05/01/2014 05:39 PM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2254
Lukáš already did first round of review for build and packaging stuff. Thank you, I hope I have fixed all your concerns. There might be some more since I moved the library into libsss_dbus and libsss_dbus-devel packages.
The library is built if IFP responder is built.
Hi, I renamed the library to libsss_simpleifp.so and changed the thing that Stef wanted.
I forgot to commit last change, new patch set is attached.
Sending new patchset that fixes few packaging issues found by Jan Šafránek from OpenLMI.
And once again I was too fast. I forgot to commit an issue that documentation html since it was generated from missing file (the path pointed to the old location in sss_client).
Now should be everything all right.
* sss_dbus: introduce API Did you consider using a different base for the errors, rather than 0 or do you think it's not needed because the error is not an int but an enum?
I think there needs to be a really verbose comment about the purpose of the library. We don't want the library to be used outside the simplest cases. As Stef already said earlier, the DBus interface is /the/ interface.
sss_dbus_object -- would it help for the 'interfaces' to be an array? After all, an object should support multiple interfaces.
Why is sss_dbus_bool typedef-ed as uint32_t and not the standard C bool? You're treating the types differently later on anyway.
I don't think the doxygen docs on sss_dbus_send_message should say what exactly the function does, just that it's a shorthand.
* sss_dbus: implement API The hunks that change src/lib/dbus/sss_dbus.exports and src/lib/dbus/sss_dbus.h belong to patch #1.
sss_dbus_send_message_ex - if you're checking all input parameters of public functions, you should also check _reply.
Why does sss_dbus_fetch_attr() return "***_attrs" ? Isn't the return value supposed to be a single attribute, not an array? There is a similar issue with sss_dbus_parse_attr().
You should always wrap multi-line macros with "do-while(0);" See: http://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html I know it's not sctrictly needed with the way you use the macros, but it's still best practice.
I don't like the macros in sss_dbus_parser.c, a macro that changes the flow of a program makes it quite hard to debug. Any chance the macro could just set an error code instead?
sss_dbus_parse_basic(): Instead of using the default handler, you can set ret to INVALID on initialization and then not use the default handler at all. The added bonus is that if you extend the type enum but forget to add the new type to the switch-case, you'll get a warning.
Why is there both sss_dbus_parse_single_attr() and sss_dbus_parse_attr()?
There is an extra semicolon after "case DBUS_TYPE_OBJECT_PATH:"
* build patch: in sssd.spec.in, you don't need to add BuildRequires: dbus-devel, we already require dbus-devel for core SSSD. I also prefer to have BuildRequires all at the top, because they apply to the SRPM, not the binary RPMs.
libsss_dbus-devel must require fully-versioned libsss_dbus.
src/lib/dbus/sss_dbus.pc.in: I think you can drop prefix and exec_prefix, right? Only includedir and libdir are used.
* unit test patch: Please squash the hunk that modifies src/lib/dbus/sss_dbus.c into an earlier patch.
The tests themselves look good to me, I'm glad we've finally acquired the habit of unit-testing the code if possible.
* sss_dbus: add support for string dictionary Again, the BuildRequires is already required by core sssd. You should also drop the explicit dhash Requires, RPM will just pick that up.
In the "done" handler of sss_dbus_parse_array(), can you either split the for so that each part is on its own line or all on one? This way seems unreadable to me. I also prefer: for (int i; ;;) { /* stuff */ } over: int i; for (i; ;;) { /* stuff */ } In the context of this done: handler, the scope of 'i' would be the same.
After applying this patch, valgrind shows a leak in the unit test: ==26658== 4 bytes in 1 blocks are definitely lost in loss record 1 of 8 ==26658== at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==26658== by 0x40B148: default_alloc (sss_dbus.c:32) ==26658== by 0x40AC29: sss_dbus_alloc_zero (sss_dbus_utils.c:29) ==26658== by 0x40AE07: sss_dbus_strdup (sss_dbus_utils.c:84) ==26658== by 0x40A79C: sss_dbus_parse_dict (sss_dbus_parser.c:151) ==26658== by 0x40A406: sss_dbus_parse_array (sss_dbus_parser.c:411) ==26658== by 0x4093D2: sss_dbus_parse_variant (sss_dbus_parser.c:472) ==26658== by 0x408D58: sss_dbus_parse_single_attr (sss_dbus_parser.c:512) ==26658== by 0x408C35: sss_dbus_parse_attr (sss_dbus_parser.c:546) ==26658== by 0x403C72: test_sss_dbus_parse_attr_string_dict (test_sss_dbus.c:661) ==26658== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==26658== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2)
* 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.
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.