[SSSD] Design Discussion: Simple D-Bus API

Pavel Březina pbrezina at redhat.com
Tue Mar 11 10:10:05 UTC 2014


On 03/10/2014 06:14 PM, Sumit Bose wrote:
> On Fri, Mar 07, 2014 at 03:02:30PM +0100, Jakub Hrozek wrote:
>> On Fri, Mar 07, 2014 at 12:48:31PM +0100, Pavel Březina wrote:
>>> On 03/07/2014 12:00 PM, Jakub Hrozek wrote:
>>>> On Fri, Mar 07, 2014 at 11:08:17AM +0100, Pavel Březina wrote:
>>>>> On 02/25/2014 10:11 AM, Pavel Březina wrote:
>>>>>> On 02/24/2014 01:17 PM, Jakub Hrozek wrote:
>>>>>>> On Mon, Feb 24, 2014 at 12:32:04PM +0100, Pavel Březina wrote:
>>>>>>>> https://fedorahosted.org/sssd/wiki/DesignDocs/DBusResponder
>>>>>>>>
>>>>>>>> Hi, I couldn't find the original thread so I'm starting a new
>>>>>>>> one.
>>>>>>>
>>>>>>> The design was reviewed in a private thread mostly as the API
>>>>>>> evolved organically from a proof of concept. You're right we
>>>>>>> should have followed up on the devel list.
>>>>>>>
>>>>>>>> I would like us to agree on best practice of naming methods. The
>>>>>>>> current design is somewhat inconsistent, since it uses e.g.
>>>>>>>> FindUserByName(name) and GetDomain(name) for obtaining one
>>>>>>>> object path by name.
>>>>>>>>
>>>>>>>> I'm proposing the following convention: * List<class>()
>>>>>>>> returning array of object paths, no arguments - ListUsers -
>>>>>>>> ListDomains * Find<class><condition>(arg1, ...) returning array
>>>>>>>> of object paths - FindUsersByName(filter) -
>>>>>>>> FindGroupsByName(filter) * Get<class><condition>(arg1, ...)
>>>>>>>> returning single object path - GetUserById(id) -
>>>>>>>> GetDomainByName(name)
>>>>>>>
>>>>>>> FWIW, I modeled the API based on the existing AccountsService API
>>>>>>> and Nalin's subsequent redesign document. I agree that more
>>>>>>> consistent API would be good, but I would prefer to always use
>>>>>>> Find everywhere to avoid clashing with Get/GetAll from
>>>>>>> DBus.Properties.
>>>>>>
>>>>>> Simple D-Bus API design:
>>>>>> https://fedorahosted.org/sssd/wiki/DesignDocs/DBusSimpleAPI
>>>>>>
>>>>>> I suggested this convention namely for the following three functions:
>>>>>> * char ** sss_dbus_invoke_list * char ** sss_dbus_invoke_find * char
>>>>>> *  sss_dbus_invoke_get
>>>>>>
>>>>>> These functions invoke [List|Find|Get]<method> D-Bus method and
>>>>>> parse the result into either array of object path (list, find) or
>>>>>> single item (get).
>>>>>>
>>>>>> If we strictly follow the convention (or other we agree on), it will
>>>>>> allow us to expose new D-Bus methods via this API without actually
>>>>>> modifying the API which I find quite nice.
>>>>>>
>>>>>> You can oppose that user still needs to remember D-Bus method name
>>>>>> and its arguments but I think this is a good compromise between
>>>>>> simplicity and generality. Also user can provide invalid method name
>>>>>> or invalid arguments but than it will simply return an error as it
>>>>>> would do so with libdbus.
>>>>>
>>>>> I have done few changes into the API:
>>>>>
>>>>> * A statement about multi threaded usage was added: each thread needs to
>>>>>    use its own context.
>>>>>
>>>>> * Error reporting changed. Previously, the functions returned structure
>>>>>    (code, message), but this proved to be quite an inconvenience.
>>>>>    Since the only purpose of returning message was to pass a D-Bus error
>>>>>    to user, I changed it to:
>>>>>    - return only error code
>>>>>    - save last D-Bus error in sss_dbus context
>>>>>    - provide a function to fetch this error
>>>>
>>>> I general, I think the proposal is good. I have a couple of comments:
>>>>
>>>> * I still disagree that we should use Get* at all in the DBus API, but
>>>>    rather we should use FindXXXX only to avoid clashing with the
>>>>    ObjectManager API
>>>
>>> Sure, I don't really oppose. I just want to see consistency between
>>> method prefix and returned value.
>>>
>>> So should we have List* without parameters, Find* with parameters
>>> both returning array of object class?
>>>
>>> Or List* with parameters, returning array of object class, and Find*
>>> with parameters, returning single object class?
>>
>> I like this approach better. I'm (finally!) able to do some more DBus
>> responder work today and I'm incorporating some input from Stef into the
>> design page. I would appreciate your feedback. I'll reply to this thread
>> when I'm done.
>>
>>>
>>>> * Please add a comment to the header saying that the interface is
>>>>    minimal on purpose and the user should use the full DBus power
>>>>    instead. Yes, this conflicts with the next point somewhat, but I think
>>>>    we should strike the right balance.
>>>
>>> OK.
>>>
>>>> * The API is a wrapper around DBus interface that is supposed to shield
>>>>    you from DBus completely. I wonder if the API should simply do
>>>>    s/dbus/ifp/ ?
>>>
>>> I actually never wanted to shield D-Bus completely. I wanted it only
>>> to hide the complex things (error handling, many calls to D-Bus API
>>> to simply send a message, parsing attributes, ...).
>>
>> Fair enough. I saw both complaints from potential consumers -- some
>> developers were going "omg dbus is such a heavyweight desktop technology,
>> do I really need to use it" ? I realize 'hiding' dbus behind a different
>> name might be a little irrational, but so were their concerns in my
>> opinion.
>
> I think as long as well call it the D-Bus responder we should keep dbus
> in the names. Otherwise we might get questions like 'How do I start the
> InfoPipe responder?'.
>
>>
>>>
>>>> * Similar for accepting method names as parameters. I see the point in
>>>>    having a lot of flexibility, but it seems like the interface is
>>>>    exposing too many details this way and being a bit error-prone (Do I
>>>>    user "user" or "User"? What happens if you use a class that doesn't
>>>>    exist?)
>>>
>>> I think the flexibility here is a very key point. It will allow you
>>> to use the API without adding new functions/enum for every new
>>> object we expose in SSSD. I wanted to avoid this.
>>>
>>> If you put an invalid method, it is the same as if you use D-Bus and
>>> put there an invalid method. It will error out with a nice message
>>> coming from D-Bus explaining what's wrong.
>>
>> How often would we add new objects?
>>
>> But I'm not too opposed, anyway.
>>
>>>
>>>>    What about adding an enum that would encapsulate the types and passing
>>>>    an enum instead of a free-form name? So instead of:
>>>>      ret = sss_dbus_invoke_list(ctx, "user", &paths);
>>>>      ret = sss_dbus_invoke_list(ctx, "group", &paths);
>>>>    you would call:
>>>>      ret = sss_ifp_invoke_list(ctx, IFP_CLS_USR, &paths);
>>>>      ret = sss_ifp_invoke_list(ctx, IFP_CLS_GRP, &paths);
>>>
>>>
>>> I thought of it. But then again, you will add a new object class,
>>> say sudorules. You will need to amend the library as well.
>>>
>>> If we do this, we would also have to create symbols for every
>>> method. To make the check complete, we would have to also check
>>> parameters of every method. Doing it on compile time would require
>>> to create a function for every method we want to expose via this
>>> library.
>>>
>>> It's getting kinda big, isn't it?
>
> If we allow that user generated strings are used here I think we have to
> make sure that the D-Bus objects paths will be always available under
> the same name to not break any application using this interface.
>
> Nevertheless I think it would be useful to add some convenience calls
> for known and common use cases. E.g.
> sss_dbus_list_domains(sss_dbus_ctx *ctx, char **domain_list),
> sss_dbus_fetch_domain(sss_dbus_ctx *ctx, const char *dom_name, sss_dbus_attr ***dom_attrs),
> sss_dbus_fetch_user(sss_dbus_ctx *ctx, const char *user_name, sss_dbus_attr **user_attrs)

