On 05/16/2014 03:32 PM, 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:
>>>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?
>>
>>It is enough to have zero based enum here in my opinion. All
>>functions return proper error code. But I can change it if you want.
>
>I think it's fine. It might be nice to add a strerror function in a
>later patch, though.
>
>>
>>>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.
>>
>>OK. I'll try to craft something.
>>
>>>sss_dbus_object -- would it help for the 'interfaces' to be an array?
>>>After all, an object should support multiple interfaces.
>>
>>How would we handle interfaces that does not have distinct
>>properties and method names?
>>
>>The main purpose of this struct was to have all information about an
>>object on one place with simple access to name and object path.
>>
>>However I would be fine to drop it, since I don't actually use it in
>>OpenLMI... it is used merely in the common functions.
>
>Ah, then it's fine. I think I misunderstood the field in the structure.
>
>>
>>>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 think it should be more an alias to dbus_bool_t (I didn't need to
>>include dbus.h before so I just used appropriate type for it).
>>
>>But I think I can use bool from stdbool.h there, since we switched
>>to C99 anyway.
>
>I think the standard C bool would make sense. Also, does this typedef need
>to be part of the public header? Can the bool be an opaque type?
Since I switched to std bool then the typedef became useless so I removed it
completely.
>>>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.
>>
>>Sometimes the caller may not need to be interested in reply - mainly
>>for methods that don't return any value (just dbus error).
>
>Yeah, I realized that after I wrote this comment, just forgot to remove
>the comment from the e-mail..sorry
>
>>
>>>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().
>>
>>This was first done that way, because was expanding "extra
>>attributes" instead of having them in a hash table. Therefore asking
>>for one attribute may resulted in having a set of attribute.
>>
>>I kept it this way, even though it is not strictly needed now,
>>because I wanted to kept the structure opaque and you need to ask to
>>convert it to a specific data type.
>>
>>There would have been more getter functions, that would work with
>>single attribute.
>>
>>I think having it unified is a benefit here - you can simply switch
>>from working with one attribute to all attribute.
>
>OK.
>
>>
>>>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?
>>
>>Given how the macro is used I think this is an acceptable exception.
>>
>>But if you strongly disagree, I will think about it.
>
>Please add the do-while.
>
>>
>>>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.
Those are DBUS types that are #defined, so I'd stick with current code here
instead of having ret initialized.
>>>Why is there both sss_dbus_parse_single_attr() and
>>>sss_dbus_parse_attr()?
The parser code is the same for single attribute and list of attributes (from
dict_entry). I haven't came up with a better name for it, since it is named
as what it does.
>>>There is an extra semicolon after "case DBUS_TYPE_OBJECT_PATH:"
It is required by the language grammar, since a label cannot be followed by
declaration. I could move de declaration to the beginning of the function but
I wanted to keep it there for clarity.
>>>* 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.
>>
>>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?
>
>>
>>>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==
Hmm, the dbus message is correctly unrefed in sss_dbus_fetch_attr(), this
leak should not occur. Is it possible that it is due to the fact that the
message is actually not sent over the dbus?
>>>
>>>* 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.
Squash.
All concerns should be fixed (but the memory leak in test). I also changed
the prefix to sss_sifp as Sumit asked (sending as separate patch).
I have a few comments about exported functions in sss_simpleifp.exports
1. The function sss_sifp_send_message_exis in public header file sss_sifp.h,
but it *is not* exported in sss_simpleifp.exports
sh-4.2$ git grep sss_sifp_send_message_ex
src/lib/sifp/sss_sifp.c:sss_sifp_send_message_ex(sss_sifp_ctx *ctx,
src/lib/sifp/sss_sifp.h: * sss_sifp_send_message_ex(ctx, msg, 5000, _reply);
src/lib/sifp/sss_sifp.h:sss_sifp_send_message_ex(sss_sifp_ctx *ctx,
sh-4.2$ objdump -T .libs/libsss_simpleifp.so | grep SSS_SIMPLEIFP | grep
sss_sifp_send_message
0000000000001e90 g DF .text 0000000000000010 SSS_SIMPLEIFP_0.0 sss_sifp_send_message
2. The function sss_sifp_get_iface_for_object is in private header file,
but it *is* exported in sss_simpleifp.exports
sh-4.2$ objdump -T .libs/libsss_simpleifp.so | grep SSS_SIMPLEIFP | grep
sss_sifp_get_iface_for_object
0000000000004760 g DF .text 000000000000006b SSS_SIMPLEIFP_0.0
sss_sifp_get_iface_for_object
sh-4.2$ git grep sss_sifp_get_iface_for_object
src/lib/sifp/sss_sifp_common.c: interface =
sss_sifp_get_iface_for_object(object_path);
src/lib/sifp/sss_sifp_private.h:sss_sifp_get_iface_for_object(const char *object_path);
src/lib/sifp/sss_sifp_utils.c:sss_sifp_get_iface_for_object(const char *object_path)
src/lib/sifp/sss_simpleifp.exports: sss_sifp_get_iface_for_object;
LS