[SSSD] [PATCH] IFP: Add GetUserAttrs call

Stef Walter stefw at redhat.com
Thu Apr 24 10:59:45 UTC 2014


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#ga2fea5f972d1bfe7bcde8c0ec65ca9e90

> [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



More information about the sssd-devel mailing list