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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