OK, we can do this.

>
>>>
>>>> * The API still uses types:
>>>>      sss_dbus_invoke_get(sss_dbus_ctx *ctx, const char *method,
>>>>                          char **_object_path, int first_arg_type, ...);
>>>>     Didn't we say the API should be type-less and everythign should be a
>>>>     string?
>>>
>>> Did we agree also on input attribute? I think we said this to be
>>> true only for output.
>>
>> Ah, I see. Yes, it would be possible to deduct the type from the method,
>> but let's not do that. Especially if the goal is not to build a layer
>> that shields from dbus completely.
>>
>>>
>>>> * Similarly, find_attr_as_string could drop the _string suffix.
>>>
>>> It's meant that attributes are arrays, in general. _string will
>>> return it only as single value, _array will return it as is. Do you
>>> have a better name?
>>
>> Simply find_attr(). Just drop the suffix.
>
> I wonder if with this interfaces we can make sss_dbus_attr and maybe
> even sss_dbus_object opaque?

Well, we can. I originally had it opaque but then scratch it. I thought 
that the user may want to perform some advanced filtering or sorting on 
attributes for further processing, thus I made it a known type. I don't 
think it is something that will be susceptible to changes.

> I wonder what the use case for a sss_dbus_object might be?

To have all information (object path, attributes and name) on one place. 
If you will need to remember object path, together with attributes you 
can just create sss_dbus_object, instead of having two variables. Name 
is usually part of output/trace, so I made it easily accessible.

> bye,
> Sumit
>>
>>>
>>>> * I think we could remove sss_dbus_fetch_attrs() and only allow
>>>>    either one attribute or all of them. The bus would publish them all anyway.
>>>
>>> How so? Properties.Get should return only one attribute at time,
>>> GetAll all of them.
>>
>> Right, that's what I'm saying. I don't get the point of a function that
>> only lets you filter some attributes, it seems that a single attribute
>> or all of them would be OK.
>>
>>>
>>>> * How does the API deal with requesting an object that is not yet
>>>>    published on the bus? There should be an error code for this (common)
>>>>    mistake
>>>
>>> D-Bus error. The same as if you would request it via D-Bus.
>>
>> OK. Needs a special error code, though.
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>




More information about the sssd-devel mailing list