On 23.04.2014 22:43, Jakub Hrozek wrote:
Hi,
the attached patches upstream functionality that mod_lookup_identity
has been using for a while, but which we couldn't push to master due
to the pending sbus changes. Now that these are accepted, it's time
to merge the DBus methods themselves. Previously, mod_lookup_identity
relied on a private branch with quite a bit of ad-hoc code.
Please note that when the work in progress to expose users, groups
and domains is merged as well, this interface will become obsolete in
favor of the objects. But we shouldn't let a released version of
mod_lookup_identity rely on unreleased sssd code any longer.
Moreover, all patches except the last one will be used even later on
and there's no fixed timeline on converting mod_lookup_identity.
Did some high level review of the DBus related stuff...
[PATCH 1/6] SBUS: two trivial style fixes SSIA
[PATCH 2/6] SBUS: Add a convenience function Adds a convenience
function that constructs a DBusError internally and as such can be
used to mark an sbus request as failed without having to create a
DBusError instance by the caller. The error message must be either
allocated on top of dbus_req or a constant, DBus itself doesn't copy
the error message! This is reflected in the doxygen docstring.
Hmmm, rather than that (and because of the dbus behavior), my preference
would be to have an sbus_error_new() method that created a DBusError on
a MEM_CTX, and could be used like this:
sbus_request_fail_and_finish (dbus_req,
sbus_error_new (dbus_req,
"dotted.Error",
"format str: %s", arg));
This is what I sorta imagined when implementing
sbus_request_fail_and_finish()
[PATCH 3/6] IFP: Add utility functions Adds a number of utility
functions, most importanly ifp_req_create(). The ifp_req is a
structure that will be passed along with the ifp request and contains
the sbus request and some ifp-specific data, like caller ID or the
responder context.
dbus_bus_get_unix_user() blocks. Is that fine in this case?
What we really want is to be able to get this information asyncronously
and fill it in the sbus_request before dispatching a given method call
to the handler. We would also cache this information on sbus_connection.
+{ + const char *conn_name;
Suggest using 'sender' instead of 'conn_name' for this variable name.
+int ifp_enomem(struct ifp_req *ireq)
This won't work as you expect. When you're out of memory it's unlikely
that you'll be able to send a normal message. DBus has method calls
specifically for this case:
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga2fe...
[PATCH 6/6] IFP: Add GetUserAttrs call Adds a DBus method that
allows
the caller to retrieve attributes of a user. The synopsis of the call
is as follows: <method name="GetUserAttr"> <arg type="s"
name="user"
direction="in"/> <arg type="as" name="attr"
direction="in"/> <arg
type="a{sv}" name="values" direction="out"/>
</method>
+ parsed =
sbus_request_parse_or_finish(attr_req->ireq->dbus_req,
+ DBUS_TYPE_STRING, &attr_req->name,
+ DBUS_TYPE_ARRAY, DBUS_TYPE_STRING,
+ &attr_req->attrs,
+ &attr_req->nattrs,
+ DBUS_TYPE_INVALID);
attr_req->nattrs is of the wrong type. It needs to be an int. This shows
why using RawHandler is a bit tricky. I wish we had support for
unwrapping and wraping a{sv}. Is there any good sssd data structure that
it would make sense to represent such a dict as? Probably pretty tough.
Cheers,
Stef