[SSSD] [PATCH] libsss_dbus

Jakub Hrozek jhrozek at redhat.com
Fri May 16 13:32:53 UTC 2014


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?

> 
> >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.
> >
> >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.
> 
> 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==
> >
> >* 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.



More information about the sssd-devel mailing list