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)
>
> >* 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?
I wonder what the use case for a sss_dbus_object might be?
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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel