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. > > >* 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? > > >* 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 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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
