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

Jakub Hrozek jhrozek at redhat.com
Mon Jan 19 21:50:43 UTC 2015


On Fri, Jan 16, 2015 at 03:05:31PM +0100, Pavel Březina wrote:
> 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.

I'm not worried about the simple cases -- I've been using the patches
since Friday without issues. What I'm worried are the cases that are not
so easily tested, like a service being restarted by the monitor. There
is might make sense to allow the patches to brew in a less stable
branch.

Let's commit them to master only first and wait a bit. If the new sbus
functionality is requested by Stef or anyone else to be backported to
Fedora, then we'll backport the patches. I don't think the backport
would be too costly, after all, most changes are self-contained in the
sbus/ directory.

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

handler_data currently sounds better to me, is the pointer data consumed
anywhere else than the handler?

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

Nah, don't worry, then.

> 
> >>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.)

Sure.

> 
> >>+    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.

OK, thanks.

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

OK, let's worry about the performance later, just keep it on our minds
for the time being.

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

We don't have sss wrappers around talloc, unlike dhash..

> 
> It's your call.

The reason I got startled about the include is that I thought you were
using some low-level dhash.h functions directly instead of a sss_*
wrapper. But I didn't realize we don't wrap everything and you need to 
access hash_entry_t directly. So please ignore me here :)

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

So the part I don't like is not up, but "march". I really had no idea
what the function did based on the name..

(Sorry if I sound too nitpicking with this patchset, but the sbus code
is really low-level and every time I read it I need to re-learn half of
the code...so I'm trying hard for the code to stay readable..)
> 
> >>+{
> >>+    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

OK, I still think we should come up with a better name instead of
"march". sbus_parent_wildcard() ?

[...]

> >>+
> >>+        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.

Then at least add a comment that makes it clear we're always starting a
new list, please.

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

If it's minutes or an hour, please do. If it would be days, I can live
without it as long as the final patchset tests (which it does).

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

OK, thanks.

> 
> >>+{
> >>+    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.

Hmm, but be careful to not steal anything else onto mem_ctx. Isn't it
better to only steal individual items, then?

> 
> >>+    }
> >>+
> >>+    *_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 :-)

OK

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

Nah,

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

but, at least we found a bug :-)

> 
> >>+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.

OK, but can you add a comment that says the code is for future use?

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

If you re-send with the nitpicks and renames, I think I'll ack, although
I'd like to give the patchset a second read.



More information about the sssd-devel mailing list