On Thu, Apr 24, 2014 at 12:59:45PM +0200, Stef Walter wrote:
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...
Thanks, much appreciated!
> [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()
Done, this makes sense, thanks for the suggestion.
> [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?
I implemented what you suggested in patch 5/9. It's not polished yet,
because I wanted to check if that's what you had in mind before spending
too much time.
In particular, I would like to chain concurrent requests for the same
sender ID to not trash the sysbus if many requests came in after startup
for instance. Also unit tests are needed.
I'm not too happy about sbus_message_handler now either, I'd like to
refactor it to be more readable, maybe pass around a single struct
instance instead of handler/sender/interface/etc..
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.
This variable went away in the current patchset.
> +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...
Ah, OK, I must say this part was always a bit confusing to me. Why do we try
to send messages crated with dbus_message_new_error(DBUS_ERROR_NO_MEMORY)
in sbus_message_handler, should those be fixed too?
In the current patchset, I removed ifp_enomem() completely and I'd like
to add the preallocated handler in a future patch (tracked by a ticket,
otherwise we'd forget), since ENOMEM is very unlikely on Linux anyway. The
current patchset just calls "return sbus_request_finish(dbus_req, NULL);"
in an OOM situation.
> [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.
This was actually fixed in the next patch in my tree where I don't
assign to nattrs directly, which is why I never saw the error probably.
A new patchset is attached. Thanks again for the review.
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.
I wrote some helpers when working on the property getters, maybe when we
review the property access, we could take a look at making them more
generic..but in general I agree, the repetitive tasks should be
abstracted.