* [RFC PATCH 2/8] dbus: Separate interface registration from instantiation
2016-01-23 2:59 [RFC PATCH 1/8] dbus: setters and getters API for properties Andrew Zaborowski
@ 2016-01-23 2:59 ` Andrew Zaborowski
2016-01-26 21:13 ` Denis Kenzior
2016-01-23 2:59 ` [RFC PATCH 3/8] dbus: Don't show Introspectable on intermediate nodes Andrew Zaborowski
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Andrew Zaborowski @ 2016-01-23 2:59 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 20385 bytes --]
Currently l_dbus_register_interface performs three related actions:
* if needed, creates a new object node in the dbus object tree
* if needed, sets up internal structs for the new interface
* adds the interface to the object
With this patch these are three spearate calls, although the first
is still performed automatically by l_dbus_add_interface if
l_dbus_register_object wasn't called first. This is in preparation for
ObjectManager support. With this the setup_func parameter and new
interface parameters don't need to be passed every time an interface is
instiated, only when it's being registered/created.
One thing that may be unclear is that as a side effect
_dbus_object_tree_prune_node takes not only the reference to the node
being pruned but also to the object tree and a path to the object. The
latter two are needed because when the function is walking the tree
upwards to free any intermediate nodes that no longer have any
dbus-visible objects in their subtree, the function needs to be able to
check if these nodes are visible on dbus (i.e. are in tree->objects). I
found that currently if you've got a tree with objects /a and /a/b in
it, and remove all interfaces from /a first, then from /a/b, the
_dbus_object_tree_prune_node call will free /a without removing it from
tree->objects. The node reference parameter is no longer needed except
for the unit/test-dbus-service to be able to test adding and removing
intermediate nodes from the tree.
Note that while the client doesn't need to call l_dbus_register_object,
they still need to call l_dbus_unregister_object to free the object
because it's not freed automatically when the last interface gets
removed. But they can skip the l_dbus_remove_interface calls
because the interfaces will be removed either way.
---
ell/dbus-private.h | 20 ++++-
ell/dbus-service.c | 224 +++++++++++++++++++++++++++++++++++------------
ell/dbus.c | 91 +++++++++++++++++--
ell/dbus.h | 19 ++--
examples/dbus-service.c | 13 ++-
unit/test-dbus-service.c | 17 ++--
unit/test-kdbus.c | 9 +-
7 files changed, 311 insertions(+), 82 deletions(-)
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index e60d09e..fc26aa2 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -167,13 +167,27 @@ struct object_node *_dbus_object_tree_makepath(struct _dbus_object_tree *tree,
const char *path);
struct object_node *_dbus_object_tree_lookup(struct _dbus_object_tree *tree,
const char *path);
-void _dbus_object_tree_prune_node(struct object_node *node);
+
+struct object_node *_dbus_object_tree_new_object(struct _dbus_object_tree *tree,
+ const char *path,
+ void *user_data,
+ void (*destroy) (void *));
+bool _dbus_object_tree_prune_node(struct _dbus_object_tree *tree,
+ struct object_node *node,
+ const char *path);
bool _dbus_object_tree_register(struct _dbus_object_tree *tree,
- const char *path, const char *interface,
+ const char *interface,
void (*setup_func)(struct l_dbus_interface *),
- void *user_data, void (*destroy) (void *));
+ void (*destroy) (void *),
+ bool old_style_properties);
bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
+ const char *interface);
+
+bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
+ const char *path, const char *interface,
+ void *user_data);
+bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
const char *path,
const char *interface);
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 77310c6..d37989e 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -72,6 +72,9 @@ struct l_dbus_interface {
struct l_queue *methods;
struct l_queue *signals;
struct l_queue *properties;
+ bool handle_old_style_properties;
+ void (*setup_func)(struct l_dbus_interface *);
+ void (*instance_destroy)(void *);
char name[];
};
@@ -84,13 +87,14 @@ struct child_node {
struct interface_instance {
struct l_dbus_interface *interface;
void *user_data;
- void (*user_destroy) (void *);
};
struct object_node {
struct object_node *parent;
struct l_queue *instances;
struct child_node *children;
+ void *user_data;
+ void (*destroy) (void *);
};
struct _dbus_object_tree {
@@ -489,8 +493,8 @@ struct _dbus_property *_dbus_interface_find_property(struct l_dbus_interface *i,
static void interface_instance_free(struct interface_instance *instance)
{
- if (instance->user_destroy)
- instance->user_destroy(instance->user_data);
+ if (instance->interface->instance_destroy)
+ instance->interface->instance_destroy(instance->user_data);
l_free(instance);
}
@@ -539,6 +543,9 @@ static void subtree_free(struct object_node *node)
l_queue_destroy(node->instances,
(l_queue_destroy_func_t) interface_instance_free);
+ if (node->destroy)
+ node->destroy(node->user_data);
+
l_free(node);
}
@@ -626,81 +633,197 @@ struct object_node *_dbus_object_tree_lookup(struct _dbus_object_tree *tree,
return lookup_recurse(tree->root, path);
}
-void _dbus_object_tree_prune_node(struct object_node *node)
+struct object_node *_dbus_object_tree_new_object(struct _dbus_object_tree *tree,
+ const char *path,
+ void *user_data,
+ void (*destroy) (void *))
+{
+ struct object_node *node;
+
+ if (!_dbus_valid_object_path(path))
+ return NULL;
+
+ if (l_hashmap_lookup(tree->objects, path))
+ return NULL;
+
+ node = _dbus_object_tree_makepath(tree, path);
+ node->user_data = user_data;
+ node->destroy = destroy;
+
+ l_hashmap_insert(tree->objects, path, node);
+
+ return node;
+}
+
+bool _dbus_object_tree_prune_node(struct _dbus_object_tree *tree,
+ struct object_node *node,
+ const char *path)
{
- struct object_node *parent = node->parent;
- struct child_node *p = NULL, *c;
+ struct object_node *parent;
+ struct child_node *p, *c;
+ struct interface_instance *instance;
+ char parentpath[strlen(path) + 1];
+
+ if (!node) {
+ node = l_hashmap_remove(tree->objects, path);
+ if (!node)
+ return false;
+ }
+
+ while ((instance = l_queue_pop_head(node->instances)))
+ interface_instance_free(instance);
+
+ if (node->destroy) {
+ node->destroy(node->user_data);
- while (parent) {
- for (c = parent->children, p = NULL; c; p = c, c = c->next) {
- if (c->node != node)
- continue;
+ node->destroy = NULL;
+ }
- if (p)
- p->next = c->next;
- else
- parent->children = c->next;
+ if (node->children || !node->parent)
+ return true;
- subtree_free(c->node);
- l_free(c);
+ /*
+ * Walk up the tree until a node that either has more than one
+ * child, is the root or is in the objects hashmap.
+ */
+ strcpy(parentpath, path);
+ while (true) {
+ parent = node->parent;
+
+ if (parent == tree->root)
+ break;
+
+ if (parent->children->next)
break;
- }
- if (parent->children != NULL)
- return;
+ /* Parent's path */
+ parentpath[strlen(parentpath) -
+ strlen(parent->children->subpath) - 1] = '\0';
+
+ if (l_hashmap_lookup(tree->objects, parentpath))
+ break;
node = parent;
- parent = node->parent;
}
+
+ for (c = parent->children, p = NULL; c; p = c, c = c->next)
+ if (c->node == node)
+ break;
+
+ if (p)
+ p->next = c->next;
+ else
+ parent->children = c->next;
+
+ l_free(c);
+ subtree_free(node);
+
+ return true;
}
bool _dbus_object_tree_register(struct _dbus_object_tree *tree,
- const char *path, const char *interface,
+ const char *interface,
void (*setup_func)(struct l_dbus_interface *),
- void *user_data, void (*destroy) (void *))
+ void (*destroy) (void *),
+ bool old_style_properties)
{
- struct object_node *object;
struct l_dbus_interface *dbi;
- const struct l_queue_entry *entry;
- struct interface_instance *instance;
if (!_dbus_valid_interface(interface))
return false;
- if (!_dbus_valid_object_path(path))
+ /*
+ * Check to make sure we do not have this interface already
+ * registered
+ */
+ dbi = l_hashmap_lookup(tree->interfaces, interface);
+ if (dbi)
+ return false;
+
+ dbi = _dbus_interface_new(interface);
+ dbi->instance_destroy = destroy;
+ dbi->setup_func = setup_func;
+ dbi->handle_old_style_properties = old_style_properties;
+
+ l_hashmap_insert(tree->interfaces, dbi->name, dbi);
+
+ return true;
+}
+
+struct interface_check {
+ const char *interface;
+ bool result;
+};
+
+static void check_interface_used(const void *key, void *value, void *user_data)
+{
+ struct object_node *node = value;
+ struct interface_check *state = user_data;
+
+ if (l_queue_find(node->instances, match_interface_instance,
+ (char *) state->interface))
+ state->result = true;
+}
+
+bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
+ const char *interface_name)
+{
+ struct l_dbus_interface *interface;
+ struct interface_check state = { interface_name, false };
+
+ interface = l_hashmap_lookup(tree->interfaces, interface_name);
+ if (!interface)
+ return false;
+
+ /* Check that the interface is not in use */
+ l_hashmap_foreach(tree->objects, check_interface_used, &state);
+ if (state.result)
+ return false;
+
+ l_hashmap_remove(tree->interfaces, interface_name);
+
+ _dbus_interface_free(interface);
+
+ return true;
+}
+
+bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
+ const char *path, const char *interface,
+ void *user_data)
+{
+ struct object_node *object;
+ struct l_dbus_interface *dbi;
+ struct interface_instance *instance;
+
+ dbi = l_hashmap_lookup(tree->interfaces, interface);
+ if (!dbi)
return false;
object = l_hashmap_lookup(tree->objects, path);
if (!object) {
- object = _dbus_object_tree_makepath(tree, path);
- l_hashmap_insert(tree->objects, path, object);
+ object = _dbus_object_tree_new_object(tree, path, NULL, NULL);
+
+ if (!object)
+ return false;
}
/*
* Check to make sure we do not have this interface already
* registered for this object
*/
- entry = l_queue_get_entries(object->instances);
- while (entry) {
- instance = entry->data;
-
- if (!strcmp(instance->interface->name, interface))
- return false;
+ if (l_queue_find(object->instances, match_interface_instance,
+ (char *) interface))
+ return false;
- entry = entry->next;
- }
+ if (dbi->setup_func) {
+ dbi->setup_func(dbi);
- dbi = l_hashmap_lookup(tree->interfaces, interface);
- if (!dbi) {
- dbi = _dbus_interface_new(interface);
- setup_func(dbi);
- l_hashmap_insert(tree->interfaces, dbi->name, dbi);
+ dbi->setup_func = NULL;
}
instance = l_new(struct interface_instance, 1);
instance->interface = dbi;
- instance->user_destroy = destroy;
instance->user_data = user_data;
if (!object->instances)
@@ -711,13 +834,12 @@ bool _dbus_object_tree_register(struct _dbus_object_tree *tree,
return true;
}
-bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
+bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
const char *path,
const char *interface)
{
struct object_node *node;
struct interface_instance *instance;
- bool r;
node = l_hashmap_lookup(tree->objects, path);
if (!node)
@@ -725,18 +847,12 @@ bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
instance = l_queue_remove_if(node->instances,
match_interface_instance, (char *) interface);
+ if (!instance)
+ return false;
- r = instance ? true : false;
-
- if (instance)
- interface_instance_free(instance);
-
- if (l_queue_isempty(node->instances) && !node->children) {
- l_hashmap_remove(tree->objects, path);
- _dbus_object_tree_prune_node(node);
- }
+ interface_instance_free(instance);
- return r;
+ return true;
}
static void generate_interface_instance(void *data, void *user)
diff --git a/ell/dbus.c b/ell/dbus.c
index 1d9aed5..a9bb314 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -1215,10 +1215,10 @@ int _dbus_get_fd(struct l_dbus *dbus)
}
LIB_EXPORT bool l_dbus_register_interface(struct l_dbus *dbus,
- const char *path, const char *interface,
+ const char *interface,
l_dbus_interface_setup_func_t setup_func,
- void *user_data,
- l_dbus_destroy_func_t destroy)
+ l_dbus_destroy_func_t destroy,
+ bool handle_old_style_properties)
{
if (unlikely(!dbus))
return false;
@@ -1226,12 +1226,12 @@ LIB_EXPORT bool l_dbus_register_interface(struct l_dbus *dbus,
if (unlikely(!dbus->tree))
return false;
- return _dbus_object_tree_register(dbus->tree, path, interface,
- setup_func, user_data, destroy);
+ return _dbus_object_tree_register(dbus->tree, interface,
+ setup_func, destroy,
+ handle_old_style_properties);
}
LIB_EXPORT bool l_dbus_unregister_interface(struct l_dbus *dbus,
- const char *path,
const char *interface)
{
if (unlikely(!dbus))
@@ -1240,7 +1240,84 @@ LIB_EXPORT bool l_dbus_unregister_interface(struct l_dbus *dbus,
if (unlikely(!dbus->tree))
return false;
- return _dbus_object_tree_unregister(dbus->tree, path, interface);
+ return _dbus_object_tree_unregister(dbus->tree, interface);
+}
+
+LIB_EXPORT bool l_dbus_register_object(struct l_dbus *dbus, const char *path,
+ void *user_data,
+ l_dbus_destroy_func_t destroy, ...)
+{
+ va_list args;
+ const char *interface;
+ void *if_user_data;
+ bool r = true;;
+
+ if (unlikely(!dbus))
+ return false;
+
+ if (unlikely(!dbus->tree))
+ return false;
+
+ if (!_dbus_object_tree_new_object(dbus->tree, path, user_data, destroy))
+ return false;
+
+ va_start(args, destroy);
+ while ((interface = va_arg(args, const char *))) {
+ if_user_data = va_arg(args, void *);
+
+ if (!_dbus_object_tree_add_interface(dbus->tree, path,
+ interface,
+ if_user_data)) {
+ _dbus_object_tree_prune_node(dbus->tree, NULL, path);
+ r = false;
+
+ break;
+ }
+ }
+ va_end(args);
+
+ return r;
+}
+
+LIB_EXPORT bool l_dbus_unregister_object(struct l_dbus *dbus,
+ const char *object)
+{
+ if (unlikely(!dbus))
+ return false;
+
+ if (unlikely(!dbus->tree))
+ return false;
+
+ return _dbus_object_tree_prune_node(dbus->tree, NULL, object);
+}
+
+LIB_EXPORT bool l_dbus_object_add_interface(struct l_dbus *dbus,
+ const char *object,
+ const char *interface,
+ void *user_data)
+{
+ if (unlikely(!dbus))
+ return false;
+
+ if (unlikely(!dbus->tree))
+ return false;
+
+ return _dbus_object_tree_add_interface(dbus->tree, object, interface,
+ user_data);
+}
+
+LIB_EXPORT bool l_dbus_object_remove_interface(struct l_dbus *dbus,
+ const char *object,
+ const char *interface)
+{
+ if (unlikely(!dbus))
+ return false;
+
+ if (unlikely(!dbus->tree))
+ return false;
+
+ return _dbus_object_tree_remove_interface(dbus->tree, object,
+ interface);
}
void _dbus1_filter_format_match(struct dbus1_filter_data *data, char *rule,
diff --git a/ell/dbus.h b/ell/dbus.h
index 39f926b..011d59a 100644
--- a/ell/dbus.h
+++ b/ell/dbus.h
@@ -191,13 +191,22 @@ bool l_dbus_message_builder_leave_variant(
struct l_dbus_message *l_dbus_message_builder_finalize(
struct l_dbus_message_builder *builder);
-bool l_dbus_register_interface(struct l_dbus *dbus,
- const char *path, const char *interface,
+bool l_dbus_register_interface(struct l_dbus *dbus, const char *interface,
l_dbus_interface_setup_func_t setup_func,
- void *user_data,
- l_dbus_destroy_func_t destroy);
-bool l_dbus_unregister_interface(struct l_dbus *dbus, const char *path,
+ l_dbus_destroy_func_t destroy,
+ bool handle_old_style_properties);
+bool l_dbus_unregister_interface(struct l_dbus *dbus, const char *interface);
+
+bool l_dbus_register_object(struct l_dbus *dbus, const char *path,
+ void *user_data, l_dbus_destroy_func_t destroy,
+ ...);
+bool l_dbus_unregister_object(struct l_dbus *dbus, const char *object);
+
+bool l_dbus_object_add_interface(struct l_dbus *dbus, const char *object,
+ const char *interface, void *user_data);
+bool l_dbus_object_remove_interface(struct l_dbus *dbus, const char *object,
const char *interface);
+
unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
const char *name,
l_dbus_watch_func_t disconnect_func,
diff --git a/examples/dbus-service.c b/examples/dbus-service.c
index e4e5fa7..488cc47 100644
--- a/examples/dbus-service.c
+++ b/examples/dbus-service.c
@@ -307,17 +307,22 @@ int main(int argc, char *argv[])
test->string = l_strdup("Default");
test->integer = 42;
- if (!l_dbus_register_interface(dbus, "/test", "org.test",
- setup_test_interface, test,
- test_data_destroy)) {
+ if (!l_dbus_register_interface(dbus, "org.test", setup_test_interface,
+ test_data_destroy, false)) {
l_info("Unable to register interface");
test_data_destroy(test);
goto cleanup;
}
+ if (!l_dbus_object_add_interface(dbus, "/test", "org.test", test)) {
+ l_info("Unable to instantiate interface");
+ test_data_destroy(test);
+ goto cleanup;
+ }
+
l_main_run();
- l_dbus_unregister_interface(dbus, "/test", "org.test");
+ l_dbus_unregister_object(dbus, "/test");
cleanup:
l_dbus_destroy(dbus);
diff --git a/unit/test-dbus-service.c b/unit/test-dbus-service.c
index d0bf174..16fc00e 100644
--- a/unit/test-dbus-service.c
+++ b/unit/test-dbus-service.c
@@ -205,16 +205,16 @@ static void test_dbus_object_tree(const void *test_data)
tmp = _dbus_object_tree_lookup(tree, "/foo/bee");
assert(tmp);
- _dbus_object_tree_prune_node(leaf3);
+ _dbus_object_tree_prune_node(tree, leaf3, "/foo/bee/boo");
tmp = _dbus_object_tree_lookup(tree, "/foo/bee");
assert(!tmp);
tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
assert(tmp);
- _dbus_object_tree_prune_node(leaf2);
+ _dbus_object_tree_prune_node(tree, leaf2, "/foo/bar/ble");
tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
assert(tmp);
- _dbus_object_tree_prune_node(leaf1);
+ _dbus_object_tree_prune_node(tree, leaf1, "/foo/bar/baz");
tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
assert(!tmp);
tmp = _dbus_object_tree_lookup(tree, "/foo");
@@ -276,9 +276,10 @@ static void test_dbus_object_tree_introspection(const void *test_data)
tree = _dbus_object_tree_new();
- _dbus_object_tree_register(tree, "/", "org.ofono.Manager",
+ _dbus_object_tree_register(tree, "org.ofono.Manager",
build_manager_interface,
- NULL, NULL);
+ NULL, false);
+ _dbus_object_tree_add_interface(tree, "/", "org.ofono.Manager", NULL);
_dbus_object_tree_makepath(tree, "/phonesim");
@@ -298,9 +299,11 @@ static void test_dbus_object_tree_dispatch(const void *test_data)
tree = _dbus_object_tree_new();
- _dbus_object_tree_register(tree, "/", "org.ofono.Manager",
+ _dbus_object_tree_register(tree, "org.ofono.Manager",
build_manager_interface,
- dummy_data, NULL);
+ NULL, false);
+ _dbus_object_tree_add_interface(tree, "/", "org.ofono.Manager",
+ dummy_data);
message = _dbus_message_new_method_call(1, "org.ofono", "/",
"org.ofono.Manager",
diff --git a/unit/test-kdbus.c b/unit/test-kdbus.c
index 4bf41bb..3a6af3e 100644
--- a/unit/test-kdbus.c
+++ b/unit/test-kdbus.c
@@ -158,12 +158,17 @@ int main(int argc, char *argv[])
l_dbus_set_ready_handler(service, service_ready_callback,
service, NULL);
- if (!l_dbus_register_interface(service, "/test", "org.test",
- setup_test_interface, NULL, NULL)) {
+ if (!l_dbus_register_interface(service, "org.test",
+ setup_test_interface, NULL, false)) {
l_info("Unable to register interface");
goto error;
}
+ if (!l_dbus_object_add_interface(service, "/test", "org.test", NULL)) {
+ l_info("Unable to instantiate interface");
+ goto error;
+ }
+
client = l_dbus_new(bus_address);
assert(client);
--
2.5.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [RFC PATCH 2/8] dbus: Separate interface registration from instantiation
2016-01-23 2:59 ` [RFC PATCH 2/8] dbus: Separate interface registration from instantiation Andrew Zaborowski
@ 2016-01-26 21:13 ` Denis Kenzior
2016-01-26 23:40 ` Andrzej Zaborowski
0 siblings, 1 reply; 18+ messages in thread
From: Denis Kenzior @ 2016-01-26 21:13 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 18615 bytes --]
Hi Andrew,
On 01/22/2016 08:59 PM, Andrew Zaborowski wrote:
> Currently l_dbus_register_interface performs three related actions:
> * if needed, creates a new object node in the dbus object tree
> * if needed, sets up internal structs for the new interface
> * adds the interface to the object
>
> With this patch these are three spearate calls, although the first
> is still performed automatically by l_dbus_add_interface if
> l_dbus_register_object wasn't called first. This is in preparation for
> ObjectManager support. With this the setup_func parameter and new
> interface parameters don't need to be passed every time an interface is
> instiated, only when it's being registered/created.
>
> One thing that may be unclear is that as a side effect
> _dbus_object_tree_prune_node takes not only the reference to the node
> being pruned but also to the object tree and a path to the object. The
> latter two are needed because when the function is walking the tree
> upwards to free any intermediate nodes that no longer have any
> dbus-visible objects in their subtree, the function needs to be able to
> check if these nodes are visible on dbus (i.e. are in tree->objects). I
> found that currently if you've got a tree with objects /a and /a/b in
> it, and remove all interfaces from /a first, then from /a/b, the
> _dbus_object_tree_prune_node call will free /a without removing it from
> tree->objects. The node reference parameter is no longer needed except
> for the unit/test-dbus-service to be able to test adding and removing
> intermediate nodes from the tree.
If we register object on '/a', then register object on '/a/b'. We have
two objects in the tree->objects hashmap. If we unregister object '/a'
first, then tree->objects will no longer contain '/a'. Then
unregistering '/a/b' will prune the tree upwards.
Tree pruning is intended to be fully separate from the object hash, so
I'm really not following.
Perhaps you're conflating unregistering all interfaces vs unregistering
the object? But then you mention that your semantics of not removing
the object from the tree if all interfaces are removed is intentional in
the next paragraph.
>
> Note that while the client doesn't need to call l_dbus_register_object,
> they still need to call l_dbus_unregister_object to free the object
> because it's not freed automatically when the last interface gets
> removed. But they can skip the l_dbus_remove_interface calls
> because the interfaces will be removed either way.
> ---
> ell/dbus-private.h | 20 ++++-
> ell/dbus-service.c | 224 +++++++++++++++++++++++++++++++++++------------
> ell/dbus.c | 91 +++++++++++++++++--
> ell/dbus.h | 19 ++--
> examples/dbus-service.c | 13 ++-
> unit/test-dbus-service.c | 17 ++--
> unit/test-kdbus.c | 9 +-
> 7 files changed, 311 insertions(+), 82 deletions(-)
>
> diff --git a/ell/dbus-private.h b/ell/dbus-private.h
> index e60d09e..fc26aa2 100644
> --- a/ell/dbus-private.h
> +++ b/ell/dbus-private.h
> @@ -167,13 +167,27 @@ struct object_node *_dbus_object_tree_makepath(struct _dbus_object_tree *tree,
> const char *path);
> struct object_node *_dbus_object_tree_lookup(struct _dbus_object_tree *tree,
> const char *path);
> -void _dbus_object_tree_prune_node(struct object_node *node);
> +
> +struct object_node *_dbus_object_tree_new_object(struct _dbus_object_tree *tree,
> + const char *path,
> + void *user_data,
> + void (*destroy) (void *));
> +bool _dbus_object_tree_prune_node(struct _dbus_object_tree *tree,
> + struct object_node *node,
> + const char *path);
>
> bool _dbus_object_tree_register(struct _dbus_object_tree *tree,
> - const char *path, const char *interface,
> + const char *interface,
> void (*setup_func)(struct l_dbus_interface *),
> - void *user_data, void (*destroy) (void *));
> + void (*destroy) (void *),
> + bool old_style_properties);
Should this be called _dbus_object_tree_register_interface now? Since
it only deals with interfaces and not object paths?
> bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
> + const char *interface);
> +
Similarly, should this be _dbus_object_tree_unregister_interface?
> +bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
> + const char *path, const char *interface,
> + void *user_data);
> +bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
> const char *path,
> const char *interface);
>
> diff --git a/ell/dbus-service.c b/ell/dbus-service.c
> index 77310c6..d37989e 100644
> --- a/ell/dbus-service.c
> +++ b/ell/dbus-service.c
> @@ -72,6 +72,9 @@ struct l_dbus_interface {
> struct l_queue *methods;
> struct l_queue *signals;
> struct l_queue *properties;
> + bool handle_old_style_properties;
> + void (*setup_func)(struct l_dbus_interface *);
> + void (*instance_destroy)(void *);
> char name[];
> };
>
> @@ -84,13 +87,14 @@ struct child_node {
> struct interface_instance {
> struct l_dbus_interface *interface;
> void *user_data;
> - void (*user_destroy) (void *);
> };
>
> struct object_node {
> struct object_node *parent;
> struct l_queue *instances;
> struct child_node *children;
> + void *user_data;
> + void (*destroy) (void *);
> };
>
> struct _dbus_object_tree {
> @@ -489,8 +493,8 @@ struct _dbus_property *_dbus_interface_find_property(struct l_dbus_interface *i,
>
> static void interface_instance_free(struct interface_instance *instance)
> {
> - if (instance->user_destroy)
> - instance->user_destroy(instance->user_data);
> + if (instance->interface->instance_destroy)
> + instance->interface->instance_destroy(instance->user_data);
>
> l_free(instance);
> }
> @@ -539,6 +543,9 @@ static void subtree_free(struct object_node *node)
> l_queue_destroy(node->instances,
> (l_queue_destroy_func_t) interface_instance_free);
>
> + if (node->destroy)
> + node->destroy(node->user_data);
> +
> l_free(node);
> }
>
> @@ -626,81 +633,197 @@ struct object_node *_dbus_object_tree_lookup(struct _dbus_object_tree *tree,
> return lookup_recurse(tree->root, path);
> }
>
> -void _dbus_object_tree_prune_node(struct object_node *node)
> +struct object_node *_dbus_object_tree_new_object(struct _dbus_object_tree *tree,
> + const char *path,
> + void *user_data,
> + void (*destroy) (void *))
> +{
> + struct object_node *node;
> +
> + if (!_dbus_valid_object_path(path))
> + return NULL;
> +
> + if (l_hashmap_lookup(tree->objects, path))
> + return NULL;
> +
> + node = _dbus_object_tree_makepath(tree, path);
> + node->user_data = user_data;
> + node->destroy = destroy;
> +
> + l_hashmap_insert(tree->objects, path, node);
> +
> + return node;
> +}
> +
> +bool _dbus_object_tree_prune_node(struct _dbus_object_tree *tree,
> + struct object_node *node,
> + const char *path)
This part sounds like it really belongs in a separate patch...
> {
> - struct object_node *parent = node->parent;
> - struct child_node *p = NULL, *c;
> + struct object_node *parent;
> + struct child_node *p, *c;
> + struct interface_instance *instance;
> + char parentpath[strlen(path) + 1];
> +
> + if (!node) {
> + node = l_hashmap_remove(tree->objects, path);
> + if (!node)
> + return false;
> + }
> +
> + while ((instance = l_queue_pop_head(node->instances)))
> + interface_instance_free(instance);
> +
> + if (node->destroy) {
> + node->destroy(node->user_data);
>
> - while (parent) {
> - for (c = parent->children, p = NULL; c; p = c, c = c->next) {
> - if (c->node != node)
> - continue;
> + node->destroy = NULL;
> + }
>
> - if (p)
> - p->next = c->next;
> - else
> - parent->children = c->next;
> + if (node->children || !node->parent)
> + return true;
>
> - subtree_free(c->node);
> - l_free(c);
> + /*
> + * Walk up the tree until a node that either has more than one
> + * child, is the root or is in the objects hashmap.
> + */
> + strcpy(parentpath, path);
>
> + while (true) {
> + parent = node->parent;
> +
> + if (parent == tree->root)
> + break;
> +
> + if (parent->children->next)
> break;
> - }
>
> - if (parent->children != NULL)
> - return;
> + /* Parent's path */
> + parentpath[strlen(parentpath) -
> + strlen(parent->children->subpath) - 1] = '\0';
> +
> + if (l_hashmap_lookup(tree->objects, parentpath))
> + break;
>
> node = parent;
> - parent = node->parent;
> }
> +
> + for (c = parent->children, p = NULL; c; p = c, c = c->next)
> + if (c->node == node)
> + break;
> +
> + if (p)
> + p->next = c->next;
> + else
> + parent->children = c->next;
> +
> + l_free(c);
> + subtree_free(node);
> +
> + return true;
> }
>
> bool _dbus_object_tree_register(struct _dbus_object_tree *tree,
> - const char *path, const char *interface,
> + const char *interface,
> void (*setup_func)(struct l_dbus_interface *),
> - void *user_data, void (*destroy) (void *))
> + void (*destroy) (void *),
> + bool old_style_properties)
> {
> - struct object_node *object;
> struct l_dbus_interface *dbi;
> - const struct l_queue_entry *entry;
> - struct interface_instance *instance;
>
> if (!_dbus_valid_interface(interface))
> return false;
>
> - if (!_dbus_valid_object_path(path))
> + /*
> + * Check to make sure we do not have this interface already
> + * registered
> + */
> + dbi = l_hashmap_lookup(tree->interfaces, interface);
> + if (dbi)
> + return false;
> +
> + dbi = _dbus_interface_new(interface);
> + dbi->instance_destroy = destroy;
> + dbi->setup_func = setup_func;
Can't we just call this right away and not wait for first use? Let the
programmer decide when to instantiate the interface.
> + dbi->handle_old_style_properties = old_style_properties;
> +
> + l_hashmap_insert(tree->interfaces, dbi->name, dbi);
> +
> + return true;
> +}
> +
> +struct interface_check {
> + const char *interface;
> + bool result;
> +};
> +
> +static void check_interface_used(const void *key, void *value, void *user_data)
> +{
> + struct object_node *node = value;
> + struct interface_check *state = user_data;
> +
> + if (l_queue_find(node->instances, match_interface_instance,
> + (char *) state->interface))
> + state->result = true;
> +}
> +
> +bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
> + const char *interface_name)
> +{
> + struct l_dbus_interface *interface;
> + struct interface_check state = { interface_name, false };
> +
> + interface = l_hashmap_lookup(tree->interfaces, interface_name);
> + if (!interface)
> + return false;
> +
> + /* Check that the interface is not in use */
> + l_hashmap_foreach(tree->objects, check_interface_used, &state);
> + if (state.result)
> + return false;
I'm not sure this is useful in its current form. If we want to have
this, then we probably need to distinguish between telling the
programmer that "you're trying to unregister an unknown interface" vs
"you're trying to unregister a interface that is still being used". As
an alternative, you can keep a reference count on the interface itself
and keep the object around until all instances have been destroyed, and
this function would always succeed...
However, I would keep it simple and just let the programmer control
this. If they try to remove an interface that is still being used,
things will crash spectacularly and valgrind/gdb will find it very fast.
Remember our motto, "crash early, crash often" :)
> +
> + l_hashmap_remove(tree->interfaces, interface_name);
> +
> + _dbus_interface_free(interface);
> +
> + return true;
> +}
> +
> +bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
> + const char *path, const char *interface,
> + void *user_data)
> +{
> + struct object_node *object;
> + struct l_dbus_interface *dbi;
> + struct interface_instance *instance;
> +
> + dbi = l_hashmap_lookup(tree->interfaces, interface);
> + if (!dbi)
> return false;
>
> object = l_hashmap_lookup(tree->objects, path);
> if (!object) {
> - object = _dbus_object_tree_makepath(tree, path);
> - l_hashmap_insert(tree->objects, path, object);
> + object = _dbus_object_tree_new_object(tree, path, NULL, NULL);
> +
> + if (!object)
> + return false;
> }
>
> /*
> * Check to make sure we do not have this interface already
> * registered for this object
> */
> - entry = l_queue_get_entries(object->instances);
> - while (entry) {
> - instance = entry->data;
> -
> - if (!strcmp(instance->interface->name, interface))
> - return false;
> + if (l_queue_find(object->instances, match_interface_instance,
> + (char *) interface))
> + return false;
>
> - entry = entry->next;
> - }
> + if (dbi->setup_func) {
> + dbi->setup_func(dbi);
>
See my comments above...
> - dbi = l_hashmap_lookup(tree->interfaces, interface);
> - if (!dbi) {
> - dbi = _dbus_interface_new(interface);
> - setup_func(dbi);
> - l_hashmap_insert(tree->interfaces, dbi->name, dbi);
> + dbi->setup_func = NULL;
> }
>
> instance = l_new(struct interface_instance, 1);
> instance->interface = dbi;
> - instance->user_destroy = destroy;
> instance->user_data = user_data;
>
> if (!object->instances)
> @@ -711,13 +834,12 @@ bool _dbus_object_tree_register(struct _dbus_object_tree *tree,
> return true;
> }
>
> -bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
> +bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
> const char *path,
> const char *interface)
> {
> struct object_node *node;
> struct interface_instance *instance;
> - bool r;
>
> node = l_hashmap_lookup(tree->objects, path);
> if (!node)
> @@ -725,18 +847,12 @@ bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
>
> instance = l_queue_remove_if(node->instances,
> match_interface_instance, (char *) interface);
> + if (!instance)
> + return false;
>
> - r = instance ? true : false;
> -
> - if (instance)
> - interface_instance_free(instance);
> -
> - if (l_queue_isempty(node->instances) && !node->children) {
> - l_hashmap_remove(tree->objects, path);
> - _dbus_object_tree_prune_node(node);
> - }
> + interface_instance_free(instance);
>
> - return r;
> + return true;
> }
>
> static void generate_interface_instance(void *data, void *user)
> diff --git a/ell/dbus.c b/ell/dbus.c
> index 1d9aed5..a9bb314 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
<snip>
> +
> +LIB_EXPORT bool l_dbus_register_object(struct l_dbus *dbus, const char *path,
> + void *user_data,
> + l_dbus_destroy_func_t destroy, ...)
> +{
We've talked about this before, but this one really needs a
documentation blurb or an additional example of the expected syntax.
e.g. l_dbus_register_object(dbus, "/foo", object, destroy_object,
"FooInterface", foo_data, "BarInterface", bar_data, NULL);
> + va_list args;
> + const char *interface;
> + void *if_user_data;
> + bool r = true;;
> +
> + if (unlikely(!dbus))
> + return false;
> +
> + if (unlikely(!dbus->tree))
> + return false;
> +
> + if (!_dbus_object_tree_new_object(dbus->tree, path, user_data, destroy))
> + return false;
> +
> + va_start(args, destroy);
> + while ((interface = va_arg(args, const char *))) {
> + if_user_data = va_arg(args, void *);
> +
> + if (!_dbus_object_tree_add_interface(dbus->tree, path,
> + interface,
> + if_user_data)) {
> + _dbus_object_tree_prune_node(dbus->tree, NULL, path);
> + r = false;
> +
> + break;
> + }
> + }
> + va_end(args);
> +
> + return r;
> +}
> +
<snip>
> diff --git a/unit/test-dbus-service.c b/unit/test-dbus-service.c
> index d0bf174..16fc00e 100644
> --- a/unit/test-dbus-service.c
> +++ b/unit/test-dbus-service.c
> @@ -205,16 +205,16 @@ static void test_dbus_object_tree(const void *test_data)
>
> tmp = _dbus_object_tree_lookup(tree, "/foo/bee");
> assert(tmp);
> - _dbus_object_tree_prune_node(leaf3);
> + _dbus_object_tree_prune_node(tree, leaf3, "/foo/bee/boo");
> tmp = _dbus_object_tree_lookup(tree, "/foo/bee");
> assert(!tmp);
>
> tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
> assert(tmp);
> - _dbus_object_tree_prune_node(leaf2);
> + _dbus_object_tree_prune_node(tree, leaf2, "/foo/bar/ble");
> tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
> assert(tmp);
> - _dbus_object_tree_prune_node(leaf1);
> + _dbus_object_tree_prune_node(tree, leaf1, "/foo/bar/baz");
> tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
> assert(!tmp);
> tmp = _dbus_object_tree_lookup(tree, "/foo");
> @@ -276,9 +276,10 @@ static void test_dbus_object_tree_introspection(const void *test_data)
>
> tree = _dbus_object_tree_new();
>
> - _dbus_object_tree_register(tree, "/", "org.ofono.Manager",
> + _dbus_object_tree_register(tree, "org.ofono.Manager",
> build_manager_interface,
> - NULL, NULL);
> + NULL, false);
> + _dbus_object_tree_add_interface(tree, "/", "org.ofono.Manager", NULL);
>
> _dbus_object_tree_makepath(tree, "/phonesim");
>
> @@ -298,9 +299,11 @@ static void test_dbus_object_tree_dispatch(const void *test_data)
>
> tree = _dbus_object_tree_new();
>
> - _dbus_object_tree_register(tree, "/", "org.ofono.Manager",
> + _dbus_object_tree_register(tree, "org.ofono.Manager",
> build_manager_interface,
> - dummy_data, NULL);
> + NULL, false);
> + _dbus_object_tree_add_interface(tree, "/", "org.ofono.Manager",
> + dummy_data);
>
> message = _dbus_message_new_method_call(1, "org.ofono", "/",
> "org.ofono.Manager",
<snip>
By the way, applying this patch makes unit tests fail:
denkenz(a)new-host-2 ~/ell $ unit/test-dbus-service
TEST: Test Frobate Introspection
TEST: Test Bazify Introspection
TEST: Test Mogrify Introspection
TEST: Test Changed Introspection
TEST: Test Bar Property Introspection
test-dbus-service: unit/test-dbus-service.c:157:
test_introspect_property: Assertion `!strcmp(test->expected_xml, xml)'
failed.
Aborted
Regards,
-Denis
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [RFC PATCH 2/8] dbus: Separate interface registration from instantiation
2016-01-26 21:13 ` Denis Kenzior
@ 2016-01-26 23:40 ` Andrzej Zaborowski
2016-01-27 0:16 ` Denis Kenzior
0 siblings, 1 reply; 18+ messages in thread
From: Andrzej Zaborowski @ 2016-01-26 23:40 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 23683 bytes --]
w
cuHi Denis,
On 26 January 2016 at 22:13, Denis Kenzior <denkenz@gmail.com> wrote:
> On 01/22/2016 08:59 PM, Andrew Zaborowski wrote:
>> One thing that may be unclear is that as a side effect
>> _dbus_object_tree_prune_node takes not only the reference to the node
>> being pruned but also to the object tree and a path to the object. The
>> latter two are needed because when the function is walking the tree
>> upwards to free any intermediate nodes that no longer have any
>> dbus-visible objects in their subtree, the function needs to be able to
>> check if these nodes are visible on dbus (i.e. are in tree->objects). I
>> found that currently if you've got a tree with objects /a and /a/b in
>> it, and remove all interfaces from /a first, then from /a/b, the
>> _dbus_object_tree_prune_node call will free /a without removing it from
>> tree->objects. The node reference parameter is no longer needed except
>> for the unit/test-dbus-service to be able to test adding and removing
>> intermediate nodes from the tree.
>
>
> If we register object on '/a', then register object on '/a/b'. We have two
> objects in the tree->objects hashmap. If we unregister object '/a' first,
> then tree->objects will no longer contain '/a'.
_dbus_object_tree_unregister currently does:
if (l_queue_isempty(node->instances) && !node->children) {
l_hashmap_remove(tree->objects, path);
_dbus_object_tree_prune_node(node);
}
So since /a's node->chlidren is not empty,
l_hashmap_remove(tree->objects, path); will not happen. Both /a and
/a/b will remain in the hashmap.
> Then unregistering '/a/b'
> will prune the tree upwards.
Right, at that point /a will be removed from the tree but not from
tree->objects.
On a second thought, I think the current code can be fixed by doing two things:
if (l_queue_isempty(node->instances)) {
l_hashmap_remove(tree->objects, path);
if (!node->children)
_dbus_object_tree_prune_node(node);
}
...and adding a check in _dbus_object_tree_prune_node to make sure we
don't remove nodes that still have nonempty node->instances.
>
> Tree pruning is intended to be fully separate from the object hash, so I'm
> really not following.
>
> Perhaps you're conflating unregistering all interfaces vs unregistering the
> object? But then you mention that your semantics of not removing the object
> from the tree if all interfaces are removed is intentional in the next
> paragraph.
>
>
>>
>> Note that while the client doesn't need to call l_dbus_register_object,
>> they still need to call l_dbus_unregister_object to free the object
>> because it's not freed automatically when the last interface gets
>> removed. But they can skip the l_dbus_remove_interface calls
>> because the interfaces will be removed either way.
>> ---
>> ell/dbus-private.h | 20 ++++-
>> ell/dbus-service.c | 224
>> +++++++++++++++++++++++++++++++++++------------
>> ell/dbus.c | 91 +++++++++++++++++--
>> ell/dbus.h | 19 ++--
>> examples/dbus-service.c | 13 ++-
>> unit/test-dbus-service.c | 17 ++--
>> unit/test-kdbus.c | 9 +-
>> 7 files changed, 311 insertions(+), 82 deletions(-)
>>
>> diff --git a/ell/dbus-private.h b/ell/dbus-private.h
>> index e60d09e..fc26aa2 100644
>> --- a/ell/dbus-private.h
>> +++ b/ell/dbus-private.h
>> @@ -167,13 +167,27 @@ struct object_node
>> *_dbus_object_tree_makepath(struct _dbus_object_tree *tree,
>> const char *path);
>> struct object_node *_dbus_object_tree_lookup(struct _dbus_object_tree
>> *tree,
>> const char *path);
>> -void _dbus_object_tree_prune_node(struct object_node *node);
>> +
>> +struct object_node *_dbus_object_tree_new_object(struct _dbus_object_tree
>> *tree,
>> + const char *path,
>> + void *user_data,
>> + void (*destroy) (void *));
>> +bool _dbus_object_tree_prune_node(struct _dbus_object_tree *tree,
>> + struct object_node *node,
>> + const char *path);
>>
>> bool _dbus_object_tree_register(struct _dbus_object_tree *tree,
>> - const char *path, const char *interface,
>> + const char *interface,
>> void (*setup_func)(struct l_dbus_interface
>> *),
>> - void *user_data, void (*destroy) (void
>> *));
>> + void (*destroy) (void *),
>> + bool old_style_properties);
>
>
> Should this be called _dbus_object_tree_register_interface now? Since it
> only deals with interfaces and not object paths?
Yes, let me rename it.
>
>> bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
>> + const char *interface);
>> +
>
>
> Similarly, should this be _dbus_object_tree_unregister_interface?
Yes.
>
>
>> +bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
>> + const char *path, const char
>> *interface,
>> + void *user_data);
>> +bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
>> const char *path,
>> const char *interface);
>>
>> diff --git a/ell/dbus-service.c b/ell/dbus-service.c
>> index 77310c6..d37989e 100644
>> --- a/ell/dbus-service.c
>> +++ b/ell/dbus-service.c
>> @@ -72,6 +72,9 @@ struct l_dbus_interface {
>> struct l_queue *methods;
>> struct l_queue *signals;
>> struct l_queue *properties;
>> + bool handle_old_style_properties;
>> + void (*setup_func)(struct l_dbus_interface *);
>> + void (*instance_destroy)(void *);
>> char name[];
>> };
>>
>> @@ -84,13 +87,14 @@ struct child_node {
>> struct interface_instance {
>> struct l_dbus_interface *interface;
>> void *user_data;
>> - void (*user_destroy) (void *);
>> };
>>
>> struct object_node {
>> struct object_node *parent;
>> struct l_queue *instances;
>> struct child_node *children;
>> + void *user_data;
>> + void (*destroy) (void *);
>> };
>>
>> struct _dbus_object_tree {
>> @@ -489,8 +493,8 @@ struct _dbus_property
>> *_dbus_interface_find_property(struct l_dbus_interface *i,
>>
>> static void interface_instance_free(struct interface_instance *instance)
>> {
>> - if (instance->user_destroy)
>> - instance->user_destroy(instance->user_data);
>> + if (instance->interface->instance_destroy)
>> +
>> instance->interface->instance_destroy(instance->user_data);
>>
>> l_free(instance);
>> }
>> @@ -539,6 +543,9 @@ static void subtree_free(struct object_node *node)
>> l_queue_destroy(node->instances,
>> (l_queue_destroy_func_t) interface_instance_free);
>>
>> + if (node->destroy)
>> + node->destroy(node->user_data);
>> +
>> l_free(node);
>> }
>>
>> @@ -626,81 +633,197 @@ struct object_node *_dbus_object_tree_lookup(struct
>> _dbus_object_tree *tree,
>> return lookup_recurse(tree->root, path);
>> }
>>
>> -void _dbus_object_tree_prune_node(struct object_node *node)
>> +struct object_node *_dbus_object_tree_new_object(struct _dbus_object_tree
>> *tree,
>> + const char *path,
>> + void *user_data,
>> + void (*destroy) (void *))
>> +{
>> + struct object_node *node;
>> +
>> + if (!_dbus_valid_object_path(path))
>> + return NULL;
>> +
>> + if (l_hashmap_lookup(tree->objects, path))
>> + return NULL;
>> +
>> + node = _dbus_object_tree_makepath(tree, path);
>> + node->user_data = user_data;
>> + node->destroy = destroy;
>> +
>> + l_hashmap_insert(tree->objects, path, node);
>> +
>> + return node;
>> +}
>> +
>> +bool _dbus_object_tree_prune_node(struct _dbus_object_tree *tree,
>> + struct object_node *node,
>> + const char *path)
>
>
> This part sounds like it really belongs in a separate patch...
Do you mean both of _dbus_object_tree_new_object and
_dbus_object_tree_prune_node?
>
>
>> {
>> - struct object_node *parent = node->parent;
>> - struct child_node *p = NULL, *c;
>> + struct object_node *parent;
>> + struct child_node *p, *c;
>> + struct interface_instance *instance;
>> + char parentpath[strlen(path) + 1];
>> +
>> + if (!node) {
>> + node = l_hashmap_remove(tree->objects, path);
>> + if (!node)
>> + return false;
>> + }
>> +
>> + while ((instance = l_queue_pop_head(node->instances)))
>> + interface_instance_free(instance);
>> +
>> + if (node->destroy) {
>> + node->destroy(node->user_data);
>>
>> - while (parent) {
>> - for (c = parent->children, p = NULL; c; p = c, c =
>> c->next) {
>> - if (c->node != node)
>> - continue;
>> + node->destroy = NULL;
>> + }
>>
>> - if (p)
>> - p->next = c->next;
>> - else
>> - parent->children = c->next;
>> + if (node->children || !node->parent)
>> + return true;
>>
>> - subtree_free(c->node);
>> - l_free(c);
>> + /*
>> + * Walk up the tree until a node that either has more than one
>> + * child, is the root or is in the objects hashmap.
>> + */
>> + strcpy(parentpath, path);
>>
>> + while (true) {
>> + parent = node->parent;
>> +
>> + if (parent == tree->root)
>> + break;
>> +
>> + if (parent->children->next)
>> break;
>> - }
>>
>> - if (parent->children != NULL)
>> - return;
>> + /* Parent's path */
>> + parentpath[strlen(parentpath) -
>> + strlen(parent->children->subpath) - 1] = '\0';
>> +
>> + if (l_hashmap_lookup(tree->objects, parentpath))
>> + break;
>>
>> node = parent;
>> - parent = node->parent;
>> }
>> +
>> + for (c = parent->children, p = NULL; c; p = c, c = c->next)
>> + if (c->node == node)
>> + break;
>> +
>> + if (p)
>> + p->next = c->next;
>> + else
>> + parent->children = c->next;
>> +
>> + l_free(c);
>> + subtree_free(node);
>> +
>> + return true;
>> }
>>
>> bool _dbus_object_tree_register(struct _dbus_object_tree *tree,
>> - const char *path, const char *interface,
>> + const char *interface,
>> void (*setup_func)(struct l_dbus_interface
>> *),
>> - void *user_data, void (*destroy) (void *))
>> + void (*destroy) (void *),
>> + bool old_style_properties)
>> {
>> - struct object_node *object;
>> struct l_dbus_interface *dbi;
>> - const struct l_queue_entry *entry;
>> - struct interface_instance *instance;
>>
>> if (!_dbus_valid_interface(interface))
>> return false;
>>
>> - if (!_dbus_valid_object_path(path))
>> + /*
>> + * Check to make sure we do not have this interface already
>> + * registered
>> + */
>> + dbi = l_hashmap_lookup(tree->interfaces, interface);
>> + if (dbi)
>> + return false;
>> +
>> + dbi = _dbus_interface_new(interface);
>> + dbi->instance_destroy = destroy;
>> + dbi->setup_func = setup_func;
>
>
> Can't we just call this right away and not wait for first use? Let the
> programmer decide when to instantiate the interface.
Ok.
>
>
>> + dbi->handle_old_style_properties = old_style_properties;
>> +
>> + l_hashmap_insert(tree->interfaces, dbi->name, dbi);
>> +
>> + return true;
>> +}
>> +
>> +struct interface_check {
>> + const char *interface;
>> + bool result;
>> +};
>> +
>> +static void check_interface_used(const void *key, void *value, void
>> *user_data)
>> +{
>> + struct object_node *node = value;
>> + struct interface_check *state = user_data;
>> +
>> + if (l_queue_find(node->instances, match_interface_instance,
>> + (char *) state->interface))
>> + state->result = true;
>> +}
>> +
>> +bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
>> + const char *interface_name)
>> +{
>> + struct l_dbus_interface *interface;
>> + struct interface_check state = { interface_name, false };
>> +
>> + interface = l_hashmap_lookup(tree->interfaces, interface_name);
>> + if (!interface)
>> + return false;
>> +
>> + /* Check that the interface is not in use */
>> + l_hashmap_foreach(tree->objects, check_interface_used, &state);
>> + if (state.result)
>> + return false;
>
>
> I'm not sure this is useful in its current form. If we want to have this,
> then we probably need to distinguish between telling the programmer that
> "you're trying to unregister an unknown interface" vs "you're trying to
> unregister a interface that is still being used". As an alternative, you
> can keep a reference count on the interface itself and keep the object
> around until all instances have been destroyed, and this function would
> always succeed...
>
> However, I would keep it simple and just let the programmer control this.
> If they try to remove an interface that is still being used, things will
> crash spectacularly and valgrind/gdb will find it very fast. Remember our
> motto, "crash early, crash often" :)
Yes, good point. Or we can remove the interface and remove all of its
instances which is in a way logical.
>
>
>> +
>> + l_hashmap_remove(tree->interfaces, interface_name);
>> +
>> + _dbus_interface_free(interface);
>> +
>> + return true;
>> +}
>> +
>> +bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
>> + const char *path, const char
>> *interface,
>> + void *user_data)
>> +{
>> + struct object_node *object;
>> + struct l_dbus_interface *dbi;
>> + struct interface_instance *instance;
>> +
>> + dbi = l_hashmap_lookup(tree->interfaces, interface);
>> + if (!dbi)
>> return false;
>>
>> object = l_hashmap_lookup(tree->objects, path);
>> if (!object) {
>> - object = _dbus_object_tree_makepath(tree, path);
>> - l_hashmap_insert(tree->objects, path, object);
>> + object = _dbus_object_tree_new_object(tree, path, NULL,
>> NULL);
>> +
>> + if (!object)
>> + return false;
>> }
>>
>> /*
>> * Check to make sure we do not have this interface already
>> * registered for this object
>> */
>> - entry = l_queue_get_entries(object->instances);
>> - while (entry) {
>> - instance = entry->data;
>> -
>> - if (!strcmp(instance->interface->name, interface))
>> - return false;
>> + if (l_queue_find(object->instances, match_interface_instance,
>> + (char *) interface))
>> + return false;
>>
>> - entry = entry->next;
>> - }
>> + if (dbi->setup_func) {
>> + dbi->setup_func(dbi);
>>
>
> See my comments above...
>
>
>> - dbi = l_hashmap_lookup(tree->interfaces, interface);
>> - if (!dbi) {
>> - dbi = _dbus_interface_new(interface);
>> - setup_func(dbi);
>> - l_hashmap_insert(tree->interfaces, dbi->name, dbi);
>> + dbi->setup_func = NULL;
>> }
>>
>> instance = l_new(struct interface_instance, 1);
>> instance->interface = dbi;
>> - instance->user_destroy = destroy;
>> instance->user_data = user_data;
>>
>> if (!object->instances)
>> @@ -711,13 +834,12 @@ bool _dbus_object_tree_register(struct
>> _dbus_object_tree *tree,
>> return true;
>> }
>>
>> -bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
>> +bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
>> const char *path,
>> const char *interface)
>> {
>> struct object_node *node;
>> struct interface_instance *instance;
>> - bool r;
>>
>> node = l_hashmap_lookup(tree->objects, path);
>> if (!node)
>> @@ -725,18 +847,12 @@ bool _dbus_object_tree_unregister(struct
>> _dbus_object_tree *tree,
>>
>> instance = l_queue_remove_if(node->instances,
>> match_interface_instance, (char *) interface);
>> + if (!instance)
>> + return false;
>>
>> - r = instance ? true : false;
>> -
>> - if (instance)
>> - interface_instance_free(instance);
>> -
>> - if (l_queue_isempty(node->instances) && !node->children) {
>> - l_hashmap_remove(tree->objects, path);
>> - _dbus_object_tree_prune_node(node);
>> - }
>> + interface_instance_free(instance);
>>
>> - return r;
>> + return true;
>> }
>>
>> static void generate_interface_instance(void *data, void *user)
>> diff --git a/ell/dbus.c b/ell/dbus.c
>> index 1d9aed5..a9bb314 100644
>> --- a/ell/dbus.c
>> +++ b/ell/dbus.c
>
>
> <snip>
>
>> +
>> +LIB_EXPORT bool l_dbus_register_object(struct l_dbus *dbus, const char
>> *path,
>> + void *user_data,
>> + l_dbus_destroy_func_t destroy,
>> ...)
>> +{
>
>
> We've talked about this before, but this one really needs a documentation
> blurb or an additional example of the expected syntax.
>
> e.g. l_dbus_register_object(dbus, "/foo", object, destroy_object,
> "FooInterface", foo_data, "BarInterface", bar_data, NULL);
Ok, I will try to add some doc comments across the dbus api.
>
>
>> + va_list args;
>> + const char *interface;
>> + void *if_user_data;
>> + bool r = true;;
>> +
>> + if (unlikely(!dbus))
>> + return false;
>> +
>> + if (unlikely(!dbus->tree))
>> + return false;
>> +
>> + if (!_dbus_object_tree_new_object(dbus->tree, path, user_data,
>> destroy))
>> + return false;
>> +
>> + va_start(args, destroy);
>> + while ((interface = va_arg(args, const char *))) {
>> + if_user_data = va_arg(args, void *);
>> +
>> + if (!_dbus_object_tree_add_interface(dbus->tree, path,
>> + interface,
>> + if_user_data)) {
>> + _dbus_object_tree_prune_node(dbus->tree, NULL,
>> path);
>> + r = false;
>> +
>> + break;
>> + }
>> + }
>> + va_end(args);
>> +
>> + return r;
>> +}
>> +
>
>
> <snip>
>
>
>> diff --git a/unit/test-dbus-service.c b/unit/test-dbus-service.c
>> index d0bf174..16fc00e 100644
>> --- a/unit/test-dbus-service.c
>> +++ b/unit/test-dbus-service.c
>> @@ -205,16 +205,16 @@ static void test_dbus_object_tree(const void
>> *test_data)
>>
>> tmp = _dbus_object_tree_lookup(tree, "/foo/bee");
>> assert(tmp);
>> - _dbus_object_tree_prune_node(leaf3);
>> + _dbus_object_tree_prune_node(tree, leaf3, "/foo/bee/boo");
>> tmp = _dbus_object_tree_lookup(tree, "/foo/bee");
>> assert(!tmp);
>>
>> tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
>> assert(tmp);
>> - _dbus_object_tree_prune_node(leaf2);
>> + _dbus_object_tree_prune_node(tree, leaf2, "/foo/bar/ble");
>> tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
>> assert(tmp);
>> - _dbus_object_tree_prune_node(leaf1);
>> + _dbus_object_tree_prune_node(tree, leaf1, "/foo/bar/baz");
>> tmp = _dbus_object_tree_lookup(tree, "/foo/bar");
>> assert(!tmp);
>> tmp = _dbus_object_tree_lookup(tree, "/foo");
>> @@ -276,9 +276,10 @@ static void test_dbus_object_tree_introspection(const
>> void *test_data)
>>
>> tree = _dbus_object_tree_new();
>>
>> - _dbus_object_tree_register(tree, "/", "org.ofono.Manager",
>> + _dbus_object_tree_register(tree, "org.ofono.Manager",
>> build_manager_interface,
>> - NULL, NULL);
>> + NULL, false);
>> + _dbus_object_tree_add_interface(tree, "/", "org.ofono.Manager",
>> NULL);
>>
>> _dbus_object_tree_makepath(tree, "/phonesim");
>>
>> @@ -298,9 +299,11 @@ static void test_dbus_object_tree_dispatch(const void
>> *test_data)
>>
>> tree = _dbus_object_tree_new();
>>
>> - _dbus_object_tree_register(tree, "/", "org.ofono.Manager",
>> + _dbus_object_tree_register(tree, "org.ofono.Manager",
>> build_manager_interface,
>> - dummy_data, NULL);
>> + NULL, false);
>> + _dbus_object_tree_add_interface(tree, "/", "org.ofono.Manager",
>> + dummy_data);
>>
>> message = _dbus_message_new_method_call(1, "org.ofono", "/",
>> "org.ofono.Manager",
>
>
> <snip>
>
> By the way, applying this patch makes unit tests fail:
> denkenz(a)new-host-2 ~/ell $ unit/test-dbus-service
> TEST: Test Frobate Introspection
> TEST: Test Bazify Introspection
> TEST: Test Mogrify Introspection
> TEST: Test Changed Introspection
> TEST: Test Bar Property Introspection
> test-dbus-service: unit/test-dbus-service.c:157: test_introspect_property:
> Assertion `!strcmp(test->expected_xml, xml)' failed.
> Aborted
I see why, I'll fix patch 1/8 to avoid this.
Best regards
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [RFC PATCH 2/8] dbus: Separate interface registration from instantiation
2016-01-26 23:40 ` Andrzej Zaborowski
@ 2016-01-27 0:16 ` Denis Kenzior
0 siblings, 0 replies; 18+ messages in thread
From: Denis Kenzior @ 2016-01-27 0:16 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 3012 bytes --]
Hi Andrew,
>>
>> If we register object on '/a', then register object on '/a/b'. We have two
>> objects in the tree->objects hashmap. If we unregister object '/a' first,
>> then tree->objects will no longer contain '/a'.
>
> _dbus_object_tree_unregister currently does:
>
> if (l_queue_isempty(node->instances) && !node->children) {
> l_hashmap_remove(tree->objects, path);
> _dbus_object_tree_prune_node(node);
> }
>
> So since /a's node->chlidren is not empty,
> l_hashmap_remove(tree->objects, path); will not happen. Both /a and
> /a/b will remain in the hashmap.
Aha, okay I was wrong on that.
>
>> Then unregistering '/a/b'
>> will prune the tree upwards.
>
> Right, at that point /a will be removed from the tree but not from
> tree->objects.
>
> On a second thought, I think the current code can be fixed by doing two things:
>
> if (l_queue_isempty(node->instances)) {
> l_hashmap_remove(tree->objects, path);
> if (!node->children)
> _dbus_object_tree_prune_node(node);
> }
>
Yes, I think this is the right fix. I think I even intended it to be
this way but somehow this got screwed up. Guess we never tested
removing the parent before the children. A good candidate for a unit test.
> ...and adding a check in _dbus_object_tree_prune_node to make sure we
> don't remove nodes that still have nonempty node->instances.
>
You might want to check all uses of it to see if this is really needed.
>>> +bool _dbus_object_tree_prune_node(struct _dbus_object_tree *tree,
>>> + struct object_node *node,
>>> + const char *path)
>>
>>
>> This part sounds like it really belongs in a separate patch...
>
> Do you mean both of _dbus_object_tree_new_object and
> _dbus_object_tree_prune_node?
I only meant the prune_node fix. It probably needs a separate patch and
can go in right away.
>> I'm not sure this is useful in its current form. If we want to have this,
>> then we probably need to distinguish between telling the programmer that
>> "you're trying to unregister an unknown interface" vs "you're trying to
>> unregister a interface that is still being used". As an alternative, you
>> can keep a reference count on the interface itself and keep the object
>> around until all instances have been destroyed, and this function would
>> always succeed...
>>
>> However, I would keep it simple and just let the programmer control this.
>> If they try to remove an interface that is still being used, things will
>> crash spectacularly and valgrind/gdb will find it very fast. Remember our
>> motto, "crash early, crash often" :)
>
> Yes, good point. Or we can remove the interface and remove all of its
> instances which is in a way logical.
Or could do that. I let you decide.
>
> I see why, I'll fix patch 1/8 to avoid this.
Okay, thanks.
Regards,
-Denis
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 3/8] dbus: Don't show Introspectable on intermediate nodes
2016-01-23 2:59 [RFC PATCH 1/8] dbus: setters and getters API for properties Andrew Zaborowski
2016-01-23 2:59 ` [RFC PATCH 2/8] dbus: Separate interface registration from instantiation Andrew Zaborowski
@ 2016-01-23 2:59 ` Andrew Zaborowski
2016-01-25 15:48 ` Denis Kenzior
2016-01-23 2:59 ` [RFC PATCH 4/8] dbus: Message builder function to copy from an iter Andrew Zaborowski
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Andrew Zaborowski @ 2016-01-23 2:59 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]
org.freedesktop.* service implementations don't seem to report any
interfaces on non-object paths in the tree, neither does bluez.
http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-introspectable
doesn't say much about this so this is up for discussion.
---
ell/dbus-service.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index d37989e..4324e2e 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -866,25 +866,28 @@ static void generate_interface_instance(void *data, void *user)
void _dbus_object_tree_introspect(struct _dbus_object_tree *tree,
const char *path, struct l_string *buf)
{
- struct object_node *node;
+ struct object_node *node, *object;
struct child_node *child;
node = l_hashmap_lookup(tree->objects, path);
+ object = node;
if (!node)
node = _dbus_object_tree_lookup(tree, path);
l_string_append(buf, XML_HEAD);
l_string_append(buf, "<node>\n");
- if (node) {
+ if (object) {
l_string_append(buf, static_introspectable);
+
l_queue_foreach(node->instances,
generate_interface_instance, buf);
+ }
+ if (node)
for (child = node->children; child; child = child->next)
l_string_append_printf(buf, "\t<node name=\"%s\"/>\n",
child->subpath);
- }
l_string_append(buf, "</node>\n");
}
--
2.5.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [RFC PATCH 3/8] dbus: Don't show Introspectable on intermediate nodes
2016-01-23 2:59 ` [RFC PATCH 3/8] dbus: Don't show Introspectable on intermediate nodes Andrew Zaborowski
@ 2016-01-25 15:48 ` Denis Kenzior
0 siblings, 0 replies; 18+ messages in thread
From: Denis Kenzior @ 2016-01-25 15:48 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]
Hi Andrew,
On 01/22/2016 08:59 PM, Andrew Zaborowski wrote:
> org.freedesktop.* service implementations don't seem to report any
> interfaces on non-object paths in the tree, neither does bluez.
> http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-introspectable
> doesn't say much about this so this is up for discussion.
> ---
> ell/dbus-service.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
You're correct, most org.freedesktop implementations don't do this.
However, its a bit bizarre to me that this behavior exists. Just think
about it, you're calling Introspect on an object and that object doesn't
say it supports Introspect.
e.g.
dbus-send --session --dest=org.PulseAudio1 --type=method_call
--print-reply / org.freedesktop.DBus.Introspectable.Introspect
method return sender=:1.15 -> dest=:1.69 reply_serial=2
string "<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object
Introspection 1.0//EN"
"http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
<node>
<node name="org"/>
</node>
"
The Qt implementations seem to always include Introspect. Maybe because
they add a bunch of extra interfaces by default. E.g.:
dbus-send --session --dest=org.ofono.phonesim --type=method_call
--print-reply /some/deep org.freedesktop.DBus.Introspectable.Introspect
method return sender=:1.73 -> dest=:1.76 reply_serial=2
string "<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object
Introspection 1.0//EN"
"http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
<node>
<interface name="org.freedesktop.DBus.Introspectable">
<method name="Introspect">
<arg name="xml_data" type="s" direction="out"/>
</method>
</interface>
<interface name="org.freedesktop.DBus.Peer">
<method name="Ping"/>
<method name="GetMachineId">
<arg name="machine_uuid" type="s" direction="out"/>
</method>
</interface>
<node name="path"/>
</node>
"
In the end I think having it is more correct than not, but I'm fine
going either way.
Regards,
-Denis
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 4/8] dbus: Message builder function to copy from an iter.
2016-01-23 2:59 [RFC PATCH 1/8] dbus: setters and getters API for properties Andrew Zaborowski
2016-01-23 2:59 ` [RFC PATCH 2/8] dbus: Separate interface registration from instantiation Andrew Zaborowski
2016-01-23 2:59 ` [RFC PATCH 3/8] dbus: Don't show Introspectable on intermediate nodes Andrew Zaborowski
@ 2016-01-23 2:59 ` Andrew Zaborowski
2016-01-23 2:59 ` [RFC PATCH 5/8] dbus: Private function to retrieve the tree for a connection Andrew Zaborowski
` (3 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Andrew Zaborowski @ 2016-01-23 2:59 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 4188 bytes --]
---
ell/dbus-message.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++
ell/dbus.h | 4 ++
2 files changed, 116 insertions(+)
diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 159df82..dc4850d 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -1584,6 +1584,118 @@ LIB_EXPORT bool l_dbus_message_builder_leave_variant(
return l_dbus_message_builder_leave_container(builder, 'v');
}
+LIB_EXPORT bool l_dbus_message_builder_append_from_iter(
+ struct l_dbus_message_builder *builder,
+ struct l_dbus_message_iter *from)
+{
+ static const char *simple_types = "sogybnqiuxtd";
+ char type = from->sig_start[from->sig_pos];
+ char container_type;
+ char signature[256];
+ struct l_dbus_message_iter iter;
+ void *basic_ptr;
+ uint64_t basic;
+ uint32_t uint32_val;
+ int fd;
+ bool (*get_basic)(struct l_dbus_message_iter *, char, void *);
+ bool (*enter_func)(struct l_dbus_message_iter *,
+ struct l_dbus_message_iter *);
+ bool (*enter_struct)(struct l_dbus_message_iter *,
+ struct l_dbus_message_iter *);
+ bool (*enter_array)(struct l_dbus_message_iter *,
+ struct l_dbus_message_iter *);
+ bool (*enter_variant)(struct l_dbus_message_iter *,
+ struct l_dbus_message_iter *);
+
+ if (_dbus_message_is_gvariant(from->message)) {
+ get_basic = _gvariant_iter_next_entry_basic;
+ enter_struct = _gvariant_iter_enter_struct;
+ enter_array = _gvariant_iter_enter_array;
+ enter_variant = _gvariant_iter_enter_variant;
+ } else {
+ get_basic = _dbus1_iter_next_entry_basic;
+ enter_struct = _dbus1_iter_enter_struct;
+ enter_array = _dbus1_iter_enter_array;
+ enter_variant = _dbus1_iter_enter_variant;
+ }
+
+ if (strchr(simple_types, type)) {
+ if (strchr("sog", type)) {
+ if (!get_basic(from, type, &basic_ptr))
+ return false;
+ } else {
+ basic_ptr = &basic;
+
+ if (!get_basic(from, type, basic_ptr))
+ return false;
+ }
+
+ if (!l_dbus_message_builder_append_basic(builder, type,
+ basic_ptr))
+ return false;
+
+ return true;
+ }
+
+ switch (type) {
+ case 'h':
+ if (!get_basic(from, type, &uint32_val))
+ return false;
+
+ if (uint32_val >= from->message->num_fds)
+ return false;
+
+ fd = fcntl(from->message->fds[uint32_val], F_DUPFD_CLOEXEC, 3);
+
+ uint32_val = builder->message->num_fds;
+ builder->message->fds[builder->message->num_fds++] = fd;
+
+ if (!l_dbus_message_builder_append_basic(builder, type,
+ &uint32_val))
+ return false;
+
+ return true;
+ case '(':
+ enter_func = enter_struct;
+ container_type = DBUS_CONTAINER_TYPE_STRUCT;
+ break;
+ case '{':
+ enter_func = enter_struct;
+ container_type = DBUS_CONTAINER_TYPE_DICT_ENTRY;
+ break;
+ case 'a':
+ enter_func = enter_array;
+ container_type = DBUS_CONTAINER_TYPE_ARRAY;
+ break;
+ case 'v':
+ enter_func = enter_variant;
+ container_type = DBUS_CONTAINER_TYPE_VARIANT;
+ break;
+ default:
+ return false;
+ }
+
+ if (!enter_func(from, &iter))
+ return false;
+
+ memcpy(signature, iter.sig_start, iter.sig_len);
+ signature[iter.sig_len] = '\0';
+
+ if (!l_dbus_message_builder_enter_container(builder,
+ container_type, signature))
+ return false;
+
+ while (iter.sig_pos < iter.sig_len)
+ if (!l_dbus_message_builder_append_from_iter(builder, &iter))
+ return false;
+
+ if (!l_dbus_message_builder_leave_container(builder,
+ container_type))
+ return false;
+
+ return true;
+}
+
LIB_EXPORT struct l_dbus_message *l_dbus_message_builder_finalize(
struct l_dbus_message_builder *builder)
{
diff --git a/ell/dbus.h b/ell/dbus.h
index 011d59a..29253b6 100644
--- a/ell/dbus.h
+++ b/ell/dbus.h
@@ -188,6 +188,10 @@ bool l_dbus_message_builder_enter_variant(
bool l_dbus_message_builder_leave_variant(
struct l_dbus_message_builder *builder);
+bool l_dbus_message_builder_append_from_iter(
+ struct l_dbus_message_builder *builder,
+ struct l_dbus_message_iter *from);
+
struct l_dbus_message *l_dbus_message_builder_finalize(
struct l_dbus_message_builder *builder);
--
2.5.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [RFC PATCH 5/8] dbus: Private function to retrieve the tree for a connection.
2016-01-23 2:59 [RFC PATCH 1/8] dbus: setters and getters API for properties Andrew Zaborowski
` (2 preceding siblings ...)
2016-01-23 2:59 ` [RFC PATCH 4/8] dbus: Message builder function to copy from an iter Andrew Zaborowski
@ 2016-01-23 2:59 ` Andrew Zaborowski
2016-01-23 2:59 ` [RFC PATCH 6/8] dbus: Handle legacy GetProperties and SetProperty automatically Andrew Zaborowski
` (2 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Andrew Zaborowski @ 2016-01-23 2:59 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 967 bytes --]
---
ell/dbus-private.h | 1 +
ell/dbus.c | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index fc26aa2..0db777e 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -226,6 +226,7 @@ int _dbus_kernel_remove_match(int fd, uint64_t cookie);
uint8_t _dbus_get_version(struct l_dbus *dbus);
int _dbus_get_fd(struct l_dbus *dbus);
+struct _dbus_object_tree *_dbus_get_tree(struct l_dbus *dbus);
struct dbus1_filter_data;
diff --git a/ell/dbus.c b/ell/dbus.c
index a9bb314..50a8cd3 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -1214,6 +1214,11 @@ int _dbus_get_fd(struct l_dbus *dbus)
return l_io_get_fd(dbus->io);
}
+struct _dbus_object_tree *_dbus_get_tree(struct l_dbus *dbus)
+{
+ return dbus->tree;
+}
+
LIB_EXPORT bool l_dbus_register_interface(struct l_dbus *dbus,
const char *interface,
l_dbus_interface_setup_func_t setup_func,
--
2.5.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [RFC PATCH 6/8] dbus: Handle legacy GetProperties and SetProperty automatically.
2016-01-23 2:59 [RFC PATCH 1/8] dbus: setters and getters API for properties Andrew Zaborowski
` (3 preceding siblings ...)
2016-01-23 2:59 ` [RFC PATCH 5/8] dbus: Private function to retrieve the tree for a connection Andrew Zaborowski
@ 2016-01-23 2:59 ` Andrew Zaborowski
2016-01-29 0:16 ` Denis Kenzior
2016-01-23 2:59 ` [RFC PATCH 7/8] dbus: Implement org.freedesktop.DBus.Properties Andrew Zaborowski
2016-01-23 2:59 ` [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager Andrew Zaborowski
6 siblings, 1 reply; 18+ messages in thread
From: Andrew Zaborowski @ 2016-01-23 2:59 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 11609 bytes --]
---
ell/dbus-service.c | 216 ++++++++++++++++++++++++++++++++++++++++++++++--
examples/dbus-service.c | 86 +------------------
2 files changed, 212 insertions(+), 90 deletions(-)
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 4324e2e..2db6cbe 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -100,6 +100,7 @@ struct object_node {
struct _dbus_object_tree {
struct l_hashmap *interfaces;
struct l_hashmap *objects;
+ struct l_hashmap *property_requests;
struct object_node *root;
};
@@ -523,6 +524,8 @@ struct _dbus_object_tree *_dbus_object_tree_new()
tree->objects = l_hashmap_string_new();
+ tree->property_requests = l_hashmap_new();
+
tree->root = l_new(struct object_node, 1);
return tree;
@@ -554,6 +557,7 @@ void _dbus_object_tree_free(struct _dbus_object_tree *tree)
l_hashmap_destroy(tree->interfaces,
(l_hashmap_destroy_func_t) _dbus_interface_free);
l_hashmap_destroy(tree->objects, NULL);
+ l_hashmap_destroy(tree->property_requests, NULL);
subtree_free(tree->root);
@@ -788,6 +792,211 @@ bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
return true;
}
+static void set_property_complete(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ struct l_dbus_message *error)
+{
+ struct l_dbus_message *reply;
+ struct l_dbus_message *signal;
+ const char *property_name;
+ char signature[256];
+ struct l_dbus_message_iter variant;
+ struct l_dbus_message_builder *builder;
+
+ if (error) {
+ l_dbus_message_unref(message);
+
+ l_dbus_send(dbus, error);
+
+ return;
+ }
+
+ reply = l_dbus_message_new_method_return(message);
+ l_dbus_message_set_arguments(reply, "");
+ l_dbus_send(dbus, reply);
+
+ l_dbus_message_get_arguments(message, "sv", &property_name, &variant);
+ memcpy(signature, variant.sig_start, variant.sig_len);
+ signature[variant.sig_len] = '\0';
+
+ signal = l_dbus_message_new_signal(dbus,
+ l_dbus_message_get_path(message),
+ l_dbus_message_get_interface(message),
+ "PropertyChanged");
+
+ builder = l_dbus_message_builder_new(signal);
+
+ l_dbus_message_builder_append_basic(builder, 's', property_name);
+ l_dbus_message_builder_enter_variant(builder, signature);
+ l_dbus_message_builder_append_from_iter(builder, &variant);
+ l_dbus_message_builder_leave_variant(builder);
+
+ l_dbus_message_builder_finalize(builder);
+ l_dbus_message_builder_destroy(builder);
+ l_dbus_send(dbus, signal);
+
+ l_dbus_message_unref(message);
+}
+
+static struct l_dbus_message *old_set_property(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ void *user_data)
+{
+ struct l_dbus_interface *interface;
+ const char *property_name, *signature;
+ struct _dbus_property *property;
+ struct l_dbus_message_iter variant;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+
+ interface = l_hashmap_lookup(tree->interfaces,
+ l_dbus_message_get_interface(message));
+ /* If we got here the interface must exist */
+
+ if (!l_dbus_message_get_arguments(message, "sv", &property_name,
+ &variant))
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Invalid arguments");
+
+ property = _dbus_interface_find_property(interface, property_name);
+ if (!property)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Unknown Property %s",
+ property_name);
+
+ if (!property->setter)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Property %s is read-only",
+ property_name);
+
+ signature = property->metainfo + strlen(property->metainfo) + 1;
+
+ if (l_dbus_message_iter_get_type(&variant) != signature[0])
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Expected type '%s'",
+ signature);
+
+ property->setter(dbus, l_dbus_message_ref(message), &variant,
+ set_property_complete, user_data);
+
+ return NULL;
+}
+
+struct get_properties_state {
+ struct l_dbus_message *message, *reply;
+ const struct l_queue_entry *entry;
+ struct l_dbus_message_builder *builder;
+ void *user_data;
+ int count;
+};
+
+static void get_next_property(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ struct l_dbus_message *error)
+{
+ struct get_properties_state *state;
+ struct _dbus_property *property;
+ const char *signature;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+
+ state = l_hashmap_lookup(tree->property_requests, message);
+
+ if (error) {
+ l_dbus_message_unref(state->reply);
+
+ l_dbus_send(dbus, error);
+
+ goto done;
+ }
+
+ if (state->count) {
+ l_dbus_message_builder_leave_variant(state->builder);
+ l_dbus_message_builder_leave_dict(state->builder);
+ }
+
+ if (!state->entry) {
+ l_dbus_message_builder_leave_array(state->builder);
+ l_dbus_message_builder_finalize(state->builder);
+
+ l_dbus_send(dbus, state->reply);
+
+done:
+ l_hashmap_remove(tree->property_requests, state->message);
+ l_dbus_message_unref(state->message);
+ l_dbus_message_builder_destroy(state->builder);
+ l_free(state);
+
+ return;
+ }
+
+ property = state->entry->data;
+ signature = property->metainfo + strlen(property->metainfo) + 1;
+
+ state->entry = state->entry->next;
+ state->count += 1;
+
+ l_dbus_message_builder_enter_dict(state->builder, "sv");
+ l_dbus_message_builder_append_basic(state->builder, 's',
+ property->metainfo);
+ l_dbus_message_builder_enter_variant(state->builder,
+ signature);
+ property->getter(dbus, state->message, state->builder,
+ get_next_property, state->user_data);
+}
+
+static struct l_dbus_message *old_get_properties(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ void *user_data)
+{
+ struct l_dbus_interface *interface;
+ struct get_properties_state *state;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+
+ interface = l_hashmap_lookup(tree->interfaces,
+ l_dbus_message_get_interface(message));
+ /* If we got here the interface must exist */
+
+ state = l_new(struct get_properties_state, 1);
+
+ state->message = l_dbus_message_ref(message);
+ state->reply = l_dbus_message_new_method_return(message);
+ state->entry = l_queue_get_entries(interface->properties);
+ state->builder = l_dbus_message_builder_new(state->reply);
+ state->user_data = user_data;
+
+ l_hashmap_insert(tree->property_requests, message, state);
+
+ l_dbus_message_builder_enter_array(state->builder, "{sv}");
+
+ get_next_property(dbus, message, NULL);
+
+ return NULL;
+}
+
+static void dbus_interface_setup(struct l_dbus_interface *interface)
+{
+ interface->setup_func(interface);
+ interface->setup_func = NULL;
+
+ if (!interface->handle_old_style_properties)
+ return;
+
+ l_dbus_interface_method(interface, "SetProperty", 0,
+ old_set_property, "", "sv", "name", "value");
+ l_dbus_interface_method(interface, "GetProperties", 0,
+ old_get_properties, "a{sv}", "", "properties");
+
+ l_dbus_interface_signal(interface, "PropertyChanged", 0,
+ "sv", "name", "value");
+}
+
bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
const char *path, const char *interface,
void *user_data)
@@ -816,11 +1025,8 @@ bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
(char *) interface))
return false;
- if (dbi->setup_func) {
- dbi->setup_func(dbi);
-
- dbi->setup_func = NULL;
- }
+ if (dbi->setup_func)
+ dbus_interface_setup(dbi);
instance = l_new(struct interface_instance, 1);
instance->interface = dbi;
diff --git a/examples/dbus-service.c b/examples/dbus-service.c
index 488cc47..2158bec 100644
--- a/examples/dbus-service.c
+++ b/examples/dbus-service.c
@@ -98,81 +98,6 @@ static void test_data_destroy(void *data)
l_free(test);
}
-static struct l_dbus_message *test_set_property(struct l_dbus *dbus,
- struct l_dbus_message *message,
- void *user_data)
-{
- struct test_data *test = user_data;
- struct l_dbus_message *reply;
- struct l_dbus_message *signal;
- struct l_dbus_message_iter variant;
- const char *property;
-
- if (!l_dbus_message_get_arguments(message, "sv", &property, &variant))
- return l_dbus_message_new_error(message,
- "org.test.InvalidArguments",
- "Invalid arguments");
-
- if (!strcmp(property, "String")) {
- const char *strvalue;
-
- if (!l_dbus_message_iter_get_variant(&variant, "s",
- &strvalue))
- return l_dbus_message_new_error(message,
- "org.test.InvalidArguments",
- "String value expected");
-
- l_info("New String value: %s", strvalue);
- l_free(test->string);
- test->string = l_strdup(strvalue);
-
- signal = l_dbus_message_new_signal(dbus, "/test",
- "org.test", "PropertyChanged");
- l_dbus_message_set_arguments(signal, "sv",
- "String", "s", test->string);
- } else if (!strcmp(property, "Integer")) {
- uint32_t u;
-
- if (!l_dbus_message_iter_get_variant(&variant, "u", &u))
- return l_dbus_message_new_error(message,
- "org.test.InvalidArguments",
- "Integer value expected");
-
- l_info("New Integer value: %u", u);
- test->integer = u;
- signal = l_dbus_message_new_signal(dbus, "/test",
- "org.test", "PropertyChanged");
- l_dbus_message_set_arguments(signal, "sv",
- "Integer", "u", test->integer);
- } else
- return l_dbus_message_new_error(message,
- "org.test.InvalidArguments",
- "Unknown Property %s",
- property);
-
- reply = l_dbus_message_new_method_return(message);
- l_dbus_message_set_arguments(reply, "");
- l_dbus_send(dbus, reply);
-
- l_dbus_send(dbus, signal);
- return NULL;
-}
-
-static struct l_dbus_message *test_get_properties(struct l_dbus *dbus,
- struct l_dbus_message *message,
- void *user_data)
-{
- struct test_data *test = user_data;
- struct l_dbus_message *reply;
-
- reply = l_dbus_message_new_method_return(message);
- l_dbus_message_set_arguments(reply, "a{sv}", 2,
- "String", "s", test->string,
- "Integer", "u", test->integer);
-
- return reply;
-}
-
static struct l_dbus_message *test_method_call(struct l_dbus *dbus,
struct l_dbus_message *message,
void *user_data)
@@ -260,18 +185,9 @@ static void test_int_setter(struct l_dbus *dbus,
static void setup_test_interface(struct l_dbus_interface *interface)
{
- l_dbus_interface_method(interface, "GetProperties", 0,
- test_get_properties,
- "a{sv}", "", "properties");
- l_dbus_interface_method(interface, "SetProperty", 0,
- test_set_property,
- "", "sv", "name", "value");
l_dbus_interface_method(interface, "MethodCall", 0,
test_method_call, "", "");
- l_dbus_interface_signal(interface, "PropertyChanged", 0,
- "sv", "name", "value");
-
l_dbus_interface_property(interface, "String", 0, "s",
test_string_getter, test_string_setter);
l_dbus_interface_property(interface, "Integer", 0, "u",
@@ -308,7 +224,7 @@ int main(int argc, char *argv[])
test->integer = 42;
if (!l_dbus_register_interface(dbus, "org.test", setup_test_interface,
- test_data_destroy, false)) {
+ test_data_destroy, true)) {
l_info("Unable to register interface");
test_data_destroy(test);
goto cleanup;
--
2.5.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [RFC PATCH 6/8] dbus: Handle legacy GetProperties and SetProperty automatically.
2016-01-23 2:59 ` [RFC PATCH 6/8] dbus: Handle legacy GetProperties and SetProperty automatically Andrew Zaborowski
@ 2016-01-29 0:16 ` Denis Kenzior
0 siblings, 0 replies; 18+ messages in thread
From: Denis Kenzior @ 2016-01-29 0:16 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 954 bytes --]
Hi Andrew,
On 01/22/2016 08:59 PM, Andrew Zaborowski wrote:
> ---
> ell/dbus-service.c | 216 ++++++++++++++++++++++++++++++++++++++++++++++--
> examples/dbus-service.c | 86 +------------------
> 2 files changed, 212 insertions(+), 90 deletions(-)
>
<snip>
> +static struct l_dbus_message *old_set_property(struct l_dbus *dbus,
> + struct l_dbus_message *message,
> + void *user_data)
> +{
<snip>
> +
> + signature = property->metainfo + strlen(property->metainfo) + 1;
> +
> + if (l_dbus_message_iter_get_type(&variant) != signature[0])
> + return l_dbus_message_new_error(message,
> + "org.freedesktop.DBus.Error."
> + "InvalidArgs",
> + "Expected type '%s'",
> + signature);
> +
Is this check strictly needed? The setter would be running a
l_dbus_message_iter_get_variant call anyway which does a more thorough
signature check.
Rest looks fine.
Regards,
-Denis
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 7/8] dbus: Implement org.freedesktop.DBus.Properties
2016-01-23 2:59 [RFC PATCH 1/8] dbus: setters and getters API for properties Andrew Zaborowski
` (4 preceding siblings ...)
2016-01-23 2:59 ` [RFC PATCH 6/8] dbus: Handle legacy GetProperties and SetProperty automatically Andrew Zaborowski
@ 2016-01-23 2:59 ` Andrew Zaborowski
2016-01-23 2:59 ` [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager Andrew Zaborowski
6 siblings, 0 replies; 18+ messages in thread
From: Andrew Zaborowski @ 2016-01-23 2:59 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 11651 bytes --]
---
ell/dbus-service.c | 302 ++++++++++++++++++++++++++++++++++++++++++++++++
ell/dbus.c | 1 -
examples/dbus-service.c | 7 ++
3 files changed, 309 insertions(+), 1 deletion(-)
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 2db6cbe..4802b21 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -47,6 +47,8 @@ static const char *static_introspectable =
"\t\t\t<arg name=\"xml\" type=\"s\" direction=\"out\"/>\n"
"\t\t</method>\n\t</interface>\n";
+#define DBUS_INTERFACE_PROPERTIES "org.freedesktop.DBus.Properties"
+
struct _dbus_method {
l_dbus_interface_method_cb_t cb;
uint32_t flags;
@@ -511,6 +513,8 @@ static bool match_interface_instance(const void *a, const void *b)
return false;
}
+static void properties_setup_func(struct l_dbus_interface *);
+
struct _dbus_object_tree *_dbus_object_tree_new()
{
struct _dbus_object_tree *tree;
@@ -528,6 +532,9 @@ struct _dbus_object_tree *_dbus_object_tree_new()
tree->root = l_new(struct object_node, 1);
+ _dbus_object_tree_register(tree, DBUS_INTERFACE_PROPERTIES,
+ properties_setup_func, NULL, false);
+
return tree;
}
@@ -1163,3 +1170,298 @@ bool _dbus_object_tree_dispatch(struct _dbus_object_tree *tree,
return true;
}
+
+static void properties_get_complete(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ struct l_dbus_message *error)
+{
+ struct get_properties_state *state;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+
+ state = l_hashmap_lookup(tree->property_requests, message);
+
+ if (error) {
+ l_dbus_message_unref(state->reply);
+
+ l_dbus_send(dbus, error);
+
+ goto done;
+ }
+
+ l_dbus_message_builder_leave_variant(state->builder);
+ l_dbus_message_builder_finalize(state->builder);
+
+ l_dbus_send(dbus, state->reply);
+
+done:
+ l_hashmap_remove(tree->property_requests, state->message);
+ l_dbus_message_unref(state->message);
+ l_dbus_message_builder_destroy(state->builder);
+ l_free(state);
+}
+
+static struct l_dbus_message *properties_get(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ void *user_data)
+{
+ struct l_dbus_interface *interface;
+ struct interface_instance *instance;
+ const char *interface_name, *property_name, *signature;
+ struct _dbus_property *property;
+ struct get_properties_state *state;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+ struct object_node *object;
+
+ if (!l_dbus_message_get_arguments(message, "ss", &interface_name,
+ &property_name))
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Invalid arguments");
+
+ interface = l_hashmap_lookup(tree->interfaces, interface_name);
+ if (!interface)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Unknown Interface %s",
+ interface_name);
+
+ property = _dbus_interface_find_property(interface, property_name);
+ if (!property)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Unknown Property %s",
+ property_name);
+
+ object = l_hashmap_lookup(tree->objects,
+ l_dbus_message_get_path(message));
+ /* If we got here the object must exist */
+
+ instance = l_queue_find(object->instances,
+ match_interface_instance,
+ (char *) interface_name);
+ if (!instance)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Object has no interface %s",
+ interface_name);
+
+ state = l_new(struct get_properties_state, 1);
+
+ state->message = l_dbus_message_ref(message);
+ state->reply = l_dbus_message_new_method_return(message);
+ state->builder = l_dbus_message_builder_new(state->reply);
+
+ l_hashmap_insert(tree->property_requests, message, state);
+
+ signature = property->metainfo + strlen(property->metainfo) + 1;
+
+ l_dbus_message_builder_enter_variant(state->builder,
+ signature);
+
+ property->getter(dbus, state->message, state->builder,
+ properties_get_complete, instance->user_data);
+
+ return NULL;
+}
+
+static void properties_set_complete(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ struct l_dbus_message *error)
+{
+ struct l_dbus_message *reply;
+ struct l_dbus_message *signal;
+ const char *interface_name, *property_name;
+ char signature[256];
+ struct l_dbus_message_iter variant;
+ struct l_dbus_message_builder *builder;
+
+ if (error) {
+ l_dbus_message_unref(message);
+
+ l_dbus_send(dbus, error);
+
+ return;
+ }
+
+ reply = l_dbus_message_new_method_return(message);
+ l_dbus_message_set_arguments(reply, "");
+ l_dbus_send(dbus, reply);
+
+ l_dbus_message_get_arguments(message, "ssv", &interface_name,
+ &property_name, &variant);
+ memcpy(signature, variant.sig_start, variant.sig_len);
+ signature[variant.sig_len] = '\0';
+
+ signal = l_dbus_message_new_signal(dbus,
+ l_dbus_message_get_path(message),
+ DBUS_INTERFACE_PROPERTIES,
+ "PropertiesChanged");
+
+ builder = l_dbus_message_builder_new(signal);
+
+ l_dbus_message_builder_append_basic(builder, 's', interface_name);
+ l_dbus_message_builder_enter_array(builder, "{sv}");
+ l_dbus_message_builder_enter_dict(builder, "sv");
+ l_dbus_message_builder_append_basic(builder, 's', property_name);
+ l_dbus_message_builder_enter_variant(builder, signature);
+ l_dbus_message_builder_append_from_iter(builder, &variant);
+ l_dbus_message_builder_leave_variant(builder);
+ l_dbus_message_builder_leave_dict(builder);
+ l_dbus_message_builder_leave_array(builder);
+ l_dbus_message_builder_enter_array(builder, "s");
+ l_dbus_message_builder_leave_array(builder);
+
+ l_dbus_message_builder_finalize(builder);
+ l_dbus_message_builder_destroy(builder);
+ l_dbus_send(dbus, signal);
+
+ l_dbus_message_unref(message);
+}
+
+static struct l_dbus_message *properties_set(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ void *user_data)
+{
+ struct l_dbus_interface *interface;
+ struct interface_instance *instance;
+ const char *interface_name, *property_name, *signature;
+ struct _dbus_property *property;
+ struct l_dbus_message_iter variant;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+ struct object_node *object;
+
+ if (!l_dbus_message_get_arguments(message, "ssv", &interface_name,
+ &property_name, &variant))
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Invalid arguments");
+
+ interface = l_hashmap_lookup(tree->interfaces, interface_name);
+ if (!interface)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Unknown Interface %s",
+ interface_name);
+
+ property = _dbus_interface_find_property(interface, property_name);
+ if (!property)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Unknown Property %s",
+ property_name);
+
+ if (!property->setter)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Property %s is read-only",
+ property_name);
+
+ signature = property->metainfo + strlen(property->metainfo) + 1;
+
+ if (l_dbus_message_iter_get_type(&variant) != signature[0])
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Expected type '%s'",
+ signature);
+
+ object = l_hashmap_lookup(tree->objects,
+ l_dbus_message_get_path(message));
+ /* If we got here the object must exist */
+
+ instance = l_queue_find(object->instances,
+ match_interface_instance,
+ (char *) interface_name);
+ if (!instance)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Object has no interface %s",
+ interface_name);
+
+ property->setter(dbus, l_dbus_message_ref(message), &variant,
+ properties_set_complete, instance->user_data);
+
+ return NULL;
+}
+
+static struct l_dbus_message *properties_get_all(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ void *user_data)
+{
+ struct l_dbus_interface *interface;
+ struct interface_instance *instance;
+ const char *interface_name;
+ struct get_properties_state *state;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+ struct object_node *object;
+
+ if (!l_dbus_message_get_arguments(message, "s", &interface_name))
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Invalid arguments");
+
+ interface = l_hashmap_lookup(tree->interfaces, interface_name);
+ if (!interface)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Unknown Interface %s",
+ interface_name);
+
+ object = l_hashmap_lookup(tree->objects,
+ l_dbus_message_get_path(message));
+ /* If we got here the object must exist */
+
+ instance = l_queue_find(object->instances,
+ match_interface_instance,
+ (char *) interface_name);
+ if (!instance)
+ return l_dbus_message_new_error(message,
+ "org.freedesktop.DBus.Error."
+ "InvalidArgs",
+ "Object has no interface %s",
+ interface_name);
+
+ state = l_new(struct get_properties_state, 1);
+
+ state->message = l_dbus_message_ref(message);
+ state->reply = l_dbus_message_new_method_return(message);
+ state->entry = l_queue_get_entries(interface->properties);
+ state->builder = l_dbus_message_builder_new(state->reply);
+ state->user_data = instance->user_data;
+
+ l_hashmap_insert(tree->property_requests, message, state);
+
+ l_dbus_message_builder_enter_array(state->builder, "{sv}");
+
+ get_next_property(dbus, message, NULL);
+
+ return NULL;
+}
+
+static void properties_setup_func(struct l_dbus_interface *interface)
+{
+ l_dbus_interface_method(interface, "Get", 0,
+ properties_get, "v", "ss",
+ "value", "interface_name", "property_name");
+ l_dbus_interface_method(interface, "Set", 0,
+ properties_set, "", "ssv",
+ "interface_name", "property_name", "value");
+ l_dbus_interface_method(interface, "GetAll", 0,
+ properties_get_all, "a{sv}", "s",
+ "props", "interface_name");
+
+ l_dbus_interface_signal(interface, "PropertiesChanged", 0, "sa{sv}as",
+ "interface_name", "changed_properties",
+ "invalidated_properties");
+}
diff --git a/ell/dbus.c b/ell/dbus.c
index 50a8cd3..2089750 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -51,7 +51,6 @@
#define DBUS_INTERFACE_DBUS "org.freedesktop.DBus"
#define DBUS_INTERFACE_INTROSPECTABLE "org.freedesktop.DBus.Introspectable"
-#define DBUS_INTERFACE_PROPERTIES "org.freedesktop.DBus.Properties"
#define DBUS_MAXIMUM_MATCH_RULE_LENGTH 1024
diff --git a/examples/dbus-service.c b/examples/dbus-service.c
index 2158bec..6a838cc 100644
--- a/examples/dbus-service.c
+++ b/examples/dbus-service.c
@@ -236,6 +236,13 @@ int main(int argc, char *argv[])
goto cleanup;
}
+ if (!l_dbus_object_add_interface(dbus, "/test",
+ "org.freedesktop.DBus.Properties", NULL)) {
+ l_info("Unable to instantiate the properties interface");
+ test_data_destroy(test);
+ goto cleanup;
+ }
+
l_main_run();
l_dbus_unregister_object(dbus, "/test");
--
2.5.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager
2016-01-23 2:59 [RFC PATCH 1/8] dbus: setters and getters API for properties Andrew Zaborowski
` (5 preceding siblings ...)
2016-01-23 2:59 ` [RFC PATCH 7/8] dbus: Implement org.freedesktop.DBus.Properties Andrew Zaborowski
@ 2016-01-23 2:59 ` Andrew Zaborowski
2016-01-29 3:35 ` Denis Kenzior
6 siblings, 1 reply; 18+ messages in thread
From: Andrew Zaborowski @ 2016-01-23 2:59 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 18343 bytes --]
This is a first approximation for the Object Manager implementation, it
only works for a single ObjectManager object per tree (see FIXME) and
the code that calls the property getters sequentially can probably be
deduplicated a little.
---
ell/dbus-private.h | 2 +
ell/dbus-service.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++-
ell/dbus.c | 22 +++-
ell/dbus.h | 2 +
examples/dbus-service.c | 5 +
unit/test-dbus-service.c | 5 +-
6 files changed, 360 insertions(+), 9 deletions(-)
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 0db777e..a00ef38 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -185,9 +185,11 @@ bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
const char *interface);
bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
+ struct l_dbus *dbus,
const char *path, const char *interface,
void *user_data);
bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
+ struct l_dbus *dbus,
const char *path,
const char *interface);
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 4802b21..1e3c0f0 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -27,6 +27,8 @@
#define _GNU_SOURCE
#include <stdarg.h>
#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
#include "util.h"
#include "queue.h"
@@ -48,6 +50,7 @@ static const char *static_introspectable =
"\t\t</method>\n\t</interface>\n";
#define DBUS_INTERFACE_PROPERTIES "org.freedesktop.DBus.Properties"
+#define DBUS_INTERFACE_OBJECT_MANAGER "org.freedesktop.DBus.ObjectManager"
struct _dbus_method {
l_dbus_interface_method_cb_t cb;
@@ -104,6 +107,7 @@ struct _dbus_object_tree {
struct l_hashmap *objects;
struct l_hashmap *property_requests;
struct object_node *root;
+ struct l_queue *object_managers;
};
void _dbus_method_introspection(struct _dbus_method *info,
@@ -514,6 +518,7 @@ static bool match_interface_instance(const void *a, const void *b)
}
static void properties_setup_func(struct l_dbus_interface *);
+static void object_manager_setup_func(struct l_dbus_interface *);
struct _dbus_object_tree *_dbus_object_tree_new()
{
@@ -535,6 +540,11 @@ struct _dbus_object_tree *_dbus_object_tree_new()
_dbus_object_tree_register(tree, DBUS_INTERFACE_PROPERTIES,
properties_setup_func, NULL, false);
+ tree->object_managers = l_queue_new();
+
+ _dbus_object_tree_register(tree, DBUS_INTERFACE_OBJECT_MANAGER,
+ object_manager_setup_func, NULL, false);
+
return tree;
}
@@ -568,6 +578,8 @@ void _dbus_object_tree_free(struct _dbus_object_tree *tree)
subtree_free(tree->root);
+ l_queue_destroy(tree->object_managers, NULL);
+
l_free(tree);
}
@@ -902,6 +914,7 @@ struct get_properties_state {
struct l_dbus_message_builder *builder;
void *user_data;
int count;
+ int container_dicts;
};
static void get_next_property(struct l_dbus *dbus,
@@ -930,6 +943,10 @@ static void get_next_property(struct l_dbus *dbus,
if (!state->entry) {
l_dbus_message_builder_leave_array(state->builder);
+ while (state->container_dicts--) {
+ l_dbus_message_builder_leave_dict(state->builder);
+ l_dbus_message_builder_leave_array(state->builder);
+ }
l_dbus_message_builder_finalize(state->builder);
l_dbus_send(dbus, state->reply);
@@ -1004,13 +1021,50 @@ static void dbus_interface_setup(struct l_dbus_interface *interface)
"sv", "name", "value");
}
+static void build_interfaces_added_signal(struct l_dbus *dbus,
+ const char *manager_path,
+ const char *path,
+ struct interface_instance *instance)
+{
+ struct get_properties_state *state;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+ struct l_dbus_message *signal;
+
+ signal = l_dbus_message_new_signal(dbus, manager_path,
+ DBUS_INTERFACE_OBJECT_MANAGER,
+ "InterfacesAdded");
+
+ state = l_new(struct get_properties_state, 1);
+
+ state->message = l_dbus_message_ref(signal);
+ state->reply = signal;
+ state->entry = l_queue_get_entries(instance->interface->properties);
+ state->builder = l_dbus_message_builder_new(signal);
+ state->user_data = instance->user_data;
+ state->container_dicts = 1;
+
+ l_hashmap_insert(tree->property_requests, signal, state);
+
+ l_dbus_message_builder_append_basic(state->builder, 'o', path);
+ l_dbus_message_builder_enter_array(state->builder, "{sa{sv}}");
+ l_dbus_message_builder_enter_dict(state->builder, "sa{sv}");
+ l_dbus_message_builder_append_basic(state->builder, 's',
+ instance->interface->name);
+ l_dbus_message_builder_enter_array(state->builder, "{sv}");
+
+ get_next_property(dbus, signal, NULL);
+
+}
+
bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
- const char *path, const char *interface,
- void *user_data)
+ struct l_dbus *dbus, const char *path,
+ const char *interface, void *user_data)
{
struct object_node *object;
struct l_dbus_interface *dbi;
struct interface_instance *instance;
+ const struct l_queue_entry *entry;
+ const char *manager_path;
dbi = l_hashmap_lookup(tree->interfaces, interface);
if (!dbi)
@@ -1044,15 +1098,48 @@ bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
l_queue_push_tail(object->instances, instance);
+ if (!dbus)
+ return true;
+
+ for (entry = l_queue_get_entries(tree->object_managers); entry;
+ entry = entry->next) {
+ manager_path = entry->data;
+
+ if (memcmp(path, manager_path, strlen(manager_path)))
+ continue;
+
+ build_interfaces_added_signal(dbus, manager_path,
+ path, instance);
+
+ /*
+ * FIXME: currently we only emit one InterfacesAdded signal for
+ * the operation independent of the number of Object Managers
+ * to avoid having async getters running concurrently.
+ */
+ break;
+ }
+
+ if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER))
+ l_queue_push_tail(tree->object_managers, l_strdup(path));
+
return true;
}
+static bool match_path(const void *a, const void *b)
+{
+ return !strcmp(a, b);
+}
+
bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
- const char *path,
+ struct l_dbus *dbus, const char *path,
const char *interface)
{
struct object_node *node;
struct interface_instance *instance;
+ const struct l_queue_entry *entry;
+ char *manager_path;
+ struct l_dbus_message *signal;
+ struct l_dbus_message_builder *builder;
node = l_hashmap_lookup(tree->objects, path);
if (!node)
@@ -1065,6 +1152,38 @@ bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
interface_instance_free(instance);
+ if (!dbus)
+ return true;
+
+ if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER)) {
+ manager_path = l_queue_remove_if(tree->object_managers,
+ match_path, (char *) path);
+
+ l_free(manager_path);
+ }
+
+ for (entry = l_queue_get_entries(tree->object_managers); entry;
+ entry = entry->next) {
+ manager_path = entry->data;
+
+ if (memcmp(path, manager_path, strlen(manager_path)))
+ continue;
+
+ signal = l_dbus_message_new_signal(dbus, manager_path,
+ DBUS_INTERFACE_OBJECT_MANAGER,
+ "InterfacesRemoved");
+
+ builder = l_dbus_message_builder_new(signal);
+
+ l_dbus_message_builder_append_basic(builder, 'o', path);
+ l_dbus_message_builder_enter_array(builder, "s");
+ l_dbus_message_builder_append_basic(builder, 's', interface);
+ l_dbus_message_builder_leave_array(builder);
+ l_dbus_message_builder_finalize(builder);
+ l_dbus_message_builder_destroy(builder);
+ l_dbus_send(dbus, signal);
+ }
+
return true;
}
@@ -1465,3 +1584,211 @@ static void properties_setup_func(struct l_dbus_interface *interface)
"interface_name", "changed_properties",
"invalidated_properties");
}
+
+struct get_objects_interface {
+ const char *path;
+ struct interface_instance *instance;
+};
+
+struct get_objects_state {
+ struct get_properties_state props_state;
+ struct l_queue *interfaces;
+ char *prefix;
+ int prefix_len;
+ int count;
+ int interface_count;
+};
+
+static void get_objects_next_property(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ struct l_dbus_message *error)
+{
+ struct get_objects_state *state;
+ struct get_objects_interface *interface_info;
+ struct l_dbus_interface *interface;
+ struct _dbus_property *property;
+ const char *signature;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+
+ state = l_hashmap_lookup(tree->property_requests, message);
+
+ if (error) {
+ l_dbus_message_unref(state->props_state.reply);
+
+ l_dbus_send(dbus, error);
+
+ goto done;
+ }
+
+ if (state->props_state.count) {
+ l_dbus_message_builder_leave_variant(
+ state->props_state.builder);
+ l_dbus_message_builder_leave_dict(state->props_state.builder);
+ }
+
+ while (!state->props_state.entry) {
+ if (state->interface_count) {
+ l_dbus_message_builder_leave_array(
+ state->props_state.builder);
+ l_dbus_message_builder_leave_dict(
+ state->props_state.builder);
+ state->props_state.count = 0;
+ }
+
+ interface_info = l_queue_pop_head(state->interfaces);
+
+ if ((!interface_info || !interface_info->instance) &&
+ state->count) {
+ l_dbus_message_builder_leave_array(
+ state->props_state.builder);
+ l_dbus_message_builder_leave_dict(
+ state->props_state.builder);
+ state->interface_count = 0;
+ }
+
+ if (!interface_info) {
+ l_dbus_message_builder_leave_array(
+ state->props_state.builder);
+ l_dbus_message_builder_finalize(
+ state->props_state.builder);
+
+ l_dbus_send(dbus, state->props_state.reply);
+
+done:
+ l_hashmap_remove(tree->property_requests,
+ state->props_state.message);
+ l_dbus_message_unref(state->props_state.message);
+ l_dbus_message_builder_destroy(
+ state->props_state.builder);
+ l_queue_destroy(state->interfaces,
+ (l_queue_destroy_func_t) l_free);
+ l_free(state);
+
+ return;
+ }
+
+ if (!interface_info->instance) {
+ state->count += 1;
+
+ l_dbus_message_builder_enter_dict(
+ state->props_state.builder,
+ "oa{sa{sv}}");
+ l_dbus_message_builder_append_basic(
+ state->props_state.builder,
+ 'o', interface_info->path);
+ l_dbus_message_builder_enter_array(
+ state->props_state.builder,
+ "{sa{sv}}");
+ } else {
+ state->interface_count += 1;
+
+ interface = interface_info->instance->interface;
+
+ state->props_state.entry =
+ l_queue_get_entries(interface->properties);
+ state->props_state.user_data =
+ interface_info->instance->user_data;
+
+ l_dbus_message_builder_enter_dict(
+ state->props_state.builder,
+ "sa{sv}");
+ l_dbus_message_builder_append_basic(
+ state->props_state.builder,
+ 's', interface->name);
+ l_dbus_message_builder_enter_array(
+ state->props_state.builder,
+ "{sv}");
+ }
+
+ l_free(interface_info);
+ }
+
+ property = state->props_state.entry->data;
+ signature = property->metainfo + strlen(property->metainfo) + 1;
+
+ state->props_state.entry = state->props_state.entry->next;
+ state->props_state.count += 1;
+
+ l_dbus_message_builder_enter_dict(state->props_state.builder, "sv");
+ l_dbus_message_builder_append_basic(state->props_state.builder, 's',
+ property->metainfo);
+ l_dbus_message_builder_enter_variant(state->props_state.builder,
+ signature);
+ property->getter(dbus, state->props_state.message,
+ state->props_state.builder,
+ get_objects_next_property,
+ state->props_state.user_data);
+}
+
+/* Collect the interface data to be queried for one object */
+static void get_one_object(const void *key, void *value, void *user_data)
+{
+ const char *path = key;
+ struct object_node *object = value;
+ struct get_objects_state *state = user_data;
+ const struct l_queue_entry *entry;
+ struct get_objects_interface *interface_info;
+
+ if (memcmp(path, state->prefix, state->prefix_len))
+ return;
+
+ interface_info = l_new(struct get_objects_interface, 1);
+ interface_info->path = path;
+
+ /* Add the object's own entry */
+ l_queue_push_tail(state->interfaces, interface_info);
+
+ for (entry = l_queue_get_entries(object->instances); entry;
+ entry = entry->next) {
+ interface_info = l_new(struct get_objects_interface, 1);
+ interface_info->path = path;
+ interface_info->instance = entry->data;
+
+ /* Add a complete interface entry */
+ l_queue_push_tail(state->interfaces, interface_info);
+ }
+}
+
+static struct l_dbus_message *get_managed_objects(struct l_dbus *dbus,
+ struct l_dbus_message *message,
+ void *user_data)
+{
+ struct get_objects_state *state;
+ struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+
+ state = l_new(struct get_objects_state, 1);
+ state->interfaces = l_queue_new();
+ state->prefix_len = asprintf(&state->prefix, "%s/",
+ l_dbus_message_get_path(message));
+
+ /* Build a list of interfaces to query */
+ l_hashmap_foreach(tree->objects, get_one_object, state);
+
+ free(state->prefix);
+
+ state->props_state.message = l_dbus_message_ref(message);
+ state->props_state.reply = l_dbus_message_new_method_return(message);
+ state->props_state.builder =
+ l_dbus_message_builder_new(state->props_state.reply);
+
+ l_hashmap_insert(tree->property_requests, message, state);
+
+ l_dbus_message_builder_enter_array(state->props_state.builder,
+ "{oa{sa{sv}}}");
+
+ get_objects_next_property(dbus, message, NULL);
+
+ return NULL;
+}
+
+static void object_manager_setup_func(struct l_dbus_interface *interface)
+{
+ l_dbus_interface_method(interface, "GetManagedObjects", 0,
+ get_managed_objects, "a{oa{sa{sv}}}", "",
+ "objpath_interfaces_and_properties");
+
+ l_dbus_interface_signal(interface, "InterfacesAdded", 0, "oa{sa{sv}}",
+ "object_path", "interfaces_and_properties");
+ l_dbus_interface_signal(interface, "InterfacesRemoved", 0, "oas",
+ "object_path", "interfaces");
+}
diff --git a/ell/dbus.c b/ell/dbus.c
index 2089750..ef9666c 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -51,6 +51,7 @@
#define DBUS_INTERFACE_DBUS "org.freedesktop.DBus"
#define DBUS_INTERFACE_INTROSPECTABLE "org.freedesktop.DBus.Introspectable"
+#define DBUS_INTERFACE_OBJECT_MANAGER "org.freedesktop.DBus.ObjectManager"
#define DBUS_MAXIMUM_MATCH_RULE_LENGTH 1024
@@ -1269,7 +1270,7 @@ LIB_EXPORT bool l_dbus_register_object(struct l_dbus *dbus, const char *path,
while ((interface = va_arg(args, const char *))) {
if_user_data = va_arg(args, void *);
- if (!_dbus_object_tree_add_interface(dbus->tree, path,
+ if (!_dbus_object_tree_add_interface(dbus->tree, dbus, path,
interface,
if_user_data)) {
_dbus_object_tree_prune_node(dbus->tree, NULL, path);
@@ -1306,8 +1307,8 @@ LIB_EXPORT bool l_dbus_object_add_interface(struct l_dbus *dbus,
if (unlikely(!dbus->tree))
return false;
- return _dbus_object_tree_add_interface(dbus->tree, object, interface,
- user_data);
+ return _dbus_object_tree_add_interface(dbus->tree, dbus, object,
+ interface, user_data);
}
LIB_EXPORT bool l_dbus_object_remove_interface(struct l_dbus *dbus,
@@ -1320,10 +1321,23 @@ LIB_EXPORT bool l_dbus_object_remove_interface(struct l_dbus *dbus,
if (unlikely(!dbus->tree))
return false;
- return _dbus_object_tree_remove_interface(dbus->tree, object,
+ return _dbus_object_tree_remove_interface(dbus->tree, dbus, object,
interface);
}
+LIB_EXPORT bool l_dbus_object_manager_enable(struct l_dbus *dbus)
+{
+ if (unlikely(!dbus))
+ return false;
+
+ if (unlikely(!dbus->tree))
+ return false;
+
+ return _dbus_object_tree_add_interface(dbus->tree, dbus, "/",
+ DBUS_INTERFACE_OBJECT_MANAGER,
+ dbus->tree);
+}
+
void _dbus1_filter_format_match(struct dbus1_filter_data *data, char *rule,
size_t size)
{
diff --git a/ell/dbus.h b/ell/dbus.h
index 29253b6..84bf7a4 100644
--- a/ell/dbus.h
+++ b/ell/dbus.h
@@ -211,6 +211,8 @@ bool l_dbus_object_add_interface(struct l_dbus *dbus, const char *object,
bool l_dbus_object_remove_interface(struct l_dbus *dbus, const char *object,
const char *interface);
+bool l_dbus_object_manager_enable(struct l_dbus *dbus);
+
unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
const char *name,
l_dbus_watch_func_t disconnect_func,
diff --git a/examples/dbus-service.c b/examples/dbus-service.c
index 6a838cc..6c70720 100644
--- a/examples/dbus-service.c
+++ b/examples/dbus-service.c
@@ -219,6 +219,11 @@ int main(int argc, char *argv[])
request_name_setup,
request_name_callback, NULL, NULL);
+ if (!l_dbus_object_manager_enable(dbus)) {
+ l_info("Unable to enable Object Manager");
+ goto cleanup;
+ }
+
test = l_new(struct test_data, 1);
test->string = l_strdup("Default");
test->integer = 42;
diff --git a/unit/test-dbus-service.c b/unit/test-dbus-service.c
index 16fc00e..b79703d 100644
--- a/unit/test-dbus-service.c
+++ b/unit/test-dbus-service.c
@@ -279,7 +279,8 @@ static void test_dbus_object_tree_introspection(const void *test_data)
_dbus_object_tree_register(tree, "org.ofono.Manager",
build_manager_interface,
NULL, false);
- _dbus_object_tree_add_interface(tree, "/", "org.ofono.Manager", NULL);
+ _dbus_object_tree_add_interface(tree, NULL, "/", "org.ofono.Manager",
+ NULL);
_dbus_object_tree_makepath(tree, "/phonesim");
@@ -302,7 +303,7 @@ static void test_dbus_object_tree_dispatch(const void *test_data)
_dbus_object_tree_register(tree, "org.ofono.Manager",
build_manager_interface,
NULL, false);
- _dbus_object_tree_add_interface(tree, "/", "org.ofono.Manager",
+ _dbus_object_tree_add_interface(tree, NULL, "/", "org.ofono.Manager",
dummy_data);
message = _dbus_message_new_method_call(1, "org.ofono", "/",
--
2.5.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager
2016-01-23 2:59 ` [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager Andrew Zaborowski
@ 2016-01-29 3:35 ` Denis Kenzior
2016-01-29 16:47 ` Andrzej Zaborowski
0 siblings, 1 reply; 18+ messages in thread
From: Denis Kenzior @ 2016-01-29 3:35 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 16369 bytes --]
Hi Andrew,
On 01/22/2016 08:59 PM, Andrew Zaborowski wrote:
> This is a first approximation for the Object Manager implementation, it
> only works for a single ObjectManager object per tree (see FIXME) and
> the code that calls the property getters sequentially can probably be
> deduplicated a little.
> ---
> ell/dbus-private.h | 2 +
> ell/dbus-service.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++-
> ell/dbus.c | 22 +++-
> ell/dbus.h | 2 +
> examples/dbus-service.c | 5 +
> unit/test-dbus-service.c | 5 +-
> 6 files changed, 360 insertions(+), 9 deletions(-)
>
> diff --git a/ell/dbus-private.h b/ell/dbus-private.h
> index 0db777e..a00ef38 100644
> --- a/ell/dbus-private.h
> +++ b/ell/dbus-private.h
> @@ -185,9 +185,11 @@ bool _dbus_object_tree_unregister(struct _dbus_object_tree *tree,
> const char *interface);
>
> bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
> + struct l_dbus *dbus,
> const char *path, const char *interface,
> void *user_data);
> bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
> + struct l_dbus *dbus,
> const char *path,
> const char *interface);
I'm not (yet) convinced by this change. I think this needs a bit more
thinking.
I think the way we want to do this is to model things something along
these lines:
register_interface -> registers global interface object, e.g.
l_dbus_interface
add_interface -> adds an instance of the interface to the tree, e.g.
interface_instance
Each dbus bus instance would have a 1:1 relationship to the object tree.
For the purposes of signal emission, can we put the dbus pointer in
the object_manager instance user_data? If so, the parameter introduced
above in _dbus_object_tree_add_interface and
_dbus_object_tree_remove_interface can be omitted.
>
> diff --git a/ell/dbus-service.c b/ell/dbus-service.c
> index 4802b21..1e3c0f0 100644
> --- a/ell/dbus-service.c
> +++ b/ell/dbus-service.c
> @@ -27,6 +27,8 @@
> #define _GNU_SOURCE
> #include <stdarg.h>
> #include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
>
> #include "util.h"
> #include "queue.h"
> @@ -48,6 +50,7 @@ static const char *static_introspectable =
> "\t\t</method>\n\t</interface>\n";
>
> #define DBUS_INTERFACE_PROPERTIES "org.freedesktop.DBus.Properties"
> +#define DBUS_INTERFACE_OBJECT_MANAGER "org.freedesktop.DBus.ObjectManager"
>
> struct _dbus_method {
> l_dbus_interface_method_cb_t cb;
> @@ -104,6 +107,7 @@ struct _dbus_object_tree {
> struct l_hashmap *objects;
> struct l_hashmap *property_requests;
> struct object_node *root;
> + struct l_queue *object_managers;
> };
>
> void _dbus_method_introspection(struct _dbus_method *info,
> @@ -514,6 +518,7 @@ static bool match_interface_instance(const void *a, const void *b)
> }
>
> static void properties_setup_func(struct l_dbus_interface *);
> +static void object_manager_setup_func(struct l_dbus_interface *);
>
> struct _dbus_object_tree *_dbus_object_tree_new()
> {
> @@ -535,6 +540,11 @@ struct _dbus_object_tree *_dbus_object_tree_new()
> _dbus_object_tree_register(tree, DBUS_INTERFACE_PROPERTIES,
> properties_setup_func, NULL, false);
>
> + tree->object_managers = l_queue_new();
> +
> + _dbus_object_tree_register(tree, DBUS_INTERFACE_OBJECT_MANAGER,
> + object_manager_setup_func, NULL, false);
> +
> return tree;
> }
>
> @@ -568,6 +578,8 @@ void _dbus_object_tree_free(struct _dbus_object_tree *tree)
>
> subtree_free(tree->root);
>
> + l_queue_destroy(tree->object_managers, NULL);
> +
> l_free(tree);
> }
>
> @@ -902,6 +914,7 @@ struct get_properties_state {
> struct l_dbus_message_builder *builder;
> void *user_data;
> int count;
> + int container_dicts;
> };
>
> static void get_next_property(struct l_dbus *dbus,
> @@ -930,6 +943,10 @@ static void get_next_property(struct l_dbus *dbus,
>
> if (!state->entry) {
> l_dbus_message_builder_leave_array(state->builder);
> + while (state->container_dicts--) {
> + l_dbus_message_builder_leave_dict(state->builder);
> + l_dbus_message_builder_leave_array(state->builder);
> + }
> l_dbus_message_builder_finalize(state->builder);
>
> l_dbus_send(dbus, state->reply);
> @@ -1004,13 +1021,50 @@ static void dbus_interface_setup(struct l_dbus_interface *interface)
> "sv", "name", "value");
> }
>
> +static void build_interfaces_added_signal(struct l_dbus *dbus,
> + const char *manager_path,
> + const char *path,
> + struct interface_instance *instance)
> +{
> + struct get_properties_state *state;
> + struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
> + struct l_dbus_message *signal;
> +
> + signal = l_dbus_message_new_signal(dbus, manager_path,
> + DBUS_INTERFACE_OBJECT_MANAGER,
> + "InterfacesAdded");
> +
> + state = l_new(struct get_properties_state, 1);
> +
> + state->message = l_dbus_message_ref(signal);
> + state->reply = signal;
> + state->entry = l_queue_get_entries(instance->interface->properties);
> + state->builder = l_dbus_message_builder_new(signal);
> + state->user_data = instance->user_data;
> + state->container_dicts = 1;
> +
> + l_hashmap_insert(tree->property_requests, signal, state);
> +
> + l_dbus_message_builder_append_basic(state->builder, 'o', path);
> + l_dbus_message_builder_enter_array(state->builder, "{sa{sv}}");
> + l_dbus_message_builder_enter_dict(state->builder, "sa{sv}");
> + l_dbus_message_builder_append_basic(state->builder, 's',
> + instance->interface->name);
> + l_dbus_message_builder_enter_array(state->builder, "{sv}");
> +
> + get_next_property(dbus, signal, NULL);
> +
> +}
> +
> bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
> - const char *path, const char *interface,
> - void *user_data)
> + struct l_dbus *dbus, const char *path,
> + const char *interface, void *user_data)
> {
> struct object_node *object;
> struct l_dbus_interface *dbi;
> struct interface_instance *instance;
> + const struct l_queue_entry *entry;
> + const char *manager_path;
>
> dbi = l_hashmap_lookup(tree->interfaces, interface);
> if (!dbi)
> @@ -1044,15 +1098,48 @@ bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
>
> l_queue_push_tail(object->instances, instance);
>
> + if (!dbus)
> + return true;
> +
> + for (entry = l_queue_get_entries(tree->object_managers); entry;
> + entry = entry->next) {
> + manager_path = entry->data;
> +
> + if (memcmp(path, manager_path, strlen(manager_path)))
> + continue;
> +
> + build_interfaces_added_signal(dbus, manager_path,
> + path, instance);
> +
> + /*
> + * FIXME: currently we only emit one InterfacesAdded signal for
> + * the operation independent of the number of Object Managers
> + * to avoid having async getters running concurrently.
> + */
> + break;
> + }
> +
> + if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER))
> + l_queue_push_tail(tree->object_managers, l_strdup(path));
> +
> return true;
> }
>
> +static bool match_path(const void *a, const void *b)
> +{
> + return !strcmp(a, b);
> +}
> +
> bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
> - const char *path,
> + struct l_dbus *dbus, const char *path,
> const char *interface)
> {
> struct object_node *node;
> struct interface_instance *instance;
> + const struct l_queue_entry *entry;
> + char *manager_path;
> + struct l_dbus_message *signal;
> + struct l_dbus_message_builder *builder;
>
> node = l_hashmap_lookup(tree->objects, path);
> if (!node)
> @@ -1065,6 +1152,38 @@ bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
>
> interface_instance_free(instance);
>
> + if (!dbus)
> + return true;
> +
> + if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER)) {
> + manager_path = l_queue_remove_if(tree->object_managers,
> + match_path, (char *) path);
> +
> + l_free(manager_path);
> + }
> +
> + for (entry = l_queue_get_entries(tree->object_managers); entry;
> + entry = entry->next) {
> + manager_path = entry->data;
> +
> + if (memcmp(path, manager_path, strlen(manager_path)))
> + continue;
> +
> + signal = l_dbus_message_new_signal(dbus, manager_path,
> + DBUS_INTERFACE_OBJECT_MANAGER,
> + "InterfacesRemoved");
> +
> + builder = l_dbus_message_builder_new(signal);
> +
> + l_dbus_message_builder_append_basic(builder, 'o', path);
> + l_dbus_message_builder_enter_array(builder, "s");
> + l_dbus_message_builder_append_basic(builder, 's', interface);
> + l_dbus_message_builder_leave_array(builder);
> + l_dbus_message_builder_finalize(builder);
> + l_dbus_message_builder_destroy(builder);
> + l_dbus_send(dbus, signal);
> + }
> +
> return true;
> }
>
> @@ -1465,3 +1584,211 @@ static void properties_setup_func(struct l_dbus_interface *interface)
> "interface_name", "changed_properties",
> "invalidated_properties");
> }
> +
> +struct get_objects_interface {
> + const char *path;
> + struct interface_instance *instance;
> +};
> +
> +struct get_objects_state {
> + struct get_properties_state props_state;
> + struct l_queue *interfaces;
> + char *prefix;
> + int prefix_len;
> + int count;
> + int interface_count;
> +};
> +
> +static void get_objects_next_property(struct l_dbus *dbus,
> + struct l_dbus_message *message,
> + struct l_dbus_message *error)
> +{
> + struct get_objects_state *state;
> + struct get_objects_interface *interface_info;
> + struct l_dbus_interface *interface;
> + struct _dbus_property *property;
> + const char *signature;
> + struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
> +
> + state = l_hashmap_lookup(tree->property_requests, message);
> +
> + if (error) {
> + l_dbus_message_unref(state->props_state.reply);
> +
> + l_dbus_send(dbus, error);
> +
> + goto done;
> + }
> +
> + if (state->props_state.count) {
> + l_dbus_message_builder_leave_variant(
> + state->props_state.builder);
> + l_dbus_message_builder_leave_dict(state->props_state.builder);
> + }
> +
> + while (!state->props_state.entry) {
> + if (state->interface_count) {
> + l_dbus_message_builder_leave_array(
> + state->props_state.builder);
> + l_dbus_message_builder_leave_dict(
> + state->props_state.builder);
> + state->props_state.count = 0;
> + }
> +
> + interface_info = l_queue_pop_head(state->interfaces);
> +
> + if ((!interface_info || !interface_info->instance) &&
> + state->count) {
> + l_dbus_message_builder_leave_array(
> + state->props_state.builder);
> + l_dbus_message_builder_leave_dict(
> + state->props_state.builder);
> + state->interface_count = 0;
> + }
> +
> + if (!interface_info) {
> + l_dbus_message_builder_leave_array(
> + state->props_state.builder);
> + l_dbus_message_builder_finalize(
> + state->props_state.builder);
> +
> + l_dbus_send(dbus, state->props_state.reply);
> +
> +done:
> + l_hashmap_remove(tree->property_requests,
> + state->props_state.message);
> + l_dbus_message_unref(state->props_state.message);
> + l_dbus_message_builder_destroy(
> + state->props_state.builder);
> + l_queue_destroy(state->interfaces,
> + (l_queue_destroy_func_t) l_free);
> + l_free(state);
> +
> + return;
> + }
> +
> + if (!interface_info->instance) {
> + state->count += 1;
> +
> + l_dbus_message_builder_enter_dict(
> + state->props_state.builder,
> + "oa{sa{sv}}");
> + l_dbus_message_builder_append_basic(
> + state->props_state.builder,
> + 'o', interface_info->path);
> + l_dbus_message_builder_enter_array(
> + state->props_state.builder,
> + "{sa{sv}}");
> + } else {
> + state->interface_count += 1;
> +
> + interface = interface_info->instance->interface;
> +
> + state->props_state.entry =
> + l_queue_get_entries(interface->properties);
> + state->props_state.user_data =
> + interface_info->instance->user_data;
> +
> + l_dbus_message_builder_enter_dict(
> + state->props_state.builder,
> + "sa{sv}");
> + l_dbus_message_builder_append_basic(
> + state->props_state.builder,
> + 's', interface->name);
> + l_dbus_message_builder_enter_array(
> + state->props_state.builder,
> + "{sv}");
> + }
> +
> + l_free(interface_info);
> + }
> +
> + property = state->props_state.entry->data;
> + signature = property->metainfo + strlen(property->metainfo) + 1;
> +
> + state->props_state.entry = state->props_state.entry->next;
> + state->props_state.count += 1;
> +
> + l_dbus_message_builder_enter_dict(state->props_state.builder, "sv");
> + l_dbus_message_builder_append_basic(state->props_state.builder, 's',
> + property->metainfo);
> + l_dbus_message_builder_enter_variant(state->props_state.builder,
> + signature);
> + property->getter(dbus, state->props_state.message,
> + state->props_state.builder,
> + get_objects_next_property,
> + state->props_state.user_data);
> +}
> +
> +/* Collect the interface data to be queried for one object */
> +static void get_one_object(const void *key, void *value, void *user_data)
> +{
> + const char *path = key;
> + struct object_node *object = value;
> + struct get_objects_state *state = user_data;
> + const struct l_queue_entry *entry;
> + struct get_objects_interface *interface_info;
> +
> + if (memcmp(path, state->prefix, state->prefix_len))
> + return;
> +
> + interface_info = l_new(struct get_objects_interface, 1);
> + interface_info->path = path;
> +
We have to be pretty careful here. What if the object is removed while
we're generating the reply? Do we need to rethink having async getters?
> + /* Add the object's own entry */
> + l_queue_push_tail(state->interfaces, interface_info);
> +
> + for (entry = l_queue_get_entries(object->instances); entry;
> + entry = entry->next) {
> + interface_info = l_new(struct get_objects_interface, 1);
> + interface_info->path = path;
> + interface_info->instance = entry->data;
> +
> + /* Add a complete interface entry */
> + l_queue_push_tail(state->interfaces, interface_info);
> + }
> +}
> +
> +static struct l_dbus_message *get_managed_objects(struct l_dbus *dbus,
> + struct l_dbus_message *message,
> + void *user_data)
> +{
> + struct get_objects_state *state;
> + struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
> +
> + state = l_new(struct get_objects_state, 1);
> + state->interfaces = l_queue_new();
> + state->prefix_len = asprintf(&state->prefix, "%s/",
> + l_dbus_message_get_path(message));
> +
> + /* Build a list of interfaces to query */
> + l_hashmap_foreach(tree->objects, get_one_object, state);
> +
Yikes. Is there a way to be a bit smarter here and walk the tree
instead of using the very expensive hashmap_foreach?
> + free(state->prefix);
> +
> + state->props_state.message = l_dbus_message_ref(message);
> + state->props_state.reply = l_dbus_message_new_method_return(message);
> + state->props_state.builder =
> + l_dbus_message_builder_new(state->props_state.reply);
> +
> + l_hashmap_insert(tree->property_requests, message, state);
> +
> + l_dbus_message_builder_enter_array(state->props_state.builder,
> + "{oa{sa{sv}}}");
> +
> + get_objects_next_property(dbus, message, NULL);
> +
> + return NULL;
> +}
> +
> +static void object_manager_setup_func(struct l_dbus_interface *interface)
> +{
> + l_dbus_interface_method(interface, "GetManagedObjects", 0,
> + get_managed_objects, "a{oa{sa{sv}}}", "",
> + "objpath_interfaces_and_properties");
> +
> + l_dbus_interface_signal(interface, "InterfacesAdded", 0, "oa{sa{sv}}",
> + "object_path", "interfaces_and_properties");
> + l_dbus_interface_signal(interface, "InterfacesRemoved", 0, "oas",
> + "object_path", "interfaces");
> +}
<snip>
Regards,
-Denis
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager
2016-01-29 3:35 ` Denis Kenzior
@ 2016-01-29 16:47 ` Andrzej Zaborowski
2016-01-29 17:44 ` Denis Kenzior
0 siblings, 1 reply; 18+ messages in thread
From: Andrzej Zaborowski @ 2016-01-29 16:47 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 18593 bytes --]
Hi Denis,
On 29 January 2016 at 04:35, Denis Kenzior <denkenz@gmail.com> wrote:
> I think the way we want to do this is to model things something along these
> lines:
>
> register_interface -> registers global interface object, e.g.
> l_dbus_interface
> add_interface -> adds an instance of the interface to the tree, e.g.
> interface_instance
>
> Each dbus bus instance would have a 1:1 relationship to the object tree.
So far that is what we have in this patch series, right?
> For the purposes of signal emission, can we put the dbus pointer in the
> object_manager instance user_data? If so, the parameter introduced above in
> _dbus_object_tree_add_interface and _dbus_object_tree_remove_interface can
> be omitted.
Ok, I'll do that.
>
>
>>
>> diff --git a/ell/dbus-service.c b/ell/dbus-service.c
>> index 4802b21..1e3c0f0 100644
>> --- a/ell/dbus-service.c
>> +++ b/ell/dbus-service.c
>> @@ -27,6 +27,8 @@
>> #define _GNU_SOURCE
>> #include <stdarg.h>
>> #include <string.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>>
>> #include "util.h"
>> #include "queue.h"
>> @@ -48,6 +50,7 @@ static const char *static_introspectable =
>> "\t\t</method>\n\t</interface>\n";
>>
>> #define DBUS_INTERFACE_PROPERTIES "org.freedesktop.DBus.Properties"
>> +#define DBUS_INTERFACE_OBJECT_MANAGER
>> "org.freedesktop.DBus.ObjectManager"
>>
>> struct _dbus_method {
>> l_dbus_interface_method_cb_t cb;
>> @@ -104,6 +107,7 @@ struct _dbus_object_tree {
>> struct l_hashmap *objects;
>> struct l_hashmap *property_requests;
>> struct object_node *root;
>> + struct l_queue *object_managers;
>> };
>>
>> void _dbus_method_introspection(struct _dbus_method *info,
>> @@ -514,6 +518,7 @@ static bool match_interface_instance(const void *a,
>> const void *b)
>> }
>>
>> static void properties_setup_func(struct l_dbus_interface *);
>> +static void object_manager_setup_func(struct l_dbus_interface *);
>>
>> struct _dbus_object_tree *_dbus_object_tree_new()
>> {
>> @@ -535,6 +540,11 @@ struct _dbus_object_tree *_dbus_object_tree_new()
>> _dbus_object_tree_register(tree, DBUS_INTERFACE_PROPERTIES,
>> properties_setup_func, NULL,
>> false);
>>
>> + tree->object_managers = l_queue_new();
>> +
>> + _dbus_object_tree_register(tree, DBUS_INTERFACE_OBJECT_MANAGER,
>> + object_manager_setup_func, NULL,
>> false);
>> +
>> return tree;
>> }
>>
>> @@ -568,6 +578,8 @@ void _dbus_object_tree_free(struct _dbus_object_tree
>> *tree)
>>
>> subtree_free(tree->root);
>>
>> + l_queue_destroy(tree->object_managers, NULL);
>> +
>> l_free(tree);
>> }
>>
>> @@ -902,6 +914,7 @@ struct get_properties_state {
>> struct l_dbus_message_builder *builder;
>> void *user_data;
>> int count;
>> + int container_dicts;
>> };
>>
>> static void get_next_property(struct l_dbus *dbus,
>> @@ -930,6 +943,10 @@ static void get_next_property(struct l_dbus *dbus,
>>
>> if (!state->entry) {
>> l_dbus_message_builder_leave_array(state->builder);
>> + while (state->container_dicts--) {
>> + l_dbus_message_builder_leave_dict(state->builder);
>> +
>> l_dbus_message_builder_leave_array(state->builder);
>> + }
>> l_dbus_message_builder_finalize(state->builder);
>>
>> l_dbus_send(dbus, state->reply);
>> @@ -1004,13 +1021,50 @@ static void dbus_interface_setup(struct
>> l_dbus_interface *interface)
>> "sv", "name", "value");
>> }
>>
>> +static void build_interfaces_added_signal(struct l_dbus *dbus,
>> + const char *manager_path,
>> + const char *path,
>> + struct interface_instance
>> *instance)
>> +{
>> + struct get_properties_state *state;
>> + struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
>> + struct l_dbus_message *signal;
>> +
>> + signal = l_dbus_message_new_signal(dbus, manager_path,
>> +
>> DBUS_INTERFACE_OBJECT_MANAGER,
>> + "InterfacesAdded");
>> +
>> + state = l_new(struct get_properties_state, 1);
>> +
>> + state->message = l_dbus_message_ref(signal);
>> + state->reply = signal;
>> + state->entry =
>> l_queue_get_entries(instance->interface->properties);
>> + state->builder = l_dbus_message_builder_new(signal);
>> + state->user_data = instance->user_data;
>> + state->container_dicts = 1;
>> +
>> + l_hashmap_insert(tree->property_requests, signal, state);
>> +
>> + l_dbus_message_builder_append_basic(state->builder, 'o', path);
>> + l_dbus_message_builder_enter_array(state->builder, "{sa{sv}}");
>> + l_dbus_message_builder_enter_dict(state->builder, "sa{sv}");
>> + l_dbus_message_builder_append_basic(state->builder, 's',
>> +
>> instance->interface->name);
>> + l_dbus_message_builder_enter_array(state->builder, "{sv}");
>> +
>> + get_next_property(dbus, signal, NULL);
>> +
>> +}
>> +
>> bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
>> - const char *path, const char
>> *interface,
>> - void *user_data)
>> + struct l_dbus *dbus, const char
>> *path,
>> + const char *interface, void
>> *user_data)
>> {
>> struct object_node *object;
>> struct l_dbus_interface *dbi;
>> struct interface_instance *instance;
>> + const struct l_queue_entry *entry;
>> + const char *manager_path;
>>
>> dbi = l_hashmap_lookup(tree->interfaces, interface);
>> if (!dbi)
>> @@ -1044,15 +1098,48 @@ bool _dbus_object_tree_add_interface(struct
>> _dbus_object_tree *tree,
>>
>> l_queue_push_tail(object->instances, instance);
>>
>> + if (!dbus)
>> + return true;
>> +
>> + for (entry = l_queue_get_entries(tree->object_managers); entry;
>> + entry = entry->next) {
>> + manager_path = entry->data;
>> +
>> + if (memcmp(path, manager_path, strlen(manager_path)))
>> + continue;
>> +
>> + build_interfaces_added_signal(dbus, manager_path,
>> + path, instance);
>> +
>> + /*
>> + * FIXME: currently we only emit one InterfacesAdded
>> signal for
>> + * the operation independent of the number of Object
>> Managers
>> + * to avoid having async getters running concurrently.
>> + */
>> + break;
>> + }
>> +
>> + if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER))
>> + l_queue_push_tail(tree->object_managers, l_strdup(path));
>> +
>> return true;
>> }
>>
>> +static bool match_path(const void *a, const void *b)
>> +{
>> + return !strcmp(a, b);
>> +}
>> +
>> bool _dbus_object_tree_remove_interface(struct _dbus_object_tree *tree,
>> - const char *path,
>> + struct l_dbus *dbus, const char
>> *path,
>> const char *interface)
>> {
>> struct object_node *node;
>> struct interface_instance *instance;
>> + const struct l_queue_entry *entry;
>> + char *manager_path;
>> + struct l_dbus_message *signal;
>> + struct l_dbus_message_builder *builder;
>>
>> node = l_hashmap_lookup(tree->objects, path);
>> if (!node)
>> @@ -1065,6 +1152,38 @@ bool _dbus_object_tree_remove_interface(struct
>> _dbus_object_tree *tree,
>>
>> interface_instance_free(instance);
>>
>> + if (!dbus)
>> + return true;
>> +
>> + if (!strcmp(interface, DBUS_INTERFACE_OBJECT_MANAGER)) {
>> + manager_path = l_queue_remove_if(tree->object_managers,
>> + match_path, (char *)
>> path);
>> +
>> + l_free(manager_path);
>> + }
>> +
>> + for (entry = l_queue_get_entries(tree->object_managers); entry;
>> + entry = entry->next) {
>> + manager_path = entry->data;
>> +
>> + if (memcmp(path, manager_path, strlen(manager_path)))
>> + continue;
>> +
>> + signal = l_dbus_message_new_signal(dbus, manager_path,
>> +
>> DBUS_INTERFACE_OBJECT_MANAGER,
>> + "InterfacesRemoved");
>> +
>> + builder = l_dbus_message_builder_new(signal);
>> +
>> + l_dbus_message_builder_append_basic(builder, 'o', path);
>> + l_dbus_message_builder_enter_array(builder, "s");
>> + l_dbus_message_builder_append_basic(builder, 's',
>> interface);
>> + l_dbus_message_builder_leave_array(builder);
>> + l_dbus_message_builder_finalize(builder);
>> + l_dbus_message_builder_destroy(builder);
>> + l_dbus_send(dbus, signal);
>> + }
>> +
>> return true;
>> }
>>
>> @@ -1465,3 +1584,211 @@ static void properties_setup_func(struct
>> l_dbus_interface *interface)
>> "interface_name", "changed_properties",
>> "invalidated_properties");
>> }
>> +
>> +struct get_objects_interface {
>> + const char *path;
>> + struct interface_instance *instance;
>> +};
>> +
>> +struct get_objects_state {
>> + struct get_properties_state props_state;
>> + struct l_queue *interfaces;
>> + char *prefix;
>> + int prefix_len;
>> + int count;
>> + int interface_count;
>> +};
>> +
>> +static void get_objects_next_property(struct l_dbus *dbus,
>> + struct l_dbus_message *message,
>> + struct l_dbus_message *error)
>> +{
>> + struct get_objects_state *state;
>> + struct get_objects_interface *interface_info;
>> + struct l_dbus_interface *interface;
>> + struct _dbus_property *property;
>> + const char *signature;
>> + struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
>> +
>> + state = l_hashmap_lookup(tree->property_requests, message);
>> +
>> + if (error) {
>> + l_dbus_message_unref(state->props_state.reply);
>> +
>> + l_dbus_send(dbus, error);
>> +
>> + goto done;
>> + }
>> +
>> + if (state->props_state.count) {
>> + l_dbus_message_builder_leave_variant(
>> +
>> state->props_state.builder);
>> +
>> l_dbus_message_builder_leave_dict(state->props_state.builder);
>> + }
>> +
>> + while (!state->props_state.entry) {
>> + if (state->interface_count) {
>> + l_dbus_message_builder_leave_array(
>> +
>> state->props_state.builder);
>> + l_dbus_message_builder_leave_dict(
>> +
>> state->props_state.builder);
>> + state->props_state.count = 0;
>> + }
>> +
>> + interface_info = l_queue_pop_head(state->interfaces);
>> +
>> + if ((!interface_info || !interface_info->instance) &&
>> + state->count) {
>> + l_dbus_message_builder_leave_array(
>> +
>> state->props_state.builder);
>> + l_dbus_message_builder_leave_dict(
>> + state->props_state.builder);
>> + state->interface_count = 0;
>> + }
>> +
>> + if (!interface_info) {
>> + l_dbus_message_builder_leave_array(
>> +
>> state->props_state.builder);
>> + l_dbus_message_builder_finalize(
>> +
>> state->props_state.builder);
>> +
>> + l_dbus_send(dbus, state->props_state.reply);
>> +
>> +done:
>> + l_hashmap_remove(tree->property_requests,
>> +
>> state->props_state.message);
>> + l_dbus_message_unref(state->props_state.message);
>> + l_dbus_message_builder_destroy(
>> +
>> state->props_state.builder);
>> + l_queue_destroy(state->interfaces,
>> + (l_queue_destroy_func_t) l_free);
>> + l_free(state);
>> +
>> + return;
>> + }
>> +
>> + if (!interface_info->instance) {
>> + state->count += 1;
>> +
>> + l_dbus_message_builder_enter_dict(
>> +
>> state->props_state.builder,
>> + "oa{sa{sv}}");
>> + l_dbus_message_builder_append_basic(
>> +
>> state->props_state.builder,
>> + 'o',
>> interface_info->path);
>> + l_dbus_message_builder_enter_array(
>> +
>> state->props_state.builder,
>> + "{sa{sv}}");
>> + } else {
>> + state->interface_count += 1;
>> +
>> + interface = interface_info->instance->interface;
>> +
>> + state->props_state.entry =
>> +
>> l_queue_get_entries(interface->properties);
>> + state->props_state.user_data =
>> + interface_info->instance->user_data;
>> +
>> + l_dbus_message_builder_enter_dict(
>> +
>> state->props_state.builder,
>> + "sa{sv}");
>> + l_dbus_message_builder_append_basic(
>> +
>> state->props_state.builder,
>> + 's', interface->name);
>> + l_dbus_message_builder_enter_array(
>> +
>> state->props_state.builder,
>> + "{sv}");
>> + }
>> +
>> + l_free(interface_info);
>> + }
>> +
>> + property = state->props_state.entry->data;
>> + signature = property->metainfo + strlen(property->metainfo) + 1;
>> +
>> + state->props_state.entry = state->props_state.entry->next;
>> + state->props_state.count += 1;
>> +
>> + l_dbus_message_builder_enter_dict(state->props_state.builder,
>> "sv");
>> + l_dbus_message_builder_append_basic(state->props_state.builder,
>> 's',
>> + property->metainfo);
>> + l_dbus_message_builder_enter_variant(state->props_state.builder,
>> + signature);
>> + property->getter(dbus, state->props_state.message,
>> + state->props_state.builder,
>> + get_objects_next_property,
>> + state->props_state.user_data);
>> +}
>> +
>> +/* Collect the interface data to be queried for one object */
>> +static void get_one_object(const void *key, void *value, void *user_data)
>> +{
>> + const char *path = key;
>> + struct object_node *object = value;
>> + struct get_objects_state *state = user_data;
>> + const struct l_queue_entry *entry;
>> + struct get_objects_interface *interface_info;
>> +
>> + if (memcmp(path, state->prefix, state->prefix_len))
>> + return;
>> +
>> + interface_info = l_new(struct get_objects_interface, 1);
>> + interface_info->path = path;
>> +
>
>
> We have to be pretty careful here. What if the object is removed while
> we're generating the reply? Do we need to rethink having async getters?
That's a good point, and I also haven't yet added any code to forget
about the running getters and setters calls when a dbus connection is
shut down. But I don't think we can avoid async getters and setters.
In this case it should be sufficient to check that the path and the
instance are still present just before making the call.
>
>
>> + /* Add the object's own entry */
>> + l_queue_push_tail(state->interfaces, interface_info);
>> +
>> + for (entry = l_queue_get_entries(object->instances); entry;
>> + entry = entry->next) {
>> + interface_info = l_new(struct get_objects_interface, 1);
>> + interface_info->path = path;
>> + interface_info->instance = entry->data;
>> +
>> + /* Add a complete interface entry */
>> + l_queue_push_tail(state->interfaces, interface_info);
>> + }
>> +}
>> +
>> +static struct l_dbus_message *get_managed_objects(struct l_dbus *dbus,
>> + struct l_dbus_message
>> *message,
>> + void *user_data)
>> +{
>> + struct get_objects_state *state;
>> + struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
>> +
>> + state = l_new(struct get_objects_state, 1);
>> + state->interfaces = l_queue_new();
>> + state->prefix_len = asprintf(&state->prefix, "%s/",
>> + l_dbus_message_get_path(message));
>> +
>> + /* Build a list of interfaces to query */
>> + l_hashmap_foreach(tree->objects, get_one_object, state);
>> +
>
>
> Yikes. Is there a way to be a bit smarter here and walk the tree instead of
> using the very expensive hashmap_foreach?
We can do either, but it doesn't seem like l_hashmap_foreach is that
expensive. It is going to make some useless iterations while there
are empty buckets, later it's just as fast as a list. The tree on the
other hand has more nodes we're not interested in.
Best regards
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager
2016-01-29 16:47 ` Andrzej Zaborowski
@ 2016-01-29 17:44 ` Denis Kenzior
2016-01-29 18:53 ` Andrzej Zaborowski
0 siblings, 1 reply; 18+ messages in thread
From: Denis Kenzior @ 2016-01-29 17:44 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]
Hi Andrew,
On 01/29/2016 10:47 AM, Andrzej Zaborowski wrote:
> Hi Denis,
>
> On 29 January 2016 at 04:35, Denis Kenzior <denkenz@gmail.com> wrote:
>> I think the way we want to do this is to model things something along these
>> lines:
>>
>> register_interface -> registers global interface object, e.g.
>> l_dbus_interface
>> add_interface -> adds an instance of the interface to the tree, e.g.
>> interface_instance
>>
>> Each dbus bus instance would have a 1:1 relationship to the object tree.
>
> So far that is what we have in this patch series, right?
Sort of. We have the interfaces hanging off the tree object. I think
they might belong on a global data structure and shared between multiple
object trees.
>> We have to be pretty careful here. What if the object is removed while
>> we're generating the reply? Do we need to rethink having async getters?
>
> That's a good point, and I also haven't yet added any code to forget
> about the running getters and setters calls when a dbus connection is
> shut down. But I don't think we can avoid async getters and setters.
> In this case it should be sufficient to check that the path and the
> instance are still present just before making the call.
Setters are not the problem. Those can be async.
I'm now not convinced that getters can be async given the complexity of
the resulting code. See what you can come up with, but we might need to
think carefully about this some more.
>>
>>
>> Yikes. Is there a way to be a bit smarter here and walk the tree instead of
>> using the very expensive hashmap_foreach?
>
> We can do either, but it doesn't seem like l_hashmap_foreach is that
> expensive. It is going to make some useless iterations while there
> are empty buckets, later it's just as fast as a list. The tree on the
> other hand has more nodes we're not interested in.
Not sure I agree. If we have an object manager on a subpath, then
there's no point walking the entire hashmap. Walking through the
children of the node would be faster in this case and eliminates some
strcmps.
For the root ObjectManager, this call is also pointless since we know
all the objects will be included. Anyway, I think we have more
important things to consider, but keep this one in mind.
Regards,
-Denis
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager
2016-01-29 17:44 ` Denis Kenzior
@ 2016-01-29 18:53 ` Andrzej Zaborowski
2016-01-29 19:19 ` Denis Kenzior
0 siblings, 1 reply; 18+ messages in thread
From: Andrzej Zaborowski @ 2016-01-29 18:53 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]
Hi Denis,
On 29 January 2016 at 18:44, Denis Kenzior <denkenz@gmail.com> wrote:
> Sort of. We have the interfaces hanging off the tree object. I think they
> might belong on a global data structure and shared between multiple object
> trees.
That is something I have considered too. The code change would
probably be small so could also be done in a separate step, what do
you think?
Note that if two pieces in a modular application have separate dbus
trees and use the same interfaces, when registering them they'd have
to check the error code and ignore the error if it says the interface
is already registered.
>
>>> We have to be pretty careful here. What if the object is removed while
>>> we're generating the reply? Do we need to rethink having async getters?
>>
>>
>> That's a good point, and I also haven't yet added any code to forget
>> about the running getters and setters calls when a dbus connection is
>> shut down. But I don't think we can avoid async getters and setters.
>> In this case it should be sufficient to check that the path and the
>> instance are still present just before making the call.
>
>
> Setters are not the problem. Those can be async.
>
> I'm now not convinced that getters can be async given the complexity of the
> resulting code. See what you can come up with, but we might need to think
> carefully about this some more.
I'm not sure synchronous getters are possible for all clients, and I
think the code can be made a little cleaner. Let me submit one more
version and see if it's okay.
>
>>>
>>>
>>> Yikes. Is there a way to be a bit smarter here and walk the tree instead
>>> of
>>> using the very expensive hashmap_foreach?
>>
>>
>> We can do either, but it doesn't seem like l_hashmap_foreach is that
>> expensive. It is going to make some useless iterations while there
>> are empty buckets, later it's just as fast as a list. The tree on the
>> other hand has more nodes we're not interested in.
>
>
> Not sure I agree. If we have an object manager on a subpath, then there's
> no point walking the entire hashmap. Walking through the children of the
> node would be faster in this case and eliminates some strcmps.
>
> For the root ObjectManager, this call is also pointless since we know all
> the objects will be included.
Well, it depends on how the tree looks and how the code gets compiled
but in terms of all loop iterations, if you had long paths with a
single leaf each then walking the full tree may be more a lot longer
than walking a balanced hashmap with 128 buckets.
> Anyway, I think we have more important things
> to consider, but keep this one in mind.
I'll try doing this the way you suggest as it should be a simple
change and for non-root object managers it makes more sense.
Best regards
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 8/8] dbus: Implement org.freedesktop.DBus.ObjectManager
2016-01-29 18:53 ` Andrzej Zaborowski
@ 2016-01-29 19:19 ` Denis Kenzior
0 siblings, 0 replies; 18+ messages in thread
From: Denis Kenzior @ 2016-01-29 19:19 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 3760 bytes --]
Hi Andrew,
On 01/29/2016 12:53 PM, Andrzej Zaborowski wrote:
> Hi Denis,
>
> On 29 January 2016 at 18:44, Denis Kenzior <denkenz@gmail.com> wrote:
>> Sort of. We have the interfaces hanging off the tree object. I think they
>> might belong on a global data structure and shared between multiple object
>> trees.
>
> That is something I have considered too. The code change would
> probably be small so could also be done in a separate step, what do
> you think?
Whichever works and is easier for you.
>
> Note that if two pieces in a modular application have separate dbus
> trees and use the same interfaces, when registering them they'd have
> to check the error code and ignore the error if it says the interface
> is already registered.
>
I'm just thinking of a case where we have dbus + kdbus active or dbus +
custom bus. Probably unlikely, but the discussed structure makes sense
anyway.
Don't think we'd have multiple interface registrations in our apps.
Lets see if this will actually affect us. Worst case we change the
return value to int and return -EALREADY.
>>
>>>> We have to be pretty careful here. What if the object is removed while
>>>> we're generating the reply? Do we need to rethink having async getters?
>>>
>>>
>>> That's a good point, and I also haven't yet added any code to forget
>>> about the running getters and setters calls when a dbus connection is
>>> shut down. But I don't think we can avoid async getters and setters.
>>> In this case it should be sufficient to check that the path and the
>>> instance are still present just before making the call.
>>
>>
>> Setters are not the problem. Those can be async.
>>
>> I'm now not convinced that getters can be async given the complexity of the
>> resulting code. See what you can come up with, but we might need to think
>> carefully about this some more.
>
> I'm not sure synchronous getters are possible for all clients, and I
> think the code can be made a little cleaner. Let me submit one more
> version and see if it's okay.
>
Sure. But FYI the gdbus getters are all synchronous. So it might be
something we have to live with.
>>
>>>>
>>>>
>>>> Yikes. Is there a way to be a bit smarter here and walk the tree instead
>>>> of
>>>> using the very expensive hashmap_foreach?
>>>
>>>
>>> We can do either, but it doesn't seem like l_hashmap_foreach is that
>>> expensive. It is going to make some useless iterations while there
>>> are empty buckets, later it's just as fast as a list. The tree on the
>>> other hand has more nodes we're not interested in.
>>
>>
>> Not sure I agree. If we have an object manager on a subpath, then there's
>> no point walking the entire hashmap. Walking through the children of the
>> node would be faster in this case and eliminates some strcmps.
>>
>> For the root ObjectManager, this call is also pointless since we know all
>> the objects will be included.
>
> Well, it depends on how the tree looks and how the code gets compiled
> but in terms of all loop iterations, if you had long paths with a
> single leaf each then walking the full tree may be more a lot longer
> than walking a balanced hashmap with 128 buckets.
True, but how likely is that? :) Our trees tend to have lots of objects
hanging off / and then some nested objects. E.g. oFono operators, gprs
contexts, sms messages or iwd networks. So lets optimize for those cases.
>
>> Anyway, I think we have more important things
>> to consider, but keep this one in mind.
>
> I'll try doing this the way you suggest as it should be a simple
> change and for non-root object managers it makes more sense.
>
Sounds good.
Regards,
-Denis
^ permalink raw reply [flat|nested] 18+ messages in thread