I realized that I forgot to add support for having both direct object path and fallback defined. I.e.:
/org/freedesktop/sssd/infopipe /org/freedesktop/sssd/infopipe/*
On 12/16/2014 01:19 PM, Pavel Březina wrote:
I realized that I forgot to add support for having both direct object path and fallback defined. I.e.:
/org/freedesktop/sssd/infopipe /org/freedesktop/sssd/infopipe/*
And one more time. While working on some improvements on ifp and sbus code I found out that there are some copy & paste relics in the last patch. Now they are gone.
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:
I realized that I forgot to add support for having both direct object path and fallback defined. I.e.:
/org/freedesktop/sssd/infopipe /org/freedesktop/sssd/infopipe/*
And one more time. While working on some improvements on ifp and sbus code I found out that there are some copy & paste relics in the last patch. Now they are gone.
There are some Coverity warnings: http://cov01.lab.eng.brq.redhat.com/covscanhub/task/16341/log/fixed.err
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 haven't done any testing yet except I'm going to install the whole patchset on my laptop and will run it for some time. Proper testing needs to include crashing services on purpose, too. So all ACKs here are just code-wise for now.
From 2ad42ef7047ed4a6ae54e5e0ec6a3bc567c6ddbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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.
From 4b170924c8ebb57f35b0288f821686ac280c61e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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.
From 2a14d7d8fae7c31bf96c26dd084b4be2cb72ad7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 11 Dec 2014 11:01:57 +0100 Subject: [PATCH 03/13] sbus: use 'path/*' to represent a D-Bus fallback
Two nitpicks inline.
Use 'path/*' instead of 'path*' since it better describes what we are actually doing i.e. registering a message handler for a subtree.
Although D-Bus fallback will invoke a message handler for both 'path' and 'path/subtree' object paths it does not make usually sence to support the same interfaces for both parent and it children.
This commit also renames related functions to better describe what are they doing.
Note: the tilda in comments is used to suppress -Wcomment warning
Preparation for: https://fedorahosted.org/sssd/ticket/2339
src/responder/ifp/ifp_components.h | 2 +- src/responder/ifp/ifp_domains.h | 2 +- src/sbus/sssd_dbus_interface.c | 92 +++++++++++++++++++++++--------------- 3 files changed, 58 insertions(+), 38 deletions(-)
diff --git a/src/responder/ifp/ifp_components.h b/src/responder/ifp/ifp_components.h index 49f1294cd32bdd6e6a2cbbf2937e1e66e0ad4dc8..6c6300442762c083e53b5be524d706d0dcf508d4 100644 --- a/src/responder/ifp/ifp_components.h +++ b/src/responder/ifp/ifp_components.h @@ -25,7 +25,7 @@ #include "responder/ifp/ifp_private.h"
#define INFOPIPE_COMPONENT_PATH_PFX "/org/freedesktop/sssd/infopipe/Components" -#define INFOPIPE_COMPONENT_PATH INFOPIPE_COMPONENT_PATH_PFX "*" +#define INFOPIPE_COMPONENT_PATH INFOPIPE_COMPONENT_PATH_PFX "/*"
#define INFOPIPE_BACKEND_PATH INFOPIPE_COMPONENT_PATH_PFX "/Backends*"
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.
/* org.freedesktop.sssd.infopipe */
[...]
+static char *sbus_opath_get_base_path(TALLOC_CTX *mem_ctx,
const char *object_path){
- char *wildcard;
- wildcard = strrchr(path, '*');
- if (wildcard != NULL) {
/* This path was registered as fallback */if (*(wildcard + 1) != '\0') {/* Wildcard is only allowed as the last character in the path */return false;}return true;
- char *tree_path;
- size_t len;
- tree_path = talloc_strdup(mem_ctx, object_path);
- if (tree_path == NULL) {
return NULL;
- return false;
- if (!sbus_opath_is_subtree(tree_path)) {
return tree_path;- }
- /* replace / only if it is not a root path (only slash) */
- len = strlen(tree_path);
- 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.
- return tree_path;
}
From 17287e82bb882736bbb15e2423d14d007bb32624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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.
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..)
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?
#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()?
+{
- 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?
+}
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)
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?
From cd4afa9147f584f76e131bfe751ead7d720267aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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?
ACK
From 54980360b218dfdb1f8d6dd79435e6ae68b6224d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 12 Dec 2014 15:14:09 +0100 Subject: [PATCH 06/13] sbus: add object path to sbus request
ACK
From 0ce78e07f422f5b9d78beffee8c277cfb5387df0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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
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.
+{
- 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..
- }
- *_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?
From 7cf0af2d532dfdcedd3c7f03615217d5c945d6f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Sat, 13 Dec 2014 14:25:14 +0100 Subject: [PATCH 08/13] sbus: support org.freedesktop.DBus.Introspectable
I'm glad we got rid of sbus_introspect_ctx, I never liked it :-)
This commit brings back support of Introspectable interface and enables support of multiple interfaces there. It also refactors the old code so the generator and introspect xml format especially is more readable.
[...]
+#define WRITE_DATA(file, ret, label, fmt, ...) do { \
- ret = fprintf(file, fmt, ##__VA_ARGS__); \
- if (ret < 0) { \
ret = EIO; \goto label; \
Ugh, a macro that changes program flow...I know how unlikely it is that fprintf() fails, but it's really hard to read code where you don't know where it would jump to..
- } \
+} while (0)
+#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..
[...]
+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'.
From f19a8e250fb10d3a2a2174b9c0357cbd6e8e1a3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@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?
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?
return ret;} diff --git a/src/sbus/sssd_dbus_private.h b/src/sbus/sssd_dbus_private.h index 62c2d6f9e71e40601830ff797533b0aba647944d..24b6bec18cf8331942480312505b2dce77d2c2ac 100644 --- a/src/sbus/sssd_dbus_private.h +++ b/src/sbus/sssd_dbus_private.h @@ -69,6 +69,7 @@ struct sbus_connection { /* =Standard=interfaces=================================================== */
struct sbus_vtable *sbus_introspect_vtable(void); +struct sbus_vtable *sbus_properties_vtable(void);
/* =Watches=============================================================== */
@@ -117,6 +118,11 @@ sbus_opath_hash_init(TALLOC_CTX *mem_ctx, struct sbus_connection *conn, hash_table_t **_table);
+struct sbus_interface * +sbus_opath_hash_lookup_iface(hash_table_t *table,
const char *object_path,const char *iface_name);errno_t sbus_opath_hash_lookup_supported(TALLOC_CTX *mem_ctx, hash_table_t *table, diff --git a/src/sbus/sssd_dbus_properties.c b/src/sbus/sssd_dbus_properties.c index c6bdffda72b5a02f352cdeb27ac75b30bf07d6ba..f82dfe731b90ee20cdbe5d4762542ccbc37d744b 100644 --- a/src/sbus/sssd_dbus_properties.c +++ b/src/sbus/sssd_dbus_properties.c @@ -1,9 +1,12 @@ /* Authors: Stef Walter stefw@redhat.com
Pavel Březina <pbrezina@redhat.com>Copyright (C) 2014 Red Hat
SBUS: Interface introspection
Wrong comment :-)
- This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation; either version 3 of the License, or
@@ -18,11 +21,302 @@ along with this program. If not, see http://www.gnu.org/licenses/. */
+#include "config.h"
#include "util/util.h" #include "sbus/sssd_dbus.h" #include "sbus/sssd_dbus_meta.h" #include "sbus/sssd_dbus_private.h"
+#define CHECK_SIGNATURE(req, error, label, exp) do { \
- const char *__sig; \
- __sig = dbus_message_get_signature(req->message); \
- if (strcmp(__sig, exp) != 0) { \
error = sbus_error_new(req, DBUS_ERROR_INVALID_ARGS, \"Invalid arguments: expected \"%s\", got \"%s\"", exp, __sig); \goto label; \
Another flow-changing macro...I'm fine with wrappering this in a function, but please don't jump from macros..
- } \
+} while (0)
The rest looks good.
On Fri, Jan 16, 2015 at 02:06:20PM +0100, 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:
I realized that I forgot to add support for having both direct object path and fallback defined. I.e.:
/org/freedesktop/sssd/infopipe /org/freedesktop/sssd/infopipe/*
And one more time. While working on some improvements on ifp and sbus code I found out that there are some copy & paste relics in the last patch. Now they are gone.
There are some Coverity warnings: http://cov01.lab.eng.brq.redhat.com/covscanhub/task/16341/log/fixed.err
Sorry about sending an internal link. I'll paste the warnings instead: Error: DEADCODE (CWE-561): sssd-1.12.90/src/sbus/sssd_dbus_connection.c:515: string_non_null: String literal ""org.freedesktop.DBus.Error.NoMemory"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:515: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NoMemory"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:507: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:507: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:527: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:527: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:538: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:538: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:501: string_non_null: String literal ""org.freedesktop.DBus.Error.UnknownMethod"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:501: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.UnknownMethod"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: notnull: At condition "dbus_error", the value of "dbus_error" cannot be "NULL". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: dead_error_condition: The condition "dbus_error" must be true. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: dead_error_line: Execution cannot reach the expression ""org.freedesktop.DBus.Error.Failed"" inside this statement: "reply = dbus_message_new_er...".
CI passed: http://sssd-ci.duckdns.org/logs/commit/38/75be1d2555a53b6df26d602324ccc431a5...
On 01/16/2015 02:06 PM, Jakub Hrozek wrote:
+#define CHECK_SIGNATURE(req, error, label, exp) do { \
- const char *__sig; \
- __sig = dbus_message_get_signature(req->message); \
- if (strcmp(__sig, exp) != 0) { \
error = sbus_error_new(req, DBUS_ERROR_INVALID_ARGS, \"Invalid arguments: expected \"%s\", got \"%s\"", exp, __sig); \goto label; \Another flow-changing macro...I'm fine with wrappering this in a function, but please don't jump from macros..
It's funny that you don't like it, since I learned it from you :-)
The whole point of those macros is to move error checking out of the function. If you put the code inside a function you still have to test the result in the caller so it doesn't make any sense.
I can get rid of CHECK_SIGNATURE since it is used on three places only anyway. But WRITE_DATA makes the code more readable. As you said yourself, fprintf is not really expected to fail. This way it is possible to focus on the flow of the generator itself instead of seeing four lines that are not supposed to be executed ever after each fprintf.
This is IMHO much better: WRITE_DATA(file, ret, done, FMT_METHOD_NOARG, method->name); WRITE_DATA(file, ret, done, FMT_METHOD, method->name); WRITE_DATA(file, ret, done, FMT_METHOD_CLOSE);
instead of (copied from expanded macro): ret = fprintf(file, " <method name="%s" />\n", method->name); if (ret < 0) { ret = 5; goto done; }
ret = fprintf(file, " <method name="%s">\n", method->name); if (ret < 0) { ret = 5; goto done; }
ret = fprintf(file, " </method>\n"); if (ret < 0) { ret = 5; goto done; }
On Fri, Jan 16, 2015 at 02:25:39PM +0100, Pavel Březina wrote:
On 01/16/2015 02:06 PM, Jakub Hrozek wrote:
+#define CHECK_SIGNATURE(req, error, label, exp) do { \
- const char *__sig; \
- __sig = dbus_message_get_signature(req->message); \
- if (strcmp(__sig, exp) != 0) { \
error = sbus_error_new(req, DBUS_ERROR_INVALID_ARGS, \"Invalid arguments: expected \"%s\", got \"%s\"", exp, __sig); \goto label; \Another flow-changing macro...I'm fine with wrappering this in a function, but please don't jump from macros..
It's funny that you don't like it, since I learned it from you :-)
Damn it, past Jakub :-)
In general, macros that change the flow are really hard to debug and what's even worse is that it might not be obvious that the macro can also jump.
The whole point of those macros is to move error checking out of the function. If you put the code inside a function you still have to test the result in the caller so it doesn't make any sense.
I can get rid of CHECK_SIGNATURE since it is used on three places only anyway. But WRITE_DATA makes the code more readable. As you said yourself, fprintf is not really expected to fail. This way it is possible to focus on the flow of the generator itself instead of seeing four lines that are not supposed to be executed ever after each fprintf.
This is IMHO much better: WRITE_DATA(file, ret, done, FMT_METHOD_NOARG, method->name); WRITE_DATA(file, ret, done, FMT_METHOD, method->name); WRITE_DATA(file, ret, done, FMT_METHOD_CLOSE);
OK, in this single occurence, let's allow the jump in the macro, but please rename the macro so that it's clear that it can change the flow of the program. Maybe something like "WRITE_OR_FAIL".
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@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@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@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@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@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@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@redhat.com
Pavel Březina <pbrezina@redhat.com> Copyright (C) 2014 Red HatSBUS: Interface introspection
Wrong comment :-)
Ok :)
Thank you for the review!
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@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@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@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@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@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
~~~ typoWill 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@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.
On 01/19/2015 10:50 PM, Jakub Hrozek wrote:
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() ?
[...]
How about sbus_opath_parent_subtree()?
On Tue, Jan 20, 2015 at 10:03:16AM +0100, Pavel Březina wrote:
On 01/19/2015 10:50 PM, Jakub Hrozek wrote:
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() ?
[...]
How about sbus_opath_parent_subtree()?
Sounds good to me.
On 01/20/2015 12:05 PM, Jakub Hrozek wrote:
On Tue, Jan 20, 2015 at 10:03:16AM +0100, Pavel Březina wrote:
On 01/19/2015 10:50 PM, Jakub Hrozek wrote:
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() ?
[...]
How about sbus_opath_parent_subtree()?
Sounds good to me.
Here it is. I hope I addressed everything but the review mail was quite huge so it is possible I have missed something.
On 01/20/2015 02:07 PM, Pavel Březina wrote:
On 01/20/2015 12:05 PM, Jakub Hrozek wrote:
On Tue, Jan 20, 2015 at 10:03:16AM +0100, Pavel Březina wrote:
On 01/19/2015 10:50 PM, Jakub Hrozek wrote:
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() ?
[...]
How about sbus_opath_parent_subtree()?
Sounds good to me.
Here it is. I hope I addressed everything but the review mail was quite huge so it is possible I have missed something.
I'm sending the patch set one more time since a forgot to rename pvt to handler_data as requested. Also 8th patch did not contain latest changes since I left them unstaged.
On Tue, Jan 20, 2015 at 09:47:39PM +0100, Pavel Březina wrote:
On 01/20/2015 02:07 PM, Pavel Březina wrote:
On 01/20/2015 12:05 PM, Jakub Hrozek wrote:
On Tue, Jan 20, 2015 at 10:03:16AM +0100, Pavel Březina wrote:
On 01/19/2015 10:50 PM, Jakub Hrozek wrote:
> >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() ?
[...]
How about sbus_opath_parent_subtree()?
Sounds good to me.
Here it is. I hope I addressed everything but the review mail was quite huge so it is possible I have missed something.
I'm sending the patch set one more time since a forgot to rename pvt to handler_data as requested. Also 8th patch did not contain latest changes since I left them unstaged.
Thank you, all my changes were addressed. ACK to all, pending Coverity and CI scans which are queued.
My testing consisted of several ordinary operation, testing that a service that received SEGV is restarted and testing that a service that was restarted due to a timeout works fine.
On Wed, Jan 21, 2015 at 01:19:03PM +0100, Jakub Hrozek wrote:
On Tue, Jan 20, 2015 at 09:47:39PM +0100, Pavel Březina wrote:
On 01/20/2015 02:07 PM, Pavel Březina wrote:
On 01/20/2015 12:05 PM, Jakub Hrozek wrote:
On Tue, Jan 20, 2015 at 10:03:16AM +0100, Pavel Březina wrote:
On 01/19/2015 10:50 PM, Jakub Hrozek wrote:
>> >>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() ?
[...]
How about sbus_opath_parent_subtree()?
Sounds good to me.
Here it is. I hope I addressed everything but the review mail was quite huge so it is possible I have missed something.
I'm sending the patch set one more time since a forgot to rename pvt to handler_data as requested. Also 8th patch did not contain latest changes since I left them unstaged.
Thank you, all my changes were addressed. ACK to all, pending Coverity and CI scans which are queued.
My testing consisted of several ordinary operation, testing that a service that received SEGV is restarted and testing that a service that was restarted due to a timeout works fine.
Sorry, but there are some Coverity warnings: sssd-1.12.90/src/sbus/sssd_dbus_connection.c:515: string_non_null: String literal ""org.freedesktop.DBus.Error.NoMemory"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:515: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NoMemory"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:507: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:507: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:527: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:527: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:538: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:538: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:501: string_non_null: String literal ""org.freedesktop.DBus.Error.UnknownMethod"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:501: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.UnknownMethod"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: notnull: At condition "dbus_error", the value of "dbus_error" cannot be "NULL". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: dead_error_condition: The condition "dbus_error" must be true. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: dead_error_line: Execution cannot reach the expression ""org.freedesktop.DBus.Error.Failed"" inside this statement: "reply = dbus_message_new_er...".
feel free to ping me if you need the full log (it's stored on a rh-internal machine, so not suitable for public ML)
On Wed, Jan 21, 2015 at 09:03:43PM +0100, Jakub Hrozek wrote:
On Wed, Jan 21, 2015 at 01:19:03PM +0100, Jakub Hrozek wrote:
On Tue, Jan 20, 2015 at 09:47:39PM +0100, Pavel Březina wrote:
On 01/20/2015 02:07 PM, Pavel Březina wrote:
On 01/20/2015 12:05 PM, Jakub Hrozek wrote:
On Tue, Jan 20, 2015 at 10:03:16AM +0100, Pavel Březina wrote:
On 01/19/2015 10:50 PM, Jakub Hrozek wrote: >>> >>>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() ? > >[...]
How about sbus_opath_parent_subtree()?
Sounds good to me.
Here it is. I hope I addressed everything but the review mail was quite huge so it is possible I have missed something.
I'm sending the patch set one more time since a forgot to rename pvt to handler_data as requested. Also 8th patch did not contain latest changes since I left them unstaged.
Thank you, all my changes were addressed. ACK to all, pending Coverity and CI scans which are queued.
My testing consisted of several ordinary operation, testing that a service that received SEGV is restarted and testing that a service that was restarted due to a timeout works fine.
Sorry, but there are some Coverity warnings: sssd-1.12.90/src/sbus/sssd_dbus_connection.c:515: string_non_null: String literal ""org.freedesktop.DBus.Error.NoMemory"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:515: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NoMemory"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:507: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:507: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:527: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:527: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:538: string_non_null: String literal ""org.freedesktop.DBus.Error.NotSupported"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:538: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.NotSupported"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:501: string_non_null: String literal ""org.freedesktop.DBus.Error.UnknownMethod"" is never null. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:501: assignment: Assigning: "dbus_error" = ""org.freedesktop.DBus.Error.UnknownMethod"". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: notnull: At condition "dbus_error", the value of "dbus_error" cannot be "NULL". sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: dead_error_condition: The condition "dbus_error" must be true. sssd-1.12.90/src/sbus/sssd_dbus_connection.c:562: dead_error_line: Execution cannot reach the expression ""org.freedesktop.DBus.Error.Failed"" inside this statement: "reply = dbus_message_new_er...".
feel free to ping me if you need the full log (it's stored on a rh-internal machine, so not suitable for public ML)
Mea culpa, the warnings were actually from the /old/ RPMs. I managed to reverse the baseline and RPM under test again. Sorry.
The patches are ACKed.
On Fri, Jan 23, 2015 at 09:28:30PM +0100, Jakub Hrozek wrote:
The patches are ACKed.
* 66277b21179d95f6e96abed01a20ccbccf27ce99 * b742179ac0790068380618ab72a06af18544f09c * 80d0bd38268c02fd32f62b02ae59f19229ca1a79 * 21e05273eed8cc914871938061554589883e67ce * 71c9027d4192bf149afa4fcf9fef93bf6e901121 * 46ee931314e6a5517f5c6b6b14f759364be119cc * 894f09f146f0c9cda9e0f7dfe1916519d73dde72 * d87e960c17d7598781cf032d06ba03a3ecadbfa2 * 9fa95168d80beba04b333b06edc492ecb8b085a1
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina