[SSSD] [PATCHES] sbus: support multiple interfaces on single object path

Pavel Březina pbrezina at redhat.com
Fri Jan 16 14:05:31 UTC 2015


On 01/16/2015 02:06 PM, Jakub Hrozek wrote:
> On Wed, Dec 17, 2014 at 01:36:01PM +0100, Pavel Březina wrote:
>> On 12/16/2014 01:19 PM, Pavel Březina wrote:
> After seeing the complexity of the patches, I propose to commit them to
> master only..some really fundamental code is changed and I don't feel we
> should break 1.12.x's sbus code...1.12.x is already being used by some
> downstreams. We can backport later.

I don't oppose. But the sbus code has lots of unit tests and if it would 
not work the whole sssd would stop functioning immediately so we would 
notice. I think it is very unlikely that it breaks anything and we 
wouldn't see it soon.

>>  From 2ad42ef7047ed4a6ae54e5e0ec6a3bc567c6ddbf Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 10 Dec 2014 19:24:58 +0100
>> Subject: [PATCH 01/13] sbus: add new iface via sbus_conn_register_iface()
>>
>> Rename sbus_conn_add_interface() to sbus_conn_register_iface()
>> and remove sbus_new_interface() calls since it is just one more
>> unnecessary call outside the sbus code.
>>
>> The function sbus_new_interface() is made static and used
>> directly in sbus_conn_register_iface().
>>
>> The name was chosen to better describe what the function is
>> doing. That it registers an interface on a given object path.
>> The same interface can be used with different paths so it is
>> not really about adding an interface.
>>
>> Preparation for:
>> https://fedorahosted.org/sssd/ticket/2339
>
> I agree with the intention, but can you also (maybe in another patch,
> I'll leave that up to you) consolidate the naming of the private data
> variable? It seems to be passed in as 'pvt' to the new function, but
> received 'handler_data' in the handler.
>
> Alternatively, add a comment. This is just to make the code easier to
> follow when we look at it again next time.

Do you propose pvt -> handler_data or the opposite?

>
>>  From 4b170924c8ebb57f35b0288f821686ac280c61e3 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 10 Dec 2014 22:37:17 +0100
>> Subject: [PATCH 02/13] sbus: move iface and object path code to separate file
>
> One nitpick, you changed formatting from:
>
>> -static bool sbus_iface_handles_path(struct sbus_interface_p *intf_p,
>> -                                    const char *path)
>> -{
>> -    if (sbus_path_has_fallback(intf_p->intf->path)) {
>> -        return sbus_fb_path_has_prefix(path, intf_p->intf->path);
>> -    }
>> -
>> -    return strcmp(path, intf_p->intf->path) == 0;
>> -}
>> -
>
> to:
>
>> +bool sbus_iface_handles_path(struct sbus_interface_p *intf_p,
>> +                                    const char *path)
>> +{
>> +    if (sbus_path_has_fallback(intf_p->intf->path)) {
>> +        return sbus_fb_path_has_prefix(path, intf_p->intf->path);
>> +    }
>> +
>> +    return strcmp(path, intf_p->intf->path) == 0;
>> +}
>
> Normally I wouldn't care but the latter reads funny to me. Please ignore
> if the function is modified in later patches, I'm revieweing
> patch-by-patch now.

This whole function is removed in later patches. I'll see how many 
conflicts it will create.

>> diff --git a/src/responder/ifp/ifp_domains.h b/src/responder/ifp/ifp_domains.h
>> index d6ef1a04d10ee69ea620fb638c8ee358d3fe21f5..d6ed6c73deb47a78775424afa28915b38d358777 100644
>> --- a/src/responder/ifp/ifp_domains.h
>> +++ b/src/responder/ifp/ifp_domains.h
>> @@ -26,7 +26,7 @@
>>   #include "responder/ifp/ifp_private.h"
>>
>>   #define INFOPIPE_DOMAIN_PATH_PFX "/org/freedesktop/sssd/infopipe/Domains"
>> -#define INFOPIPE_DOMAIN_PATH     INFOPIPE_DOMAIN_PATH_PFX"*"
>> +#define INFOPIPE_DOMAIN_PATH     INFOPIPE_DOMAIN_PATH_PFX"/*"
>
> Does it make sense to have some kind of global suffix #defined ? It's
> not like Components and Domains will differ in the wildcard, right?
>
> Then we can:
>      #define INFOPIPE_DOMAIN_PATH INFOPIPE_DOMAIN_PATH_PFX""IFP_SUBTREE_SUFFIX
>
> or similar.

We can do it. I'm touching these constants in my wip patches so I'll do 
this change there, ok?

(I'm unifying and shortening the names there so it stays nice with 
bigger paths.)

>> +    tree_path[len - 1] = '\0';
>> +    tree_path[len - 2] = len - 2 != 0 ? '\0' : '/';
>
> Can you add braces here or even turn into if-else ? This ternary
> operator expression was really hard to read for me.

Will do.

>> +
>> +    return tree_path;
>>   }
>
>>  From 17287e82bb882736bbb15e2423d14d007bb32624 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Thu, 11 Dec 2014 22:23:32 +0100
>> Subject: [PATCH 04/13] sbus: add sbus_opath_hash
>>
>> Add a hash table to store registered object paths and their
>> interfaces. It also provides function to add new entry and
>> lookup an interface in this table.
>>
>> When an entry or the table itself is destroyed, registered
>> object path is unregistered through delete callback.
>>
>> Preparation for:
>> https://fedorahosted.org/sssd/ticket/2339
>
> This commit cannot be compiled on its own, there's a bunch of unused
> static functions. This would break bisecting. If there's enough functions that
> it makes sense to introduce them as a single commit and use in another,
> then it might make sense to add them into a module of their own.

