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