[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