I guess I can move them to subsequent patch that actually uses them.

> Even though I don't like premature optimizations, I wonder if it would
> make sense to keep the paths in some tree-like structure so the same
> data can be traversed every time instead of constructing the parents and
> wildcard matches every time with _march_up(). The sbus code should be
> reasonable fast, it's quite low-level.. (Consider application that would
> crawl 10.000 user object paths..)

The code allows to traverse the object path to the root but usually only 
0-1 traverses needs to be done. It depends on how we will map paths and 
interfaces. For example:

/org/freedesktop/sssd/infopipe/Users FindByName - 0 steps up
/org/freedesktop/sssd/infopipe/Users/$uid Get(name) - 1 step up

So I don't expect this to be a bottle neck. If it turns out to be one, 
we can create a more complex and sophisticated way.

>> ---
>>   src/sbus/sssd_dbus_interface.c | 241 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 241 insertions(+)
>>
>> diff --git a/src/sbus/sssd_dbus_interface.c b/src/sbus/sssd_dbus_interface.c
>> index cd41c4f29f700d010836a45c73092a0eb16ec9f2..e1275377b4afaf299c20adeff64896b46e9b53cf 100644
>> --- a/src/sbus/sssd_dbus_interface.c
>> +++ b/src/sbus/sssd_dbus_interface.c
>> @@ -20,6 +20,7 @@
>>
>>   #include <talloc.h>
>>   #include <dbus/dbus.h>
>> +#include <dhash.h>
>
> I don't think we need to include dhash directly, since we're using
> sss_hash_* right?

sss_hash_* is for initialization only. It is probably unnecessary, but 
I'm used to #include files directly in the file that uses them. But I 
can removed it.

But in that case I don't have it #include dbus and talloc as well...

It's your call.

