[SSSD] Design Discussion: Simple D-Bus API

Jakub Hrozek jhrozek at redhat.com
Fri Mar 7 11:00:17 UTC 2014


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

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

* 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/ ?

* 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?)
  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);

* 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?

* Similarly, find_attr_as_string could drop the _string suffix.

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



More information about the sssd-devel mailing list