>>   #include "util/util.h"
>>   #include "sbus/sssd_dbus.h"
>> @@ -28,6 +29,26 @@
>>   DBusObjectPathVTable dbus_object_path_vtable =
>>       { NULL, sbus_message_handler, NULL, NULL, NULL, NULL };
>>
>> +struct sbus_interface_list {
>> +    struct sbus_interface_list *prev, *next;
>> +    struct sbus_interface *interface;
>> +};
>> +
>> +static struct sbus_interface *
>> +sbus_iface_list_lookup(struct sbus_interface_list *list,
>> +                       const char *iface)
>> +{
>> +    struct sbus_interface_list *item;
>> +
>> +    DLIST_FOR_EACH(item, list) {
>> +        if (strcmp(item->interface->vtable->meta->name, iface) == 0) {
>> +            return item->interface;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>>   static bool path_in_interface_list(struct sbus_interface_p *list,
>>                                      const char *path)
>>   {
>> @@ -121,6 +142,42 @@ static char *sbus_opath_get_base_path(TALLOC_CTX *mem_ctx,
>>       return tree_path;
>>   }
>>
>> +static char *sbus_opath_march_up(TALLOC_CTX *mem_ctx,
>> +                                 const char *path)
>
> sbus_opath_parent()?

I imagine that sbus_opath_parent does the following:
/org/freedesktop -> /org -> /

march_up does:
/org/freedesktop -> /org/* -> /*

So I wouldn't choose _parent.

>> +{
>> +    char *subtree;
>> +    char *slash;
>> +
>> +    /* first remove /~* from the end, stop when we have reached the root i.e.
>> +     * subtree == "/" */
>> +    subtree = sbus_opath_get_base_path(mem_ctx, path);
>> +    if (subtree == NULL || subtree[1] == '\0') {
>> +        return NULL;
>> +    }
>> +
>> +    /* Find the first separator and replace the part with asterisk. */
>> +    slash = strrchr(subtree, '/');
>> +    if (slash == NULL) {
>> +        /* we cannot continue up */
>> +        talloc_free(subtree);
>> +        return NULL;
>> +    }
>> +
>> +    if (*(slash + 1) == '\0') {
>> +        /* this object path is invalid since it cannot end with slash */
>> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid object path '%s'?\n", path);
>> +        talloc_free(subtree);
>> +        return NULL;
>> +    }
>> +
>> +    /* because object path cannot end with / there is enough space for
>> +     * asterisk and terminating zero */
>> +    *(slash + 1) = '*';
>> +    *(slash + 2) = '\0';
>> +
>> +    return subtree;
>
> confused..a subtree should be something further down the hierarchy, but
> here we're constructing a parent path, right?

subtree of a parent path, see above

>> +}
>> +
>>   bool sbus_iface_handles_path(struct sbus_interface_p *intf_p,
>>                                       const char *path)
>>   {
>> @@ -131,6 +188,190 @@ bool sbus_iface_handles_path(struct sbus_interface_p *intf_p,
>>       return strcmp(path, intf_p->intf->path) == 0;
>>   }
>>
>> +static void
>> +sbus_opath_hash_delete_cb(hash_entry_t *item,
>> +                          hash_destroy_enum deltype,
>> +                          void *pvt)
>> +{
>> +    struct sbus_connection *conn;
>> +    char *path;
>> +
>> +    conn = talloc_get_type(pvt, struct sbus_connection);
>> +    path = sbus_opath_get_base_path(NULL, item->key.str);
>> +
>> +    dbus_connection_unregister_object_path(conn->dbus.conn, path);
>> +}
>> +
>> +static errno_t
>> +sbus_opath_hash_init(TALLOC_CTX *mem_ctx,
>> +                     struct sbus_connection *conn,
>> +                     hash_table_t **_table)
>> +{
>> +    return sss_hash_create_ex(mem_ctx, 10, _table, 0, 0, 0, 0,
>> +                              sbus_opath_hash_delete_cb, conn);
>> +}
>> +
>> +static errno_t
>> +sbus_opath_hash_add_iface(hash_table_t *table,
>> +                          const char *object_path,
>> +                          struct sbus_interface *iface,
>> +                          bool *_path_known)
>> +{
>> +    TALLOC_CTX *tmp_ctx = NULL;
>> +    struct sbus_interface_list *list = NULL;
>> +    struct sbus_interface_list *item = NULL;
>> +    const char *iface_name = iface->vtable->meta->name;
>> +    hash_key_t key;
>> +    hash_value_t value;
>> +    errno_t ret;
>> +    int hret;
>> +
>> +    tmp_ctx = talloc_new(NULL);
>> +    if (tmp_ctx == NULL) {
>> +        return ENOMEM;
>> +    }
>> +
>> +    DEBUG(SSSDBG_TRACE_FUNC, "Registering interface %s with path %s\n",
>> +          iface_name, object_path);
>> +
>> +    /* create new list item */
>> +
>> +    item = talloc_zero(tmp_ctx, struct sbus_interface_list);
>> +    if (item == NULL) {
>> +        return ENOMEM;
>> +    }
>> +
>> +    item->interface = iface;
>> +
>> +    /* first lookup existing list in hash table */
>> +
>> +    key.type = HASH_KEY_STRING;
>> +    key.str = talloc_strdup(tmp_ctx, object_path);
>> +    if (key.str == NULL) {
>> +        ret = ENOMEM;
>> +        goto done;
>> +    }
>> +
>> +    hret = hash_lookup(table, &key, &value);
>> +    if (hret == HASH_SUCCESS) {
>> +        /* This object path has already some interface registered. We will
>> +         * check for existence of the interface currently being added and
>> +         * add it if missing. */
>> +
>> +        *_path_known = true;
>
> I would prefer a local path_known function rather than touching the
> output variable elsewhere than the done: handler. The compiler will most
> likely optimize it out, but it's more idiomatic for SSSD code and allows
> to check that a certain fallback value is always used (since the local
> variable will be initialized to something safe)

Ok.

>> +
>> +        list = talloc_get_type(value.ptr, struct sbus_interface_list);
>> +        if (sbus_iface_list_lookup(list, iface_name) != NULL) {
>> +            DEBUG(SSSDBG_MINOR_FAILURE, "Trying to register the same interface"
>> +                  " twice: iface=%s, opath=%s\n", iface_name, object_path);
>> +            ret = EEXIST;
>> +            goto done;
>> +        }
>> +
>> +        DLIST_ADD_END(list, item, struct sbus_interface_list *);
>> +        ret = EOK;
>> +        goto done;
>> +    } else if (hret != HASH_ERROR_KEY_NOT_FOUND) {
>> +        ret = EIO;
>> +        goto done;
>> +    }
>> +
>> +    /* otherwise create new hash entry */
>> +
>> +    *_path_known = false;
>> +    DLIST_ADD(list, item);
>
> Can you just use list = item here? Looks like list is always NULL here. Or
> do you feel it's less readable?

I personally like this one better but I don't oppose.

>>  From cd4afa9147f584f76e131bfe751ead7d720267aa Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Fri, 12 Dec 2014 12:09:02 +0100
>> Subject: [PATCH 05/13] sbus: support multiple interfaces on single path
>>
>> This patch removes the old message handler which is replaced
>> with a new one that supports multiple interfaces registered
>> on single object path.
>>
>> It temporarily removes support of Introspect and Properties
>> standard D-Bus interfaces. The support is brought back by
>> following patches.
>
> How much work would it be to ifdef out the tests that don't run as a
> result of this change to see what works after each patch?

Dunno. I can try.

>>  From 0ce78e07f422f5b9d78beffee8c277cfb5387df0 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Sat, 13 Dec 2014 00:15:39 +0100
>> Subject: [PATCH 07/13] sbus: add sbus_opath_hash_lookup_supported()
>>
>> This function acquirer list of all interfaces that are supported on
>                       ~~~
>                       typo

Will fix.

>> given object path. It is a preparation for Introspect interface.
>> ---
>>   src/sbus/sssd_dbus_interface.c | 123 +++++++++++++++++++++++++++++++++++++++--
>>   src/sbus/sssd_dbus_private.h   |  11 ++++
>>   2 files changed, 129 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/sbus/sssd_dbus_interface.c b/src/sbus/sssd_dbus_interface.c
>> index 4433b508c000e7b7dc7c963e9e1b1026c9d41e50..932348f851ba4360f405434bf6654c4832e600a7 100644
>> --- a/src/sbus/sssd_dbus_interface.c
>> +++ b/src/sbus/sssd_dbus_interface.c
>> @@ -27,11 +27,6 @@
>>   #include "sbus/sssd_dbus_meta.h"
>>   #include "sbus/sssd_dbus_private.h"
>>
>> -struct sbus_interface_list {
>> -    struct sbus_interface_list *prev, *next;
>> -    struct sbus_interface *interface;
>> -};
>> -
>>   static struct sbus_interface *
>>   sbus_iface_list_lookup(struct sbus_interface_list *list,
>>                          const char *iface)
>> @@ -47,6 +42,55 @@ sbus_iface_list_lookup(struct sbus_interface_list *list,
>>       return NULL;
>>   }
>>
>> +static errno_t
>> +sbus_iface_list_copy(TALLOC_CTX *mem_ctx,
>> +                     struct sbus_interface_list *list,
>> +                     struct sbus_interface_list **_copy)
>
> I'm fine with the copy as long as it's only used for introspection,
> which is a really rare operation.

Yes, it is only for introspection. Also the number of interfaces will be 
in tens at most so it will not be a bottle neck.

>> +{
>> +    TALLOC_CTX *tmp_ctx;
>> +    struct sbus_interface_list *new_list = NULL;
>> +    struct sbus_interface_list *new_item;
>> +    struct sbus_interface_list *item;
>> +    errno_t ret;
>> +
>> +    if (list == NULL) {
>> +        *_copy = NULL;
>> +        return EOK;
>> +    }
>> +
>> +    tmp_ctx = talloc_new(NULL);
>> +    if (tmp_ctx == NULL) {
>> +        return ENOMEM;
>> +    }
>> +
>> +    DLIST_FOR_EACH(item, list) {
>> +        if (sbus_iface_list_lookup(new_list,
>> +               item->interface->vtable->meta->name) != NULL) {
>> +            /* already in list */
>> +            continue;
>> +        }
>> +
>> +        new_item = talloc_zero(tmp_ctx, struct sbus_interface_list);
>> +        if (new_item == NULL) {
>> +            ret = ENOMEM;
>> +            goto done;
>> +        }
>> +
>> +        new_item->interface = item->interface;
>> +        DLIST_ADD(new_list, new_item);
>
> New item is added onto new_list, yet it seems allocated on tmp_ctx..

I should probably talloc_steal(mem_ctx, tmp_ctx) since list items should 
not be attached to another item.

>> +    }
>> +
>> +    *_copy = talloc_steal(mem_ctx, new_list);
>> +    ret = EOK;
>> +
>> +done:
>> +    if (ret != EOK) {
>> +        talloc_free(tmp_ctx);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   /**
>>    * Object paths that represent all objects under the path:
>>    * /org/object/path/~* (without tilda)
>> @@ -311,6 +355,75 @@ done:
>>       return iface;
>>   }
>>
>> +/**
>> + * Acquire list of all interfaces that are supported on given object path.
>> + */
>> +errno_t
>> +sbus_opath_hash_lookup_supported(TALLOC_CTX *mem_ctx,
>> +                                 hash_table_t *table,
>> +                                 const char *object_path,
>> +                                 struct sbus_interface_list **_list)
>
> Would sbus_get_opath_ifaces() be a better name?

Well it does lookup interfaces that are supported on given object path 
working with hash table, so I think this is the most precise name :-)

>>  From 7cf0af2d532dfdcedd3c7f03615217d5c945d6f0 Mon Sep 17 00:00:00 2001
>> +#define METHOD_HAS_ARGS(m) ((m)->in_args != NULL && (m)->out_args != NULL)
>> +#define SIGNAL_HAS_ARGS(s) ((s)->args != NULL)
>
> Is it necessary to defined these as macros and then use only once..

Those are not code shortening macros but semantic macros...

if (method->in_args == NULL && method->out_args == NULL)
if (!METHOD_HAS_ARGS(method))

Which one reads better? But I can remove it... your call.

FYI there is an error in METHOD_HAS_ARGS. It should contain || instead 
of &&.

>> +static int
>> +sbus_introspect_generate_args(FILE *file,
>> +                              const struct sbus_arg_meta *args,
>> +                              const char *direction)
>
> It would be much more readable to use an enum here than rely on
> direction==NULL implying 'it's a signal'.

Ok. I agree.

>>  From f19a8e250fb10d3a2a2174b9c0357cbd6e8e1a3d Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Sun, 14 Dec 2014 19:02:49 +0100
>> Subject: [PATCH 09/13] sbus: support org.freedesktop.DBus.Properties
>>
>> Bring back org.freedesktop.DBus.Properties with support of
>> multiple interfaces on single object path.
>
> Tests pass again after this point.
>
> Do you think it makes sense to keep setters around even though we don't
> implement them?

I thought about it than I decided to preimplement it the same way as the 
old code did. I think we can keep the code around since setters will be 
implementet soon, I hope. Techniquily the only think that is missing are 
handlers themselves and this code works as is.

>> ---
>>   src/sbus/sssd_dbus_interface.c  |  13 +-
>>   src/sbus/sssd_dbus_private.h    |   6 +
>>   src/sbus/sssd_dbus_properties.c | 513 +++++++++++++++++++++++-----------------
>>   3 files changed, 312 insertions(+), 220 deletions(-)
>>
>> diff --git a/src/sbus/sssd_dbus_interface.c b/src/sbus/sssd_dbus_interface.c
>> index cbabd022358814f2c117c36367f6e5b754a7ffe7..16919c967b2d3e11036a78f9328b358da0503db7 100644
>> --- a/src/sbus/sssd_dbus_interface.c
>> +++ b/src/sbus/sssd_dbus_interface.c
>> @@ -304,7 +304,7 @@ sbus_opath_hash_has_path(hash_table_t *table,
>>    * in the path hierarchy and try to lookup the parent node. This continues
>>    * until the root is reached.
>>    */
>> -static struct sbus_interface *
>> +struct sbus_interface *
>>   sbus_opath_hash_lookup_iface(hash_table_t *table,
>>                                const char *object_path,
>>                                const char *iface_name)
>> @@ -547,8 +547,19 @@ sbus_conn_register_iface(struct sbus_connection *conn,
>>       }
>>
>>       /* register standard interfaces with this object path as well */
>> +    ret = sbus_conn_register_iface(conn, sbus_properties_vtable(),
>> +                                   object_path, conn);
>> +    if (ret != EOK) {
>> +        return ret;
>> +    }
>> +
>>       ret = sbus_conn_register_iface(conn, sbus_introspect_vtable(),
>>                                      object_path, conn);
>> +    if (ret != EOK) {
>> +        return ret;
>> +    }
>> +
>> +    ret = EOK;
>
> Can't we end the function with return EOK since there's no done: label?

Yep.

>>
>>       return ret;
>>   }

>>   /*
>>       Authors:
>>           Stef Walter <stefw at redhat.com>
>> +        Pavel Březina <pbrezina at redhat.com>
>>
>>       Copyright (C) 2014 Red Hat
>>
>> +    SBUS: Interface introspection
>
> Wrong comment :-)

Ok :)

Thank you for the review!




More information about the sssd-devel mailing list