* [PATCH 2/8] unit: Remove dbus watch tests using old API
2016-03-24 2:07 [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Andrew Zaborowski
@ 2016-03-24 2:07 ` Andrew Zaborowski
2016-03-24 2:07 ` [PATCH 3/8] dbus: Remove now unused filter logic Andrew Zaborowski
` (6 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2016-03-24 2:07 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 7067 bytes --]
---
unit/test-dbus-watch.c | 226 -------------------------------------------------
1 file changed, 226 deletions(-)
diff --git a/unit/test-dbus-watch.c b/unit/test-dbus-watch.c
index 684ac34..0dd2018 100644
--- a/unit/test-dbus-watch.c
+++ b/unit/test-dbus-watch.c
@@ -24,12 +24,8 @@
#include <config.h>
#endif
-#include <unistd.h>
#include <stdlib.h>
-#include <sys/wait.h>
#include <assert.h>
-#include <time.h>
-#include <stdio.h>
#include <ell/ell.h>
#include "ell/dbus-private.h"
@@ -38,213 +34,6 @@
#define DBUS_SERVICE_DBUS "org.freedesktop.DBus"
#define DBUS_PATH_DBUS "/org/freedesktop/DBus"
#define DBUS_INTERFACE_DBUS "org.freedesktop.DBus"
-#define DBUS_MAXIMUM_MATCH_RULE_LENGTH 1024
-
-struct watch_test {
- const char *name;
- const char *service;
- const char *path;
- const char *interface;
- const char *method;
- const char *expected;
-};
-
-static const struct watch_test match_test_1 = {
- .name = ":1.101",
- .service = DBUS_SERVICE_DBUS,
- .path = DBUS_PATH_DBUS,
- .interface = DBUS_INTERFACE_DBUS,
- .method = "NameOwnerChanged",
- .expected = "type='signal',"
- "sender='org.freedesktop.DBus',"
- "path='/org/freedesktop/DBus',"
- "interface='org.freedesktop.DBus',"
- "member='NameOwnerChanged',"
- "arg0=':1.101'",
-};
-
-static const struct watch_test match_test_2 = {
- .name = ":1.102",
- .service = NULL,
- .path = DBUS_PATH_DBUS,
- .interface = DBUS_INTERFACE_DBUS,
- .method = "NameOwnerChanged",
- .expected = "type='signal',"
- "path='/org/freedesktop/DBus',"
- "interface='org.freedesktop.DBus',"
- "member='NameOwnerChanged',"
- "arg0=':1.102'",
-};
-
-static const struct watch_test match_test_3 = {
- .name = ":1.102",
- .service = DBUS_SERVICE_DBUS,
- .path = NULL,
- .interface = DBUS_INTERFACE_DBUS,
- .method = "NameOwnerChanged",
- .expected = "type='signal',"
- "sender='org.freedesktop.DBus',"
- "interface='org.freedesktop.DBus',"
- "member='NameOwnerChanged',"
- "arg0=':1.102'",
-};
-
-static const struct watch_test match_test_4 = {
- .name = ":1.102",
- .service = DBUS_SERVICE_DBUS,
- .path = DBUS_PATH_DBUS,
- .interface = NULL,
- .method = "NameOwnerChanged",
- .expected = "type='signal',"
- "sender='org.freedesktop.DBus',"
- "path='/org/freedesktop/DBus',"
- "member='NameOwnerChanged',"
- "arg0=':1.102'",
-};
-
-static const struct watch_test match_test_5 = {
- .name = ":1.102",
- .service = DBUS_SERVICE_DBUS,
- .path = DBUS_PATH_DBUS,
- .interface = DBUS_INTERFACE_DBUS,
- .method = NULL,
- .expected = "type='signal',"
- "sender='org.freedesktop.DBus',"
- "path='/org/freedesktop/DBus',"
- "interface='org.freedesktop.DBus',"
- "arg0=':1.102'",
-};
-
-static const struct watch_test match_test_6 = {
- .name = NULL,
- .service = DBUS_SERVICE_DBUS,
- .path = DBUS_PATH_DBUS,
- .interface = DBUS_INTERFACE_DBUS,
- .method = "NameOwnerChanged",
- .expected = "type='signal',"
- "sender='org.freedesktop.DBus',"
- "path='/org/freedesktop/DBus',"
- "interface='org.freedesktop.DBus',"
- "member='NameOwnerChanged'",
-};
-
-static const struct watch_test match_test_7 = {
- .name = ":1.101",
- .service = NULL,
- .path = NULL,
- .interface = DBUS_INTERFACE_DBUS,
- .method = "NameOwnerChanged",
- .expected = "type='signal',"
- "interface='org.freedesktop.DBus',"
- "member='NameOwnerChanged',"
- "arg0=':1.101'",
-};
-
-static const struct watch_test match_test_8 = {
- .name = ":1.101",
- .service = NULL,
- .path = NULL,
- .interface = NULL,
- .method = "NameOwnerChanged",
- .expected = "type='signal',"
- "member='NameOwnerChanged',"
- "arg0=':1.101'",
-};
-
-static const struct watch_test match_test_9 = {
- .name = NULL,
- .service = NULL,
- .path = NULL,
- .interface = NULL,
- .method = NULL,
- .expected = "type='signal'",
-};
-
-static struct watch_test disconnect_test = {
- .name = ":101.1",
- .service = DBUS_SERVICE_DBUS,
- .path = DBUS_PATH_DBUS,
- .interface = DBUS_INTERFACE_DBUS,
- .method = "NameOwnerChanged",
-};
-
-static void test_match(const void *test_data)
-{
- const struct watch_test *test = test_data;
- struct dbus1_filter_data *data;
- char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
-
- data = _dbus1_filter_data_get(NULL,
- NULL,
- test->service,
- test->path,
- test->interface,
- test->method,
- test->name,
- NULL,
- NULL,
- NULL);
-
- _dbus1_filter_format_match(data, rule, sizeof(rule));
-
- assert(strcmp(rule, test->expected) == 0);
-
- _dbus1_filter_data_destroy(data);
-}
-
-static void disconnect_cb(struct l_dbus *dbus, void *user_data)
-{
- int *count = user_data;
-
- (*count)++;
-}
-
-static void send_filter_signal(struct dbus1_filter_data *data,
- const char *name, const char *old, const char *new)
-{
- struct l_dbus_message *message;
-
- message = _dbus_message_new_signal(2,
- DBUS_PATH_DBUS,
- DBUS_INTERFACE_DBUS,
- "NameOwnerChanged");
- l_dbus_message_set_arguments(message, "sss", name, old, new);
- _dbus_message_set_sender(message, DBUS_SERVICE_DBUS);
- _dbus1_signal_dispatcher(message, data);
- l_dbus_message_unref(message);
-}
-
-static void test_disconnect_watch(const void *test_data)
-{
- const struct watch_test *test = test_data;
- struct dbus1_filter_data *data;
- int count = 0;
-
- data = _dbus1_filter_data_get(NULL,
- _dbus1_name_owner_changed_filter,
- test->service,
- test->path,
- test->interface,
- test->method,
- test->name,
- disconnect_cb,
- &count,
- NULL);
-
- send_filter_signal(data, ":0.1", ":0.1", "");
- assert(count == 0);
-
- send_filter_signal(data, ":0.1", ":0.1", ":0.2");
- assert(count == 0);
-
- send_filter_signal(data, ":101.1", ":101.1", ":101.1");
- assert(count == 0);
-
- send_filter_signal(data, ":101.1", ":101.1", "");
- assert(count == 1);
-
- _dbus1_filter_data_destroy(data);
-}
static void test_rule_to_str(const void *test_data)
{
@@ -474,21 +263,6 @@ int main(int argc, char *argv[])
{
l_test_init(&argc, &argv);
- l_test_add("DBus filter NameOwnerChanged", test_match, &match_test_1);
- l_test_add("DBus filter NULL service", test_match, &match_test_2);
- l_test_add("DBus filter NULL path", test_match, &match_test_3);
- l_test_add("DBus filter NULL interface", test_match, &match_test_4);
- l_test_add("DBus filter NULL method", test_match, &match_test_5);
- l_test_add("DBus filter NULL argument", test_match, &match_test_6);
- l_test_add("DBus filter NULL service and path", test_match,
- &match_test_7);
- l_test_add("DBus filter NULL service, path and interface", test_match,
- &match_test_8);
- l_test_add("DBus filter NULL all fields", test_match, &match_test_9);
-
- l_test_add("DBus disconnect watch", test_disconnect_watch,
- &disconnect_test);
-
l_test_add("_dbus_filter_rule_to_str", test_rule_to_str, NULL);
l_test_add("DBus filter tree", test_filter_tree, NULL);
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 3/8] dbus: Remove now unused filter logic
2016-03-24 2:07 [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Andrew Zaborowski
2016-03-24 2:07 ` [PATCH 2/8] unit: Remove dbus watch tests using old API Andrew Zaborowski
@ 2016-03-24 2:07 ` Andrew Zaborowski
2016-03-24 2:07 ` [PATCH 4/8] dbus: l_dbus_name_acquire public API and driver declarations Andrew Zaborowski
` (5 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2016-03-24 2:07 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 7130 bytes --]
---
ell/dbus-private.h | 20 -----
ell/dbus.c | 209 -----------------------------------------------------
2 files changed, 229 deletions(-)
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 3b40fea..b455df2 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -287,23 +287,3 @@ char *_dbus_filter_rule_to_str(const struct _dbus_filter_condition *rule,
void _dbus_filter_dispatch(struct l_dbus_message *message, void *user_data);
void _dbus_filter_name_owner_notify(struct _dbus_filter *filter,
const char *name, const char *owner);
-
-struct dbus1_filter_data;
-
-void _dbus1_filter_format_match(struct dbus1_filter_data *data, char *rule,
- size_t size);
-
-struct dbus1_filter_data *_dbus1_filter_data_get(struct l_dbus *dbus,
- l_dbus_message_func_t filter,
- const char *sender,
- const char *path,
- const char *interface,
- const char *member,
- const char *argument,
- l_dbus_watch_func_t disconnect_func,
- void *user_data,
- l_dbus_destroy_func_t destroy);
-void _dbus1_filter_data_destroy(void *user_data);
-void _dbus1_signal_dispatcher(struct l_dbus_message *message, void *user_data);
-void _dbus1_name_owner_changed_filter(struct l_dbus_message *message,
- void *user_data);
diff --git a/ell/dbus.c b/ell/dbus.c
index 6bf5734..b52192d 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -139,23 +139,6 @@ struct service_watch_data {
bool removed;
};
-struct dbus1_filter_data {
- struct l_dbus *dbus;
- char *sender;
- char *path;
- char *interface;
- char *member;
- char *argument;
- void *user_data;
- l_dbus_message_func_t handle_func;
- l_dbus_destroy_func_t destroy_func;
-
- l_dbus_watch_func_t connect_func;
- l_dbus_watch_func_t disconnect_func;
- char *owner;
- uint32_t get_name_owner_id;
-};
-
static void message_queue_destroy(void *data)
{
struct message_callback *callback = data;
@@ -1683,198 +1666,6 @@ LIB_EXPORT bool l_dbus_object_manager_enable(struct l_dbus *dbus)
dbus);
}
-void _dbus1_filter_format_match(struct dbus1_filter_data *data, char *rule,
- size_t size)
-{
- int offset;
-
- offset = snprintf(rule, size, "type='signal'");
-
- if (data->sender)
- offset += snprintf(rule + offset, size - offset,
- ",sender='%s'", data->sender);
- if (data->path)
- offset += snprintf(rule + offset, size - offset,
- ",path='%s'", data->path);
- if (data->interface)
- offset += snprintf(rule + offset, size - offset,
- ",interface='%s'", data->interface);
- if (data->member)
- offset += snprintf(rule + offset, size - offset,
- ",member='%s'", data->member);
- if (data->argument)
- snprintf(rule + offset, size - offset,
- ",arg0='%s'", data->argument);
-}
-
-struct dbus1_filter_data *_dbus1_filter_data_get(struct l_dbus *dbus,
- l_dbus_message_func_t filter,
- const char *sender,
- const char *path,
- const char *interface,
- const char *member,
- const char *argument,
- l_dbus_watch_func_t disconnect_func,
- void *user_data,
- l_dbus_destroy_func_t destroy)
-{
- struct dbus1_filter_data *data;
-
- data = l_new(struct dbus1_filter_data, 1);
-
- data->dbus = dbus;
- data->handle_func = filter;
- data->sender = l_strdup(sender);
- data->path = l_strdup(path);
- data->interface = l_strdup(interface);
- data->member = l_strdup(member);
- data->argument = l_strdup(argument);
- data->user_data = user_data;
- data->destroy_func = destroy;
-
- data->disconnect_func = disconnect_func;
-
- return data;
-}
-
-void _dbus1_filter_data_destroy(void *user_data)
-{
- struct dbus1_filter_data *data = user_data;
-
- l_free(data->sender);
- l_free(data->path);
- l_free(data->interface);
- l_free(data->member);
- l_free(data->argument);
-
- l_free(data->owner);
-
- if (data->get_name_owner_id)
- l_dbus_cancel(data->dbus, data->get_name_owner_id);
-
- if (data->destroy_func)
- data->destroy_func(data->user_data);
-
- l_free(data);
-}
-
-static void dbus1_send_match(struct l_dbus *dbus, const char *rule,
- const char *method)
-{
- struct l_dbus_message *message;
-
- message = l_dbus_message_new_method_call(dbus,
- DBUS_SERVICE_DBUS,
- DBUS_PATH_DBUS,
- L_DBUS_INTERFACE_DBUS,
- method);
-
- l_dbus_message_set_arguments(message, "s", rule);
-
- send_message(dbus, false, message, NULL, NULL, NULL);
-}
-
-static void dbus1_bus_add_match(struct l_dbus *dbus, const char *rule)
-{
- dbus1_send_match(dbus, rule, "AddMatch");
-}
-
-static void dbus1_bus_remove_match(struct l_dbus *dbus, const char *rule)
-{
- dbus1_send_match(dbus, rule, "RemoveMatch");
-}
-
-static void add_match(struct dbus1_filter_data *data)
-{
- char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
-
- _dbus1_filter_format_match(data, rule, sizeof(rule));
-
- dbus1_bus_add_match(data->dbus, rule);
-}
-
-static void remove_match(struct dbus1_filter_data *data)
-{
- char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
-
- _dbus1_filter_format_match(data, rule, sizeof(rule));
-
- dbus1_bus_remove_match(data->dbus, rule);
-}
-
-static void filter_data_destroy(void *user_data)
-{
- remove_match(user_data);
-
- _dbus1_filter_data_destroy(user_data);
-}
-
-void _dbus1_signal_dispatcher(struct l_dbus_message *message, void *user_data)
-{
- struct dbus1_filter_data *data = user_data;
- const char *sender, *path, *iface, *member;
-
- if (_dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_SIGNAL)
- return;
-
- sender = l_dbus_message_get_sender(message);
- if (!sender)
- return;
-
- if (data->sender && strcmp(sender, data->sender))
- return;
-
- path = l_dbus_message_get_path(message);
- if (data->path && strcmp(path, data->path))
- return;
-
- iface = l_dbus_message_get_interface(message);
- if (data->interface && strcmp(iface, data->interface))
- return;
-
- member = l_dbus_message_get_member(message);
- if (data->member && strcmp(member, data->member))
- return;
-
- if (data->handle_func)
- data->handle_func(message, data);
-}
-
-void _dbus1_name_owner_changed_filter(struct l_dbus_message *message,
- void *user_data)
-{
- struct dbus1_filter_data *data = user_data;
- char *name, *old, *new;
-
- if (!l_dbus_message_get_arguments(message, "sss",
- &name, &old, &new))
- return;
-
- if (strcmp(name, data->argument))
- return;
-
- if (!data->owner) {
- if (data->get_name_owner_id)
- l_dbus_cancel(data->dbus, data->get_name_owner_id);
-
- data->get_name_owner_id = 0;
- } else {
- if (!strcmp(data->owner, new))
- return;
- }
-
- l_free(data->owner);
- data->owner = l_strdup(new);
-
- if (*new == '\0') {
- if (data->disconnect_func)
- data->disconnect_func(data->dbus, data->user_data);
- } else {
- if (data->connect_func)
- data->connect_func(data->dbus, data->user_data);
- }
-}
-
LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
const char *name,
l_dbus_watch_func_t disconnect_func,
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 4/8] dbus: l_dbus_name_acquire public API and driver declarations
2016-03-24 2:07 [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Andrew Zaborowski
2016-03-24 2:07 ` [PATCH 2/8] unit: Remove dbus watch tests using old API Andrew Zaborowski
2016-03-24 2:07 ` [PATCH 3/8] dbus: Remove now unused filter logic Andrew Zaborowski
@ 2016-03-24 2:07 ` Andrew Zaborowski
2016-03-25 3:03 ` Denis Kenzior
2016-03-24 2:07 ` [PATCH 5/8] dbus: kdbus driver->name_acquire implementation Andrew Zaborowski
` (4 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Andrew Zaborowski @ 2016-03-24 2:07 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 3110 bytes --]
---
ell/dbus.c | 33 +++++++++++++++++++++++++++++++++
ell/dbus.h | 8 ++++++++
2 files changed, 41 insertions(+)
diff --git a/ell/dbus.c b/ell/dbus.c
index b52192d..1570d33 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -64,6 +64,10 @@ struct l_dbus_ops {
struct l_dbus_message *(*recv_message)(struct l_dbus *bus);
void (*free)(struct l_dbus *bus);
struct _dbus_filter_ops filter_ops;
+ uint32_t (*name_acquire)(struct l_dbus *dbus, const char *name,
+ bool allow_replacement, bool replace_existing,
+ bool queue, l_dbus_name_acquire_func_t callback,
+ void *user_data);
};
struct l_dbus {
@@ -1893,3 +1897,32 @@ LIB_EXPORT bool l_dbus_remove_signal_watch(struct l_dbus *dbus, unsigned int id)
{
return _dbus_filter_remove_rule(dbus->filter, id);
}
+
+/**
+ * l_dbus_name_acquire:
+ * @dbus: D-Bus connection
+ * @name: Well-known bus name to be acquired
+ * @allow_replacement: Whether to allow another peer's name request to
+ * take the name ownership away from this connection
+ * @replace_existing: Whether to allow D-Bus to take the name's ownership
+ * away from another peer in case the name is already
+ * owned and allows replacement. Ignored if name is
+ * currently free.
+ * @queue: Whether to allow the name request to be queued by D-Bus in
+ * case it cannot be acquired now, rather than to return a failure.
+ * @callback: Callback to receive the request result when done.
+ *
+ * Acquire a well-known bus name (service name) on the bus.
+ *
+ * Returns: a non-zero request serial that can be passed to l_dbus_cancel
+ * while waiting for the callback, or zero on failure.
+ **/
+LIB_EXPORT uint32_t l_dbus_name_acquire(struct l_dbus *dbus, const char *name,
+ bool allow_replacement, bool replace_existing,
+ bool queue, l_dbus_name_acquire_func_t callback,
+ void *user_data)
+{
+ return dbus->driver->name_acquire(dbus, name, allow_replacement,
+ replace_existing, queue,
+ callback, user_data);
+}
diff --git a/ell/dbus.h b/ell/dbus.h
index 0f20499..b096ed3 100644
--- a/ell/dbus.h
+++ b/ell/dbus.h
@@ -67,6 +67,9 @@ typedef void (*l_dbus_interface_setup_func_t) (struct l_dbus_interface *);
typedef void (*l_dbus_watch_func_t) (struct l_dbus *dbus, void *user_data);
+typedef void (*l_dbus_name_acquire_func_t) (struct l_dbus *dbus, bool success,
+ bool queued, void *user_data);
+
struct l_dbus *l_dbus_new(const char *address);
struct l_dbus *l_dbus_new_default(enum l_dbus_bus bus);
void l_dbus_destroy(struct l_dbus *dbus);
@@ -252,6 +255,11 @@ unsigned int l_dbus_add_signal_watch(struct l_dbus *dbus,
const char *member, ...);
bool l_dbus_remove_signal_watch(struct l_dbus *dbus, unsigned int id);
+uint32_t l_dbus_name_acquire(struct l_dbus *dbus, const char *name,
+ bool allow_replacement, bool replace_existing,
+ bool queue, l_dbus_name_acquire_func_t callback,
+ void *user_data);
+
#ifdef __cplusplus
}
#endif
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/8] dbus: l_dbus_name_acquire public API and driver declarations
2016-03-24 2:07 ` [PATCH 4/8] dbus: l_dbus_name_acquire public API and driver declarations Andrew Zaborowski
@ 2016-03-25 3:03 ` Denis Kenzior
2016-03-26 0:15 ` Andrzej Zaborowski
0 siblings, 1 reply; 16+ messages in thread
From: Denis Kenzior @ 2016-03-25 3:03 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]
Hi Andrew,
On 03/23/2016 09:07 PM, Andrew Zaborowski wrote:
> ---
> ell/dbus.c | 33 +++++++++++++++++++++++++++++++++
> ell/dbus.h | 8 ++++++++
> 2 files changed, 41 insertions(+)
<snip>
>
> +
> +/**
> + * l_dbus_name_acquire:
> + * @dbus: D-Bus connection
> + * @name: Well-known bus name to be acquired
> + * @allow_replacement: Whether to allow another peer's name request to
> + * take the name ownership away from this connection
> + * @replace_existing: Whether to allow D-Bus to take the name's ownership
> + * away from another peer in case the name is already
> + * owned and allows replacement. Ignored if name is
> + * currently free.
> + * @queue: Whether to allow the name request to be queued by D-Bus in
> + * case it cannot be acquired now, rather than to return a failure.
> + * @callback: Callback to receive the request result when done.
> + *
> + * Acquire a well-known bus name (service name) on the bus.
> + *
> + * Returns: a non-zero request serial that can be passed to l_dbus_cancel
> + * while waiting for the callback, or zero on failure.
Yet kdbus version returns some hard-coded number that can't be used by
l_dbus_cancel. I don't like this API asymmetry. Can't we just return
void and let the callback parameters tell the application what happened?
Regards,
-Denis
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/8] dbus: l_dbus_name_acquire public API and driver declarations
2016-03-25 3:03 ` Denis Kenzior
@ 2016-03-26 0:15 ` Andrzej Zaborowski
0 siblings, 0 replies; 16+ messages in thread
From: Andrzej Zaborowski @ 2016-03-26 0:15 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]
Hi Denis,
On 25 March 2016 at 04:03, Denis Kenzior <denkenz@gmail.com> wrote:
> On 03/23/2016 09:07 PM, Andrew Zaborowski wrote:
>> +/**
>> + * l_dbus_name_acquire:
>> + * @dbus: D-Bus connection
>> + * @name: Well-known bus name to be acquired
>> + * @allow_replacement: Whether to allow another peer's name request to
>> + * take the name ownership away from this connection
>> + * @replace_existing: Whether to allow D-Bus to take the name's ownership
>> + * away from another peer in case the name is already
>> + * owned and allows replacement. Ignored if name is
>> + * currently free.
>> + * @queue: Whether to allow the name request to be queued by D-Bus in
>> + * case it cannot be acquired now, rather than to return a
>> failure.
>> + * @callback: Callback to receive the request result when done.
>> + *
>> + * Acquire a well-known bus name (service name) on the bus.
>> + *
>> + * Returns: a non-zero request serial that can be passed to l_dbus_cancel
>> + * while waiting for the callback, or zero on failure.
>
>
> Yet kdbus version returns some hard-coded number that can't be used by
> l_dbus_cancel. I don't like this API asymmetry.
It can't be used with l_dbus_cancel because the callback has already
happened and the comment says specifically "while waiting for the
callback".
> Can't we just return void
> and let the callback parameters tell the application what happened?
The problems is the callback needs to be cancellable or I think this
will put a burden on users when they can't free their local state
immediately because they need to always wait for the callback. What
about passing the result through the callback and returning 0 from
this function if the callback was immediate?
Best regards
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/8] dbus: kdbus driver->name_acquire implementation
2016-03-24 2:07 [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Andrew Zaborowski
` (2 preceding siblings ...)
2016-03-24 2:07 ` [PATCH 4/8] dbus: l_dbus_name_acquire public API and driver declarations Andrew Zaborowski
@ 2016-03-24 2:07 ` Andrew Zaborowski
2016-03-24 2:07 ` [PATCH 6/8] dbus: Classic " Andrew Zaborowski
` (3 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2016-03-24 2:07 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]
---
ell/dbus-kernel.c | 13 ++++++++++++-
ell/dbus-private.h | 4 +++-
ell/dbus.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index b47f973..9bc99d7 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -626,7 +626,8 @@ int _dbus_kernel_recv(int fd, void *kdbus_pool,
return r;
}
-int _dbus_kernel_name_acquire(int fd, const char *name)
+int _dbus_kernel_name_acquire(int fd, const char *name, bool allow_replacement,
+ bool replace_existing, bool queue, bool *queued)
{
struct {
struct kdbus_cmd head;
@@ -651,9 +652,19 @@ int _dbus_kernel_name_acquire(int fd, const char *name)
item->type = KDBUS_ITEM_NAME;
strcpy(item->str, name);
+ if (replace_existing)
+ cmd_name.head.flags |= KDBUS_NAME_REPLACE_EXISTING;
+ if (allow_replacement)
+ cmd_name.head.flags |= KDBUS_NAME_ALLOW_REPLACEMENT;
+ if (queue)
+ cmd_name.head.flags |= KDBUS_NAME_QUEUE;
+
if (ioctl(fd, KDBUS_CMD_NAME_ACQUIRE, &cmd_name) < 0)
return -errno;
+ if (queued)
+ *queued = !!(cmd_name.head.flags & KDBUS_NAME_IN_QUEUE);
+
return 0;
}
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index b455df2..8309474 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -243,7 +243,9 @@ int _dbus_kernel_recv(int fd, void *kdbus_pool,
_dbus_name_owner_change_func_t name_owner_change_func,
void *user_data);
-int _dbus_kernel_name_acquire(int fd, const char *name);
+int _dbus_kernel_name_acquire(int fd, const char *name, bool allow_replacement,
+ bool replace_existing, bool queue,
+ bool *queued);
int _dbus_kernel_add_match(int fd, uint64_t bloom_size, uint64_t bloom_n_hash,
const struct _dbus_filter_condition *rule,
int rule_len, unsigned int id);
diff --git a/ell/dbus.c b/ell/dbus.c
index 1570d33..e79e23d 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -33,6 +33,7 @@
#include <string.h>
#include <sys/socket.h>
#include <sys/un.h>
+#include <errno.h>
#include "util.h"
#include "io.h"
@@ -1120,6 +1121,34 @@ static bool kdbus_get_name_owner(struct l_dbus *dbus, const char *name)
return true;
}
+static uint32_t kdbus_name_acquire(struct l_dbus *dbus, const char *name,
+ bool allow_replacement,
+ bool replace_existing, bool queue,
+ l_dbus_name_acquire_func_t callback,
+ void *user_data)
+{
+ int fd = l_io_get_fd(dbus->io);
+ bool queued = false;
+ int r;
+
+ r = _dbus_kernel_name_acquire(fd, name, allow_replacement,
+ replace_existing, queue, &queued);
+ if (r < 0 && r != -EALREADY) {
+ l_util_debug(dbus->debug_handler,
+ dbus->debug_data, strerror(-r));
+
+ if (callback)
+ callback(dbus, false, false, user_data);
+
+ return 0;
+ }
+
+ if (callback)
+ callback(dbus, true, queued, user_data);
+
+ return 1;
+}
+
static const struct l_dbus_ops kdbus_ops = {
.version = 2,
.free = kdbus_free,
@@ -1130,6 +1159,7 @@ static const struct l_dbus_ops kdbus_ops = {
.remove_match = kdbus_remove_match,
.get_name_owner = kdbus_get_name_owner,
},
+ .name_acquire = kdbus_name_acquire,
};
static struct l_dbus *setup_kdbus(int fd)
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 6/8] dbus: Classic driver->name_acquire implementation
2016-03-24 2:07 [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Andrew Zaborowski
` (3 preceding siblings ...)
2016-03-24 2:07 ` [PATCH 5/8] dbus: kdbus driver->name_acquire implementation Andrew Zaborowski
@ 2016-03-24 2:07 ` Andrew Zaborowski
2016-03-24 2:07 ` [PATCH 7/8] unit: Use l_dbus_name_acquire to acquire well-known name Andrew Zaborowski
` (2 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2016-03-24 2:07 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]
---
ell/dbus.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/ell/dbus.c b/ell/dbus.c
index e79e23d..a82fa9a 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -812,6 +812,84 @@ static bool classic_get_name_owner(struct l_dbus *bus, const char *name)
return true;
}
+struct name_request {
+ l_dbus_name_acquire_func_t callback;
+ void *user_data;
+ struct l_dbus *dbus;
+};
+
+enum dbus_name_flag {
+ DBUS_NAME_FLAG_ALLOW_REPLACEMENT = 0x1,
+ DBUS_NAME_FLAG_REPLACE_EXISTING = 0x2,
+ DBUS_NAME_FLAG_DO_NOT_QUEUE = 0x4,
+};
+
+enum dbus_name_reply {
+ DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER = 1,
+ DBUS_REQUEST_NAME_REPLY_IN_QUEUE = 2,
+ DBUS_REQUEST_NAME_REPLY_EXISTS = 3,
+ DBUS_REQUEST_NAME_REPLY_ALREADY_OWNER = 4,
+};
+
+static void request_name_reply_cb(struct l_dbus_message *reply, void *user_data)
+{
+ struct name_request *req = user_data;
+ bool success = false, queued = false;
+ uint32_t retval;
+
+ if (!req->callback)
+ return;
+
+ /* No name owner yet */
+ if (l_dbus_message_is_error(reply))
+ goto call_back;
+
+ /* Shouldn't happen */
+ if (!l_dbus_message_get_arguments(reply, "u", &retval))
+ goto call_back;
+
+ success = (retval == DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) ||
+ (retval == DBUS_REQUEST_NAME_REPLY_ALREADY_OWNER) ||
+ (retval == DBUS_REQUEST_NAME_REPLY_IN_QUEUE);
+ queued = (retval == DBUS_REQUEST_NAME_REPLY_IN_QUEUE);
+
+call_back:
+ req->callback(req->dbus, success, queued, req->user_data);
+}
+
+static uint32_t classic_name_acquire(struct l_dbus *dbus, const char *name,
+ bool allow_replacement,
+ bool replace_existing, bool queue,
+ l_dbus_name_acquire_func_t callback,
+ void *user_data)
+{
+ struct name_request *req;
+ struct l_dbus_message *message;
+ uint32_t flags = 0;
+
+ req = l_new(struct name_request, 1);
+ req->dbus = dbus;
+ req->user_data = user_data;
+ req->callback = callback;
+
+ message = l_dbus_message_new_method_call(dbus, DBUS_SERVICE_DBUS,
+ DBUS_PATH_DBUS,
+ L_DBUS_INTERFACE_DBUS,
+ "RequestName");
+
+ if (allow_replacement)
+ flags |= DBUS_NAME_FLAG_ALLOW_REPLACEMENT;
+ if (replace_existing)
+ flags |= DBUS_NAME_FLAG_REPLACE_EXISTING;
+ if (!queue)
+ flags |= DBUS_NAME_FLAG_DO_NOT_QUEUE;
+
+ l_dbus_message_set_arguments(message, "su", name, flags);
+
+ return send_message(dbus, false, message, request_name_reply_cb,
+ req, free);
+}
+
static const struct l_dbus_ops classic_ops = {
.version = 1,
.send_message = classic_send_message,
@@ -822,6 +900,7 @@ static const struct l_dbus_ops classic_ops = {
.remove_match = classic_remove_match,
.get_name_owner = classic_get_name_owner,
},
+ .name_acquire = classic_name_acquire,
};
static void name_owner_changed_cb(struct l_dbus_message *message,
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 7/8] unit: Use l_dbus_name_acquire to acquire well-known name
2016-03-24 2:07 [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Andrew Zaborowski
` (4 preceding siblings ...)
2016-03-24 2:07 ` [PATCH 6/8] dbus: Classic " Andrew Zaborowski
@ 2016-03-24 2:07 ` Andrew Zaborowski
2016-03-24 2:07 ` [PATCH 8/8] examples: " Andrew Zaborowski
2016-03-25 2:55 ` [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Denis Kenzior
7 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2016-03-24 2:07 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2754 bytes --]
---
unit/test-dbus-properties.c | 36 ++++++------------------------------
unit/test-kdbus.c | 14 +++++++++-----
2 files changed, 15 insertions(+), 35 deletions(-)
diff --git a/unit/test-dbus-properties.c b/unit/test-dbus-properties.c
index be91dd3..c7ed384 100644
--- a/unit/test-dbus-properties.c
+++ b/unit/test-dbus-properties.c
@@ -152,32 +152,11 @@ static void test_next()
} \
} while (0)
-static void request_name_setup(struct l_dbus_message *message, void *user_data)
+static void request_name_callback(struct l_dbus *dbus, bool success,
+ bool queued, void *user_data)
{
- const char *name = "org.test";
-
- l_dbus_message_set_arguments(message, "su", name, 0);
-}
-
-static void request_name_callback(struct l_dbus_message *message,
- void *user_data)
-{
- const char *error, *text;
- uint32_t result;
-
- if (l_dbus_message_get_error(message, &error, &text)) {
- l_error("error=%s", error);
- l_error("message=%s", text);
- l_main_quit();
- return;
- }
-
- if (!l_dbus_message_get_arguments(message, "u", &result)) {
- l_main_quit();
- return;
- }
-
- l_info("request name result=%d", result);
+ l_info("request name result=%s",
+ success ? (queued ? "queued" : "success") : "failed");
test_next();
}
@@ -938,11 +917,8 @@ int main(int argc, char *argv[])
l_dbus_set_ready_handler(dbus, ready_callback, dbus, NULL);
l_dbus_set_disconnect_handler(dbus, disconnect_callback, NULL, NULL);
- l_dbus_method_call(dbus, "org.freedesktop.DBus",
- "/org/freedesktop/DBus",
- "org.freedesktop.DBus", "RequestName",
- request_name_setup,
- request_name_callback, NULL, NULL);
+ l_dbus_name_acquire(dbus, "org.test", false, false, false,
+ request_name_callback, NULL);
if (!l_dbus_register_interface(dbus, "org.test", setup_test_interface,
NULL, true)) {
diff --git a/unit/test-kdbus.c b/unit/test-kdbus.c
index dde2596..8cac761 100644
--- a/unit/test-kdbus.c
+++ b/unit/test-kdbus.c
@@ -97,16 +97,20 @@ static void client_ready_callback(void *user_data)
NULL, NULL, NULL);
}
+static void service_name_acquire_callback(struct l_dbus *dbus, bool success,
+ bool queued, void *user_data)
+{
+ if (!success)
+ l_info("Failed to acquire name");
+}
+
static void service_ready_callback(void *user_data)
{
struct l_dbus *dbus = user_data;
struct l_dbus_message *message;
- int fd = _dbus_get_fd(dbus);
- int r;
- r = _dbus_kernel_name_acquire(fd, "org.test");
- if (r < 0)
- l_info("Failed to acquire name: %s", strerror(-r));
+ l_dbus_name_acquire(dbus, "org.test", false, false, false,
+ service_name_acquire_callback, NULL);
l_info("service ready");
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 8/8] examples: Use l_dbus_name_acquire to acquire well-known name
2016-03-24 2:07 [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Andrew Zaborowski
` (5 preceding siblings ...)
2016-03-24 2:07 ` [PATCH 7/8] unit: Use l_dbus_name_acquire to acquire well-known name Andrew Zaborowski
@ 2016-03-24 2:07 ` Andrew Zaborowski
2016-03-25 2:55 ` [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Denis Kenzior
7 siblings, 0 replies; 16+ messages in thread
From: Andrew Zaborowski @ 2016-03-24 2:07 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]
---
examples/dbus-service.c | 33 ++++++---------------------------
1 file changed, 6 insertions(+), 27 deletions(-)
diff --git a/examples/dbus-service.c b/examples/dbus-service.c
index 347faae..51b7e4f 100644
--- a/examples/dbus-service.c
+++ b/examples/dbus-service.c
@@ -50,29 +50,11 @@ static void signal_handler(struct l_signal *signal, uint32_t signo,
}
}
-static void request_name_setup(struct l_dbus_message *message, void *user_data)
+static void request_name_callback(struct l_dbus *dbus, bool success,
+ bool queued, void *user_data)
{
- const char *name = "org.test";
-
- l_dbus_message_set_arguments(message, "su", name, 0);
-}
-
-static void request_name_callback(struct l_dbus_message *message,
- void *user_data)
-{
- const char *error, *text;
- uint32_t result;
-
- if (l_dbus_message_get_error(message, &error, &text)) {
- l_error("error=%s", error);
- l_error("message=%s", text);
- return;
- }
-
- if (!l_dbus_message_get_arguments(message, "u", &result))
- return;
-
- l_info("request name result=%d", result);
+ l_info("request name result=%s",
+ success ? (queued ? "queued" : "success") : "failed");
}
static void ready_callback(void *user_data)
@@ -211,11 +193,8 @@ int main(int argc, char *argv[])
l_dbus_set_ready_handler(dbus, ready_callback, dbus, NULL);
l_dbus_set_disconnect_handler(dbus, disconnect_callback, NULL, NULL);
- l_dbus_method_call(dbus, "org.freedesktop.DBus",
- "/org/freedesktop/DBus",
- L_DBUS_INTERFACE_DBUS, "RequestName",
- request_name_setup,
- request_name_callback, NULL, NULL);
+ l_dbus_name_acquire(dbus, "org.test", false, false, false,
+ request_name_callback, NULL);
if (!l_dbus_object_manager_enable(dbus)) {
l_info("Unable to enable Object Manager");
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API
2016-03-24 2:07 [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Andrew Zaborowski
` (6 preceding siblings ...)
2016-03-24 2:07 ` [PATCH 8/8] examples: " Andrew Zaborowski
@ 2016-03-25 2:55 ` Denis Kenzior
2016-03-26 0:06 ` Andrzej Zaborowski
7 siblings, 1 reply; 16+ messages in thread
From: Denis Kenzior @ 2016-03-25 2:55 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 11582 bytes --]
Hi Andrew,
On 03/23/2016 09:07 PM, Andrew Zaborowski wrote:
> The two advantages are that this should now work on top of kdbus and
> that the full list of watches is not traversed on every notification.
> ---
> ell/dbus.c | 220 ++++++++++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 153 insertions(+), 67 deletions(-)
>
> diff --git a/ell/dbus.c b/ell/dbus.c
> index d976496..6bf5734 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -89,6 +89,10 @@ struct l_dbus {
> void *debug_data;
> struct _dbus_object_tree *tree;
> struct _dbus_filter *filter;
> + struct l_hashmap *services_watched;
> + struct service_watch_data *service_watch_head;
> + unsigned int last_service_watch_id;
> + struct l_idle *service_watch_remove_work;
>
> const struct l_dbus_ops *driver;
> };
> @@ -123,6 +127,18 @@ struct signal_callback {
> void *user_data;
> };
>
> +struct service_watch_data {
> + char *bus_name;
> + l_dbus_destroy_func_t destroy_func;
> + l_dbus_watch_func_t connect_func;
> + l_dbus_watch_func_t disconnect_func;
> + void *user_data;
> + unsigned int id;
> + bool pending_get_name_owner;
> + struct service_watch_data *next;
> + bool removed;
> +};
> +
> struct dbus1_filter_data {
> struct l_dbus *dbus;
> char *sender;
> @@ -363,6 +379,35 @@ static void bus_ready(struct l_dbus *dbus)
> l_io_set_write_handler(dbus->io, message_write_handler, dbus, NULL);
> }
>
> +static void name_owner_notify(struct l_dbus *dbus, const char *name,
> + const char *old, const char *new)
> +{
> + struct service_watch_data *watch;
> + bool connect = old && new && !*old && *new;
> + bool disconnect = old && new && *old && !*new;
This makes my brain hurt. I also don't think this works properly. Half
the functions call name_owner_notify with empty strings and the other
half calls it with NULLs.
The disconnect can be expressed much simpler as *new == '\0'.
> +
> + _dbus_filter_name_owner_notify(dbus->filter, name, new);
> +
> + watch = l_hashmap_lookup(dbus->services_watched, name);
> +
> + while (watch && !strcmp(watch->bus_name, name)) {
> + if (watch->removed)
> + goto next;
> +
> + if (watch->pending_get_name_owner) {
> + watch->pending_get_name_owner = false;
> + if (new && *new)
> + watch->connect_func(dbus, watch->user_data);
> + } else if (connect && watch->connect_func)
> + watch->connect_func(dbus, watch->user_data);
> + else if (disconnect && watch->disconnect_func)
> + watch->disconnect_func(dbus, watch->user_data);
> +
> +next:
> + watch = watch->next;
> + }
> +}
> +
> static void hello_callback(struct l_dbus_message *message, void *user_data)
> {
> struct l_dbus *dbus = user_data;
> @@ -752,7 +797,7 @@ static void get_name_owner_reply_cb(struct l_dbus_message *reply,
> if (!l_dbus_message_get_arguments(req->message, "s", &name))
> return;
>
> - _dbus_filter_name_owner_notify(req->dbus->filter, name, owner);
> + name_owner_notify(req->dbus, name, NULL, owner);
should this use "" instead of NULL?
> }
>
> static bool classic_get_name_owner(struct l_dbus *bus, const char *name)
> @@ -800,7 +845,7 @@ static void name_owner_changed_cb(struct l_dbus_message *message,
> if (!l_dbus_message_get_arguments(message, "sss", &name, &old, &new))
> return;
>
> - _dbus_filter_name_owner_notify(dbus->filter, name, new);
> + name_owner_notify(dbus, name, old, new);
> }
>
> static struct l_dbus *setup_dbus1(int fd, const char *guid)
> @@ -1000,15 +1045,22 @@ static void kdbus_name_owner_change_func(const char *name, uint64_t old_owner,
> void *user_data)
> {
> struct kdbus_message_recv_data *recv_data = user_data;
> - char owner[32];
> + char old[32], new[32];
>
> l_util_debug(recv_data->dbus->debug_handler,
> recv_data->dbus->debug_data,
> "Read KDBUS Name Owner Change notification");
>
> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
> + if (old_owner)
> + snprintf(old, sizeof(old), ":1.%" PRIu64, new_owner);
old_owner here, not new_owner?
> + else
> + *old = '\0';
> + if (new_owner)
> + snprintf(new, sizeof(new), ":1.%" PRIu64, new_owner);
> + else
> + *new = '\0';
>
> - _dbus_filter_name_owner_notify(recv_data->dbus->filter, name, owner);
> + name_owner_notify(recv_data->dbus, name, old, new);
> }
>
> static struct l_dbus_message *kdbus_recv_message(struct l_dbus *dbus)
> @@ -1070,15 +1122,13 @@ static bool kdbus_get_name_owner(struct l_dbus *dbus, const char *name)
> char owner[32];
>
> owner_id = _dbus_kernel_get_name_owner(fd, kdbus->kdbus_pool, name);
> - if (!owner_id) {
> - l_util_debug(dbus->debug_handler,
> - dbus->debug_data, "Error getting name owner");
> - return false;
> - }
>
> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
> + if (owner_id)
> + snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
> + else
> + *owner = '\0';
>
> - _dbus_filter_name_owner_notify(dbus->filter, name, owner);
> + name_owner_notify(dbus, name, NULL, owner);
"" instead of NULL?
>
> return true;
> }
> @@ -1222,6 +1272,17 @@ LIB_EXPORT struct l_dbus *l_dbus_new_default(enum l_dbus_bus bus)
> return setup_address(address);
> }
>
> +static void service_watch_data_free(void *data)
> +{
> + struct service_watch_data *watch = data;
> +
> + if (watch->destroy_func)
> + watch->destroy_func(watch->user_data);
> +
> + l_free(watch->bus_name);
> + l_free(watch);
> +}
> +
> LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
> {
> if (unlikely(!dbus))
> @@ -1236,6 +1297,11 @@ LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
> l_hashmap_destroy(dbus->message_list, message_list_destroy);
> l_queue_destroy(dbus->message_queue, message_queue_destroy);
>
> + l_hashmap_destroy(dbus->services_watched, service_watch_data_free);
> +
> + if (dbus->service_watch_remove_work)
> + l_idle_remove(dbus->service_watch_remove_work);
> +
> l_io_destroy(dbus->io);
>
> if (dbus->disconnect_destroy)
> @@ -1809,33 +1875,6 @@ void _dbus1_name_owner_changed_filter(struct l_dbus_message *message,
> }
> }
>
> -static void _dbus1_get_name_owner_reply(struct l_dbus_message *message,
> - void *user_data)
> -{
> - struct dbus1_filter_data *data = user_data;
> - const char *name;
> -
> - data->get_name_owner_id = 0;
> -
> - /* No name owner yet */
> - if (l_dbus_message_is_error(message))
> - return;
> -
> - /* Shouldn't happen */
> - if (!l_dbus_message_get_arguments(message, "s", &name))
> - return;
> -
> - /* If the signal arrived before the reply, don't do anything */
> - if (data->owner && !strcmp(data->owner, name))
> - return;
> -
> - l_free(data->owner);
> - data->owner = l_strdup(name);
> -
> - if (data->connect_func)
> - data->connect_func(data->dbus, data->user_data);
> -}
> -
> LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
> const char *name,
> l_dbus_watch_func_t disconnect_func,
> @@ -1846,56 +1885,103 @@ LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
> user_data, destroy);
> }
>
> -unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
> +LIB_EXPORT unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
> const char *name,
> l_dbus_watch_func_t connect_func,
> l_dbus_watch_func_t disconnect_func,
> void *user_data,
> l_dbus_destroy_func_t destroy)
> {
> - struct dbus1_filter_data *data;
> + struct service_watch_data *watch, *first;
>
> if (!name)
> return 0;
>
> - data = _dbus1_filter_data_get(dbus, _dbus1_name_owner_changed_filter,
> - DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
> - L_DBUS_INTERFACE_DBUS, "NameOwnerChanged",
> - name,
> - disconnect_func,
> - user_data,
> - destroy);
> - if (!data)
> - return 0;
> + if (!dbus->services_watched)
> + dbus->services_watched = l_hashmap_string_new();
> +
> + watch = l_new(struct service_watch_data, 1);
> + watch->bus_name = l_strdup(name);
> + watch->id = ++dbus->last_service_watch_id;
> + watch->connect_func = connect_func;
> + watch->disconnect_func = disconnect_func;
> + watch->destroy_func = destroy;
> +
> + /*
> + * Make sure that all the entries for the same bus name are
> + * grouped together on the list and that the first entry for the
> + * name is in the hashmap for easy lookups.
> + */
> + first = l_hashmap_lookup(dbus->services_watched, name);
> + if (first) {
> + watch->next = first->next;
> + first->next = watch;
> + } else {
> + l_hashmap_insert(dbus->services_watched, name, watch);
>
> - add_match(data);
> + watch->next = dbus->service_watch_head;
> + dbus->service_watch_head = watch;
> + }
>
> if (connect_func) {
> - struct l_dbus_message *message;
> + watch->pending_get_name_owner = true;
>
> - data->connect_func = connect_func;
> + dbus->driver->filter_ops.get_name_owner(dbus, name);
> + }
>
> - message = l_dbus_message_new_method_call(dbus,
> - DBUS_SERVICE_DBUS,
> - DBUS_PATH_DBUS,
> - L_DBUS_INTERFACE_DBUS,
> - "GetNameOwner");
> + return watch->id;
> +}
>
> - l_dbus_message_set_arguments(message, "s", name);
> +static void service_watch_remove(struct l_idle *idle, void *user_data)
> +{
> + struct l_dbus *dbus = user_data;
> + struct service_watch_data **watch, *tmp;
>
> - data->get_name_owner_id =
> - l_dbus_send_with_reply(dbus, message,
> - _dbus1_get_name_owner_reply,
> - data, NULL);
> - }
> + l_idle_remove(dbus->service_watch_remove_work);
> + dbus->service_watch_remove_work = NULL;
> +
> + for (watch = &dbus->service_watch_head; *watch;
> + watch = &(*watch)->next) {
> + if (!(*watch)->removed)
> + continue;
> +
> + tmp = *watch;
> + *watch = tmp->next;
> +
> + /* Check if this was the first entry for the bus name */
> + if (l_hashmap_lookup(dbus->services_watched, tmp->bus_name) ==
> + tmp) {
> + l_hashmap_remove(dbus->services_watched, tmp->bus_name);
> +
> + if (tmp->next && !strcmp(tmp->bus_name,
> + tmp->next->bus_name))
> + l_hashmap_insert(dbus->services_watched,
> + tmp->bus_name, tmp->next);
I really wonder if we should just add l_hashmap_find or something here
and avoid the linked-list on the side. It feels a bit kludgy to me.
Alternatively, let the user remove the service watch by name instead of
messing with ids.
> + }
>
> - return l_dbus_register(dbus, _dbus1_signal_dispatcher, data,
> - filter_data_destroy);
> + service_watch_data_free(tmp);
> + }
> }
>
> LIB_EXPORT bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int id)
> {
> - return l_dbus_unregister(dbus, id);
> + struct service_watch_data *watch;
> +
> + for (watch = dbus->service_watch_head; watch; watch = watch->next)
> + if (watch->id == id && !watch->removed)
> + break;
> +
> + if (!watch)
> + return false;
> +
> + watch->removed = true;
Do you also want to make sure that the callbacks are not called by
setting them to NULL or something? Otherwise you can remove a watch,
and it will still be called due to pending events...
> +
> + if (!dbus->service_watch_remove_work)
> + dbus->service_watch_remove_work = l_idle_create(
> + service_watch_remove,
> + dbus, NULL);
> +
> + return true;
> }
>
> /**
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API
2016-03-25 2:55 ` [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API Denis Kenzior
@ 2016-03-26 0:06 ` Andrzej Zaborowski
2016-03-26 3:01 ` Denis Kenzior
0 siblings, 1 reply; 16+ messages in thread
From: Andrzej Zaborowski @ 2016-03-26 0:06 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 13920 bytes --]
Hi Denis,
On 25 March 2016 at 03:55, Denis Kenzior <denkenz@gmail.com> wrote:
> On 03/23/2016 09:07 PM, Andrew Zaborowski wrote:
>> @@ -363,6 +379,35 @@ static void bus_ready(struct l_dbus *dbus)
>> l_io_set_write_handler(dbus->io, message_write_handler, dbus,
>> NULL);
>> }
>>
>> +static void name_owner_notify(struct l_dbus *dbus, const char *name,
>> + const char *old, const char *new)
>> +{
>> + struct service_watch_data *watch;
>> + bool connect = old && new && !*old && *new;
>> + bool disconnect = old && new && *old && !*new;
>
>
> This makes my brain hurt. I also don't think this works properly. Half the
> functions call name_owner_notify with empty strings and the other half calls
> it with NULLs.
>
> The disconnect can be expressed much simpler as *new == '\0'.
Disconnects yes, the NULLs are a different case where we don't know
the old value. We can't do *old == '\0' without checking that it
isn't NULL.
But, I can change this function's signature to be something like void
name_owner_notify(dbus, const char *name, bool connect, bool
disconnect, const char *owner)
>
>
>> +
>> + _dbus_filter_name_owner_notify(dbus->filter, name, new);
>> +
>> + watch = l_hashmap_lookup(dbus->services_watched, name);
>> +
>> + while (watch && !strcmp(watch->bus_name, name)) {
>> + if (watch->removed)
>> + goto next;
>> +
>> + if (watch->pending_get_name_owner) {
>> + watch->pending_get_name_owner = false;
>> + if (new && *new)
>> + watch->connect_func(dbus,
>> watch->user_data);
>> + } else if (connect && watch->connect_func)
>> + watch->connect_func(dbus, watch->user_data);
>> + else if (disconnect && watch->disconnect_func)
>> + watch->disconnect_func(dbus, watch->user_data);
>> +
>> +next:
>> + watch = watch->next;
>> + }
>> +}
>> +
>> static void hello_callback(struct l_dbus_message *message, void
>> *user_data)
>> {
>> struct l_dbus *dbus = user_data;
>> @@ -752,7 +797,7 @@ static void get_name_owner_reply_cb(struct
>> l_dbus_message *reply,
>> if (!l_dbus_message_get_arguments(req->message, "s", &name))
>> return;
>>
>> - _dbus_filter_name_owner_notify(req->dbus->filter, name, owner);
>> + name_owner_notify(req->dbus, name, NULL, owner);
>
>
> should this use "" instead of NULL?
If we pass "" then name_owner_notify will have to assume this is a new
connection and will issue spurious connect calls. The NULL here says
to not treat this as new connection or disconnection.
>
>> }
>>
>> static bool classic_get_name_owner(struct l_dbus *bus, const char *name)
>> @@ -800,7 +845,7 @@ static void name_owner_changed_cb(struct
>> l_dbus_message *message,
>> if (!l_dbus_message_get_arguments(message, "sss", &name, &old,
>> &new))
>> return;
>>
>> - _dbus_filter_name_owner_notify(dbus->filter, name, new);
>> + name_owner_notify(dbus, name, old, new);
>> }
>>
>> static struct l_dbus *setup_dbus1(int fd, const char *guid)
>> @@ -1000,15 +1045,22 @@ static void kdbus_name_owner_change_func(const
>> char *name, uint64_t old_owner,
>> void *user_data)
>> {
>> struct kdbus_message_recv_data *recv_data = user_data;
>> - char owner[32];
>> + char old[32], new[32];
>>
>> l_util_debug(recv_data->dbus->debug_handler,
>> recv_data->dbus->debug_data,
>> "Read KDBUS Name Owner Change notification");
>>
>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
>> + if (old_owner)
>> + snprintf(old, sizeof(old), ":1.%" PRIu64, new_owner);
>
>
> old_owner here, not new_owner?
Yes, thanks for catching this.
>
>> + else
>> + *old = '\0';
>> + if (new_owner)
>> + snprintf(new, sizeof(new), ":1.%" PRIu64, new_owner);
>> + else
>> + *new = '\0';
>>
>> - _dbus_filter_name_owner_notify(recv_data->dbus->filter, name,
>> owner);
>> + name_owner_notify(recv_data->dbus, name, old, new);
>> }
>>
>> static struct l_dbus_message *kdbus_recv_message(struct l_dbus *dbus)
>> @@ -1070,15 +1122,13 @@ static bool kdbus_get_name_owner(struct l_dbus
>> *dbus, const char *name)
>> char owner[32];
>>
>> owner_id = _dbus_kernel_get_name_owner(fd, kdbus->kdbus_pool,
>> name);
>> - if (!owner_id) {
>> - l_util_debug(dbus->debug_handler,
>> - dbus->debug_data, "Error getting name
>> owner");
>> - return false;
>> - }
>>
>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>> + if (owner_id)
>> + snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>> + else
>> + *owner = '\0';
>>
>> - _dbus_filter_name_owner_notify(dbus->filter, name, owner);
>> + name_owner_notify(dbus, name, NULL, owner);
>
>
> "" instead of NULL?
>
>
>>
>> return true;
>> }
>> @@ -1222,6 +1272,17 @@ LIB_EXPORT struct l_dbus *l_dbus_new_default(enum
>> l_dbus_bus bus)
>> return setup_address(address);
>> }
>>
>> +static void service_watch_data_free(void *data)
>> +{
>> + struct service_watch_data *watch = data;
>> +
>> + if (watch->destroy_func)
>> + watch->destroy_func(watch->user_data);
>> +
>> + l_free(watch->bus_name);
>> + l_free(watch);
>> +}
>> +
>> LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
>> {
>> if (unlikely(!dbus))
>> @@ -1236,6 +1297,11 @@ LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
>> l_hashmap_destroy(dbus->message_list, message_list_destroy);
>> l_queue_destroy(dbus->message_queue, message_queue_destroy);
>>
>> + l_hashmap_destroy(dbus->services_watched,
>> service_watch_data_free);
>> +
>> + if (dbus->service_watch_remove_work)
>> + l_idle_remove(dbus->service_watch_remove_work);
>> +
>> l_io_destroy(dbus->io);
>>
>> if (dbus->disconnect_destroy)
>> @@ -1809,33 +1875,6 @@ void _dbus1_name_owner_changed_filter(struct
>> l_dbus_message *message,
>> }
>> }
>>
>> -static void _dbus1_get_name_owner_reply(struct l_dbus_message *message,
>> - void *user_data)
>> -{
>> - struct dbus1_filter_data *data = user_data;
>> - const char *name;
>> -
>> - data->get_name_owner_id = 0;
>> -
>> - /* No name owner yet */
>> - if (l_dbus_message_is_error(message))
>> - return;
>> -
>> - /* Shouldn't happen */
>> - if (!l_dbus_message_get_arguments(message, "s", &name))
>> - return;
>> -
>> - /* If the signal arrived before the reply, don't do anything */
>> - if (data->owner && !strcmp(data->owner, name))
>> - return;
>> -
>> - l_free(data->owner);
>> - data->owner = l_strdup(name);
>> -
>> - if (data->connect_func)
>> - data->connect_func(data->dbus, data->user_data);
>> -}
>> -
>> LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
>> const char *name,
>> l_dbus_watch_func_t
>> disconnect_func,
>> @@ -1846,56 +1885,103 @@ LIB_EXPORT unsigned int
>> l_dbus_add_disconnect_watch(struct l_dbus *dbus,
>> user_data, destroy);
>> }
>>
>> -unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
>> +LIB_EXPORT unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
>> const char *name,
>> l_dbus_watch_func_t connect_func,
>> l_dbus_watch_func_t
>> disconnect_func,
>> void *user_data,
>> l_dbus_destroy_func_t destroy)
>> {
>> - struct dbus1_filter_data *data;
>> + struct service_watch_data *watch, *first;
>>
>> if (!name)
>> return 0;
>>
>> - data = _dbus1_filter_data_get(dbus,
>> _dbus1_name_owner_changed_filter,
>> - DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
>> - L_DBUS_INTERFACE_DBUS, "NameOwnerChanged",
>> - name,
>> - disconnect_func,
>> - user_data,
>> - destroy);
>> - if (!data)
>> - return 0;
>> + if (!dbus->services_watched)
>> + dbus->services_watched = l_hashmap_string_new();
>> +
>> + watch = l_new(struct service_watch_data, 1);
>> + watch->bus_name = l_strdup(name);
>> + watch->id = ++dbus->last_service_watch_id;
>> + watch->connect_func = connect_func;
>> + watch->disconnect_func = disconnect_func;
>> + watch->destroy_func = destroy;
>> +
>> + /*
>> + * Make sure that all the entries for the same bus name are
>> + * grouped together on the list and that the first entry for the
>> + * name is in the hashmap for easy lookups.
>> + */
>> + first = l_hashmap_lookup(dbus->services_watched, name);
>> + if (first) {
>> + watch->next = first->next;
>> + first->next = watch;
>> + } else {
>> + l_hashmap_insert(dbus->services_watched, name, watch);
>>
>> - add_match(data);
>> + watch->next = dbus->service_watch_head;
>> + dbus->service_watch_head = watch;
>> + }
>>
>> if (connect_func) {
>> - struct l_dbus_message *message;
>> + watch->pending_get_name_owner = true;
>>
>> - data->connect_func = connect_func;
>> + dbus->driver->filter_ops.get_name_owner(dbus, name);
>> + }
>>
>> - message = l_dbus_message_new_method_call(dbus,
>> - DBUS_SERVICE_DBUS,
>> - DBUS_PATH_DBUS,
>> -
>> L_DBUS_INTERFACE_DBUS,
>> - "GetNameOwner");
>> + return watch->id;
>> +}
>>
>> - l_dbus_message_set_arguments(message, "s", name);
>> +static void service_watch_remove(struct l_idle *idle, void *user_data)
>> +{
>> + struct l_dbus *dbus = user_data;
>> + struct service_watch_data **watch, *tmp;
>>
>> - data->get_name_owner_id =
>> - l_dbus_send_with_reply(dbus, message,
>> -
>> _dbus1_get_name_owner_reply,
>> - data, NULL);
>> - }
>> + l_idle_remove(dbus->service_watch_remove_work);
>> + dbus->service_watch_remove_work = NULL;
>> +
>> + for (watch = &dbus->service_watch_head; *watch;
>> + watch = &(*watch)->next) {
>> + if (!(*watch)->removed)
>> + continue;
>> +
>> + tmp = *watch;
>> + *watch = tmp->next;
>> +
>> + /* Check if this was the first entry for the bus name */
>> + if (l_hashmap_lookup(dbus->services_watched,
>> tmp->bus_name) ==
>> + tmp) {
>> + l_hashmap_remove(dbus->services_watched,
>> tmp->bus_name);
>> +
>> + if (tmp->next && !strcmp(tmp->bus_name,
>> + tmp->next->bus_name))
>> + l_hashmap_insert(dbus->services_watched,
>> + tmp->bus_name, tmp->next);
>
>
> I really wonder if we should just add l_hashmap_find or something here and
> avoid the linked-list on the side. It feels a bit kludgy to me.
> Alternatively, let the user remove the service watch by name instead of
> messing with ids.
They'd have to also pass the callbacks and/or user_data to make sure
we don't remove other watches for the same service.
Do you think it would be cleaner if we mapped the service names to
individual, normal linked lists instead of having one linked list? I
noticed afterwards that this wouldn't be any worse than what I did
here.
>
>> + }
>>
>> - return l_dbus_register(dbus, _dbus1_signal_dispatcher, data,
>> -
>> filter_data_destroy);
>> + service_watch_data_free(tmp);
>> + }
>> }
>>
>> LIB_EXPORT bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int
>> id)
>> {
>> - return l_dbus_unregister(dbus, id);
>> + struct service_watch_data *watch;
>> +
>> + for (watch = dbus->service_watch_head; watch; watch = watch->next)
>> + if (watch->id == id && !watch->removed)
>> + break;
>> +
>> + if (!watch)
>> + return false;
>> +
>> + watch->removed = true;
>
>
> Do you also want to make sure that the callbacks are not called by setting
> them to NULL or something? Otherwise you can remove a watch, and it will
> still be called due to pending events...
name_owner_notify should already skip over entries that have removed == true.
Best regards
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API
2016-03-26 0:06 ` Andrzej Zaborowski
@ 2016-03-26 3:01 ` Denis Kenzior
2016-03-26 15:50 ` Andrzej Zaborowski
0 siblings, 1 reply; 16+ messages in thread
From: Denis Kenzior @ 2016-03-26 3:01 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 15787 bytes --]
Hi Andrew,
On 03/25/2016 07:06 PM, Andrzej Zaborowski wrote:
> Hi Denis,
>
> On 25 March 2016 at 03:55, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 03/23/2016 09:07 PM, Andrew Zaborowski wrote:
>>> @@ -363,6 +379,35 @@ static void bus_ready(struct l_dbus *dbus)
>>> l_io_set_write_handler(dbus->io, message_write_handler, dbus,
>>> NULL);
>>> }
>>>
>>> +static void name_owner_notify(struct l_dbus *dbus, const char *name,
>>> + const char *old, const char *new)
>>> +{
>>> + struct service_watch_data *watch;
>>> + bool connect = old && new && !*old && *new;
>>> + bool disconnect = old && new && *old && !*new;
>>
>>
>> This makes my brain hurt. I also don't think this works properly. Half the
>> functions call name_owner_notify with empty strings and the other half calls
>> it with NULLs.
>>
>> The disconnect can be expressed much simpler as *new == '\0'.
>
> Disconnects yes, the NULLs are a different case where we don't know
> the old value. We can't do *old == '\0' without checking that it
> isn't NULL.
>
> But, I can change this function's signature to be something like void
> name_owner_notify(dbus, const char *name, bool connect, bool
> disconnect, const char *owner)
>
Nah, the signature is fine. I think you need to make the flow a bit
simpler. The current logic listens to NameOwnerChanged and also queues
up the GetNameOwner call. If the NameOwnerChanged signal wins the race,
GetNameOwner call is canceled, connect is called. If GetNameOwner
returns first, connect is called and subsequent NameOwnerChanged (if it
arrives at all) doesn't do anything.
You try to do something similar with pending_get_name_owner flag. So
lets use it to our advantage...
>>
>>
>>> +
>>> + _dbus_filter_name_owner_notify(dbus->filter, name, new);
>>> +
>>> + watch = l_hashmap_lookup(dbus->services_watched, name);
>>> +
>>> + while (watch && !strcmp(watch->bus_name, name)) {
>>> + if (watch->removed)
>>> + goto next;
>>> +
>>> + if (watch->pending_get_name_owner) {
>>> + watch->pending_get_name_owner = false;
>>> + if (new && *new)
>>> + watch->connect_func(dbus,
>>> watch->user_data);
In case new is non-null, reset pending_get_name_owner to false and call
connect.
>>> + } else if (connect && watch->connect_func)
>>> + watch->connect_func(dbus, watch->user_data);
>>> + else if (disconnect && watch->disconnect_func)
>>> + watch->disconnect_func(dbus, watch->user_data);
>>> +
>>> +next:
>>> + watch = watch->next;
>>> + }
>>> +}
>>> +
>>> static void hello_callback(struct l_dbus_message *message, void
>>> *user_data)
>>> {
>>> struct l_dbus *dbus = user_data;
>>> @@ -752,7 +797,7 @@ static void get_name_owner_reply_cb(struct
>>> l_dbus_message *reply,
>>> if (!l_dbus_message_get_arguments(req->message, "s", &name))
>>> return;
>>>
>>> - _dbus_filter_name_owner_notify(req->dbus->filter, name, owner);
>>> + name_owner_notify(req->dbus, name, NULL, owner);
Then here, if pending_get_name_owner is false, simply return.
get_name_owner_reply lost the race.
Otherwise, just call with "" instead of NULL and simplify your life in
name_owner_notify.
>>
>>
>> should this use "" instead of NULL?
>
> If we pass "" then name_owner_notify will have to assume this is a new
> connection and will issue spurious connect calls. The NULL here says
> to not treat this as new connection or disconnection.
>
And by the way, we might want to handle the case where a service name is
changed from one unique name to another. E.g. due to RequestName being
queued or DBUS_NAME_FLAG_REPLACE_EXISTING somehow succeeding so that the
name is passed from one service to another.
For our purposes, calling disconnect and then immediately calling
connect would probably be okay for now.
>>
>>> }
>>>
>>> static bool classic_get_name_owner(struct l_dbus *bus, const char *name)
>>> @@ -800,7 +845,7 @@ static void name_owner_changed_cb(struct
>>> l_dbus_message *message,
>>> if (!l_dbus_message_get_arguments(message, "sss", &name, &old,
>>> &new))
>>> return;
>>>
>>> - _dbus_filter_name_owner_notify(dbus->filter, name, new);
>>> + name_owner_notify(dbus, name, old, new);
>>> }
>>>
>>> static struct l_dbus *setup_dbus1(int fd, const char *guid)
>>> @@ -1000,15 +1045,22 @@ static void kdbus_name_owner_change_func(const
>>> char *name, uint64_t old_owner,
>>> void *user_data)
>>> {
>>> struct kdbus_message_recv_data *recv_data = user_data;
>>> - char owner[32];
>>> + char old[32], new[32];
>>>
>>> l_util_debug(recv_data->dbus->debug_handler,
>>> recv_data->dbus->debug_data,
>>> "Read KDBUS Name Owner Change notification");
>>>
>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
>>> + if (old_owner)
>>> + snprintf(old, sizeof(old), ":1.%" PRIu64, new_owner);
>>
>>
>> old_owner here, not new_owner?
>
> Yes, thanks for catching this.
>
>>
>>> + else
>>> + *old = '\0';
>>> + if (new_owner)
>>> + snprintf(new, sizeof(new), ":1.%" PRIu64, new_owner);
>>> + else
>>> + *new = '\0';
>>>
>>> - _dbus_filter_name_owner_notify(recv_data->dbus->filter, name,
>>> owner);
>>> + name_owner_notify(recv_data->dbus, name, old, new);
>>> }
>>>
>>> static struct l_dbus_message *kdbus_recv_message(struct l_dbus *dbus)
>>> @@ -1070,15 +1122,13 @@ static bool kdbus_get_name_owner(struct l_dbus
>>> *dbus, const char *name)
>>> char owner[32];
>>>
>>> owner_id = _dbus_kernel_get_name_owner(fd, kdbus->kdbus_pool,
>>> name);
>>> - if (!owner_id) {
>>> - l_util_debug(dbus->debug_handler,
>>> - dbus->debug_data, "Error getting name
>>> owner");
>>> - return false;
>>> - }
>>>
>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>>> + if (owner_id)
>>> + snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>>> + else
>>> + *owner = '\0';
>>>
>>> - _dbus_filter_name_owner_notify(dbus->filter, name, owner);
>>> + name_owner_notify(dbus, name, NULL, owner);
>>
>>
>> "" instead of NULL?
>>
With kdbus you don't even have a race, so using "" should be safe, no?
>>
>>>
>>> return true;
>>> }
>>> @@ -1222,6 +1272,17 @@ LIB_EXPORT struct l_dbus *l_dbus_new_default(enum
>>> l_dbus_bus bus)
>>> return setup_address(address);
>>> }
>>>
>>> +static void service_watch_data_free(void *data)
>>> +{
>>> + struct service_watch_data *watch = data;
>>> +
>>> + if (watch->destroy_func)
>>> + watch->destroy_func(watch->user_data);
>>> +
>>> + l_free(watch->bus_name);
>>> + l_free(watch);
>>> +}
>>> +
>>> LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
>>> {
>>> if (unlikely(!dbus))
>>> @@ -1236,6 +1297,11 @@ LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
>>> l_hashmap_destroy(dbus->message_list, message_list_destroy);
>>> l_queue_destroy(dbus->message_queue, message_queue_destroy);
>>>
>>> + l_hashmap_destroy(dbus->services_watched,
>>> service_watch_data_free);
>>> +
>>> + if (dbus->service_watch_remove_work)
>>> + l_idle_remove(dbus->service_watch_remove_work);
>>> +
>>> l_io_destroy(dbus->io);
>>>
>>> if (dbus->disconnect_destroy)
>>> @@ -1809,33 +1875,6 @@ void _dbus1_name_owner_changed_filter(struct
>>> l_dbus_message *message,
>>> }
>>> }
>>>
>>> -static void _dbus1_get_name_owner_reply(struct l_dbus_message *message,
>>> - void *user_data)
>>> -{
>>> - struct dbus1_filter_data *data = user_data;
>>> - const char *name;
>>> -
>>> - data->get_name_owner_id = 0;
>>> -
>>> - /* No name owner yet */
>>> - if (l_dbus_message_is_error(message))
>>> - return;
>>> -
>>> - /* Shouldn't happen */
>>> - if (!l_dbus_message_get_arguments(message, "s", &name))
>>> - return;
>>> -
>>> - /* If the signal arrived before the reply, don't do anything */
>>> - if (data->owner && !strcmp(data->owner, name))
>>> - return;
>>> -
>>> - l_free(data->owner);
>>> - data->owner = l_strdup(name);
>>> -
>>> - if (data->connect_func)
>>> - data->connect_func(data->dbus, data->user_data);
>>> -}
>>> -
>>> LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
>>> const char *name,
>>> l_dbus_watch_func_t
>>> disconnect_func,
>>> @@ -1846,56 +1885,103 @@ LIB_EXPORT unsigned int
>>> l_dbus_add_disconnect_watch(struct l_dbus *dbus,
>>> user_data, destroy);
>>> }
>>>
>>> -unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
>>> +LIB_EXPORT unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
>>> const char *name,
>>> l_dbus_watch_func_t connect_func,
>>> l_dbus_watch_func_t
>>> disconnect_func,
>>> void *user_data,
>>> l_dbus_destroy_func_t destroy)
>>> {
>>> - struct dbus1_filter_data *data;
>>> + struct service_watch_data *watch, *first;
>>>
>>> if (!name)
>>> return 0;
>>>
>>> - data = _dbus1_filter_data_get(dbus,
>>> _dbus1_name_owner_changed_filter,
>>> - DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
>>> - L_DBUS_INTERFACE_DBUS, "NameOwnerChanged",
>>> - name,
>>> - disconnect_func,
>>> - user_data,
>>> - destroy);
>>> - if (!data)
>>> - return 0;
>>> + if (!dbus->services_watched)
>>> + dbus->services_watched = l_hashmap_string_new();
>>> +
>>> + watch = l_new(struct service_watch_data, 1);
>>> + watch->bus_name = l_strdup(name);
>>> + watch->id = ++dbus->last_service_watch_id;
>>> + watch->connect_func = connect_func;
>>> + watch->disconnect_func = disconnect_func;
>>> + watch->destroy_func = destroy;
>>> +
>>> + /*
>>> + * Make sure that all the entries for the same bus name are
>>> + * grouped together on the list and that the first entry for the
>>> + * name is in the hashmap for easy lookups.
>>> + */
>>> + first = l_hashmap_lookup(dbus->services_watched, name);
>>> + if (first) {
>>> + watch->next = first->next;
>>> + first->next = watch;
>>> + } else {
>>> + l_hashmap_insert(dbus->services_watched, name, watch);
>>>
>>> - add_match(data);
>>> + watch->next = dbus->service_watch_head;
>>> + dbus->service_watch_head = watch;
>>> + }
>>>
>>> if (connect_func) {
>>> - struct l_dbus_message *message;
>>> + watch->pending_get_name_owner = true;
>>>
>>> - data->connect_func = connect_func;
>>> + dbus->driver->filter_ops.get_name_owner(dbus, name);
>>> + }
>>>
>>> - message = l_dbus_message_new_method_call(dbus,
>>> - DBUS_SERVICE_DBUS,
>>> - DBUS_PATH_DBUS,
>>> -
>>> L_DBUS_INTERFACE_DBUS,
>>> - "GetNameOwner");
>>> + return watch->id;
>>> +}
>>>
>>> - l_dbus_message_set_arguments(message, "s", name);
>>> +static void service_watch_remove(struct l_idle *idle, void *user_data)
>>> +{
>>> + struct l_dbus *dbus = user_data;
>>> + struct service_watch_data **watch, *tmp;
>>>
>>> - data->get_name_owner_id =
>>> - l_dbus_send_with_reply(dbus, message,
>>> -
>>> _dbus1_get_name_owner_reply,
>>> - data, NULL);
>>> - }
>>> + l_idle_remove(dbus->service_watch_remove_work);
>>> + dbus->service_watch_remove_work = NULL;
>>> +
>>> + for (watch = &dbus->service_watch_head; *watch;
>>> + watch = &(*watch)->next) {
>>> + if (!(*watch)->removed)
>>> + continue;
>>> +
>>> + tmp = *watch;
>>> + *watch = tmp->next;
>>> +
>>> + /* Check if this was the first entry for the bus name */
>>> + if (l_hashmap_lookup(dbus->services_watched,
>>> tmp->bus_name) ==
>>> + tmp) {
>>> + l_hashmap_remove(dbus->services_watched,
>>> tmp->bus_name);
>>> +
>>> + if (tmp->next && !strcmp(tmp->bus_name,
>>> + tmp->next->bus_name))
>>> + l_hashmap_insert(dbus->services_watched,
>>> + tmp->bus_name, tmp->next);
>>
>>
>> I really wonder if we should just add l_hashmap_find or something here and
>> avoid the linked-list on the side. It feels a bit kludgy to me.
>> Alternatively, let the user remove the service watch by name instead of
>> messing with ids.
>
> They'd have to also pass the callbacks and/or user_data to make sure
> we don't remove other watches for the same service.
>
> Do you think it would be cleaner if we mapped the service names to
> individual, normal linked lists instead of having one linked list? I
> noticed afterwards that this wouldn't be any worse than what I did
> here.
Yes, I think it would be a bit more readable.
>
>>
>>> + }
>>>
>>> - return l_dbus_register(dbus, _dbus1_signal_dispatcher, data,
>>> -
>>> filter_data_destroy);
>>> + service_watch_data_free(tmp);
>>> + }
>>> }
>>>
>>> LIB_EXPORT bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int
>>> id)
>>> {
>>> - return l_dbus_unregister(dbus, id);
>>> + struct service_watch_data *watch;
>>> +
>>> + for (watch = dbus->service_watch_head; watch; watch = watch->next)
>>> + if (watch->id == id && !watch->removed)
>>> + break;
>>> +
>>> + if (!watch)
>>> + return false;
>>> +
>>> + watch->removed = true;
>>
>>
>> Do you also want to make sure that the callbacks are not called by setting
>> them to NULL or something? Otherwise you can remove a watch, and it will
>> still be called due to pending events...
>
> name_owner_notify should already skip over entries that have removed == true.
Yep, I see that now. Nevermind :)
Regards,
-Denis
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API
2016-03-26 3:01 ` Denis Kenzior
@ 2016-03-26 15:50 ` Andrzej Zaborowski
2016-03-26 22:18 ` Denis Kenzior
0 siblings, 1 reply; 16+ messages in thread
From: Andrzej Zaborowski @ 2016-03-26 15:50 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 17933 bytes --]
in the cur would Hi Denis,
On 26 March 2016 at 04:01, Denis Kenzior <denkenz@gmail.com> wrote:
> On 03/25/2016 07:06 PM, Andrzej Zaborowski wrote:
>> On 25 March 2016 at 03:55, Denis Kenzior <denkenz@gmail.com> wrote:
>>> On 03/23/2016 09:07 PM, Andrew Zaborowski wrote:
>>>>
>>>> @@ -363,6 +379,35 @@ static void bus_ready(struct l_dbus *dbus)
>>>> l_io_set_write_handler(dbus->io, message_write_handler, dbus,
>>>> NULL);
>>>> }
>>>>
>>>> +static void name_owner_notify(struct l_dbus *dbus, const char *name,
>>>> + const char *old, const char *new)
>>>> +{
>>>> + struct service_watch_data *watch;
>>>> + bool connect = old && new && !*old && *new;
>>>> + bool disconnect = old && new && *old && !*new;
>>>
>>>
>>>
>>> This makes my brain hurt. I also don't think this works properly. Half
>>> the
>>> functions call name_owner_notify with empty strings and the other half
>>> calls
>>> it with NULLs.
>>>
>>> The disconnect can be expressed much simpler as *new == '\0'.
>>
>>
>> Disconnects yes, the NULLs are a different case where we don't know
>> the old value. We can't do *old == '\0' without checking that it
>> isn't NULL.
>>
>> But, I can change this function's signature to be something like void
>> name_owner_notify(dbus, const char *name, bool connect, bool
>> disconnect, const char *owner)
>>
>
> Nah, the signature is fine. I think you need to make the flow a bit
> simpler. The current logic listens to NameOwnerChanged and also queues up
> the GetNameOwner call. If the NameOwnerChanged signal wins the race,
> GetNameOwner call is canceled, connect is called. If GetNameOwner returns
> first, connect is called and subsequent NameOwnerChanged (if it arrives at
> all) doesn't do anything.
Yep, that's also what I think this patch does, I'm not sure if you're
saying there's any issue here. We don't cancel the GetnameOnwer
though.
>
> You try to do something similar with pending_get_name_owner flag. So lets
> use it to our advantage...
pending_get_name_owner is necessary to implement the logic above
because if the watch has just been added (flag is true), we have to
call connect regardless of whether the service has just popped up or
has already existed. If the flag is false, we only call connect if
the service had been disconnected before.
>
>>>
>>>
>>>> +
>>>> + _dbus_filter_name_owner_notify(dbus->filter, name, new);
>>>> +
>>>> + watch = l_hashmap_lookup(dbus->services_watched, name);
>>>> +
>>>> + while (watch && !strcmp(watch->bus_name, name)) {
>>>> + if (watch->removed)
>>>> + goto next;
>>>> +
>>>> + if (watch->pending_get_name_owner) {
>>>> + watch->pending_get_name_owner = false;
>>>> + if (new && *new)
>>>> + watch->connect_func(dbus,
>>>> watch->user_data);
>
>
> In case new is non-null, reset pending_get_name_owner to false and call
> connect.
We have to additionally check that either the service had been
disconneted until now (old was empty) or pending_get_name_owner is
true.
>
>>>> + } else if (connect && watch->connect_func)
>>>> + watch->connect_func(dbus, watch->user_data);
>>>> + else if (disconnect && watch->disconnect_func)
>>>> + watch->disconnect_func(dbus, watch->user_data);
>>>> +
>>>> +next:
>>>> + watch = watch->next;
>>>> + }
>>>> +}
>>>> +
>>>> static void hello_callback(struct l_dbus_message *message, void
>>>> *user_data)
>>>> {
>>>> struct l_dbus *dbus = user_data;
>>>> @@ -752,7 +797,7 @@ static void get_name_owner_reply_cb(struct
>>>> l_dbus_message *reply,
>>>> if (!l_dbus_message_get_arguments(req->message, "s", &name))
>>>> return;
>>>>
>>>> - _dbus_filter_name_owner_notify(req->dbus->filter, name, owner);
>>>> + name_owner_notify(req->dbus, name, NULL, owner);
>
>
> Then here, if pending_get_name_owner is false, simply return.
> get_name_owner_reply lost the race.
We don't itereate over all the watches here. We could do that but I
wanted to extract the common driver-independent logic out to a common
function, name_owner_notify.
>
> Otherwise, just call with "" instead of NULL and simplify your life in
> name_owner_notify.
But then again there may be watches for the same name that should not
have their "connect" called, so we still need some way to convey that.
Besides that "simplification" removes a single NULL check at a cost of
triplicated code.
>
>>>
>>>
>>> should this use "" instead of NULL?
>>
>>
>> If we pass "" then name_owner_notify will have to assume this is a new
>> connection and will issue spurious connect calls. The NULL here says
>> to not treat this as new connection or disconnection.
>>
>
> And by the way, we might want to handle the case where a service name is
> changed from one unique name to another. E.g. due to RequestName being
> queued or DBUS_NAME_FLAG_REPLACE_EXISTING somehow succeeding so that the
> name is passed from one service to another.
>
> For our purposes, calling disconnect and then immediately calling connect
> would probably be okay for now.
Ah again good point, I'll add this.
>
>
>>>
>>>> }
>>>>
>>>> static bool classic_get_name_owner(struct l_dbus *bus, const char
>>>> *name)
>>>> @@ -800,7 +845,7 @@ static void name_owner_changed_cb(struct
>>>> l_dbus_message *message,
>>>> if (!l_dbus_message_get_arguments(message, "sss", &name, &old,
>>>> &new))
>>>> return;
>>>>
>>>> - _dbus_filter_name_owner_notify(dbus->filter, name, new);
>>>> + name_owner_notify(dbus, name, old, new);
>>>> }
>>>>
>>>> static struct l_dbus *setup_dbus1(int fd, const char *guid)
>>>> @@ -1000,15 +1045,22 @@ static void kdbus_name_owner_change_func(const
>>>> char *name, uint64_t old_owner,
>>>> void *user_data)
>>>> {
>>>> struct kdbus_message_recv_data *recv_data = user_data;
>>>> - char owner[32];
>>>> + char old[32], new[32];
>>>>
>>>> l_util_debug(recv_data->dbus->debug_handler,
>>>> recv_data->dbus->debug_data,
>>>> "Read KDBUS Name Owner Change notification");
>>>>
>>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
>>>> + if (old_owner)
>>>> + snprintf(old, sizeof(old), ":1.%" PRIu64, new_owner);
>>>
>>>
>>>
>>> old_owner here, not new_owner?
>>
>>
>> Yes, thanks for catching this.
>>
>>>
>>>> + else
>>>> + *old = '\0';
>>>> + if (new_owner)
>>>> + snprintf(new, sizeof(new), ":1.%" PRIu64, new_owner);
>>>> + else
>>>> + *new = '\0';
>>>>
>>>> - _dbus_filter_name_owner_notify(recv_data->dbus->filter, name,
>>>> owner);
>>>> + name_owner_notify(recv_data->dbus, name, old, new);
>>>> }
>>>>
>>>> static struct l_dbus_message *kdbus_recv_message(struct l_dbus *dbus)
>>>> @@ -1070,15 +1122,13 @@ static bool kdbus_get_name_owner(struct l_dbus
>>>> *dbus, const char *name)
>>>> char owner[32];
>>>>
>>>> owner_id = _dbus_kernel_get_name_owner(fd, kdbus->kdbus_pool,
>>>> name);
>>>> - if (!owner_id) {
>>>> - l_util_debug(dbus->debug_handler,
>>>> - dbus->debug_data, "Error getting name
>>>> owner");
>>>> - return false;
>>>> - }
>>>>
>>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>>>> + if (owner_id)
>>>> + snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>>>> + else
>>>> + *owner = '\0';
>>>>
>>>> - _dbus_filter_name_owner_notify(dbus->filter, name, owner);
>>>> + name_owner_notify(dbus, name, NULL, owner);
>>>
>>>
>>>
>>> "" instead of NULL?
>>>
>
> With kdbus you don't even have a race, so using "" should be safe, no?
Well, name_owner_notify in the current form may call connect for other
watches for the same service. We'd have to call the connect callback
right from this function and reset pending_get_name_owner. I
personally like to keep the whole logic contained in name_owner_notify
and not have the driver functions do the work, but I could do that.
>
>
>>>
>>>>
>>>> return true;
>>>> }
>>>> @@ -1222,6 +1272,17 @@ LIB_EXPORT struct l_dbus *l_dbus_new_default(enum
>>>> l_dbus_bus bus)
>>>> return setup_address(address);
>>>> }
>>>>
>>>> +static void service_watch_data_free(void *data)
>>>> +{
>>>> + struct service_watch_data *watch = data;
>>>> +
>>>> + if (watch->destroy_func)
>>>> + watch->destroy_func(watch->user_data);
>>>> +
>>>> + l_free(watch->bus_name);
>>>> + l_free(watch);
>>>> +}
>>>> +
>>>> LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
>>>> {
>>>> if (unlikely(!dbus))
>>>> @@ -1236,6 +1297,11 @@ LIB_EXPORT void l_dbus_destroy(struct l_dbus
>>>> *dbus)
>>>> l_hashmap_destroy(dbus->message_list, message_list_destroy);
>>>> l_queue_destroy(dbus->message_queue, message_queue_destroy);
>>>>
>>>> + l_hashmap_destroy(dbus->services_watched,
>>>> service_watch_data_free);
>>>> +
>>>> + if (dbus->service_watch_remove_work)
>>>> + l_idle_remove(dbus->service_watch_remove_work);
>>>> +
>>>> l_io_destroy(dbus->io);
>>>>
>>>> if (dbus->disconnect_destroy)
>>>> @@ -1809,33 +1875,6 @@ void _dbus1_name_owner_changed_filter(struct
>>>> l_dbus_message *message,
>>>> }
>>>> }
>>>>
>>>> -static void _dbus1_get_name_owner_reply(struct l_dbus_message *message,
>>>> - void *user_data)
>>>> -{
>>>> - struct dbus1_filter_data *data = user_data;
>>>> - const char *name;
>>>> -
>>>> - data->get_name_owner_id = 0;
>>>> -
>>>> - /* No name owner yet */
>>>> - if (l_dbus_message_is_error(message))
>>>> - return;
>>>> -
>>>> - /* Shouldn't happen */
>>>> - if (!l_dbus_message_get_arguments(message, "s", &name))
>>>> - return;
>>>> -
>>>> - /* If the signal arrived before the reply, don't do anything */
>>>> - if (data->owner && !strcmp(data->owner, name))
>>>> - return;
>>>> -
>>>> - l_free(data->owner);
>>>> - data->owner = l_strdup(name);
>>>> -
>>>> - if (data->connect_func)
>>>> - data->connect_func(data->dbus, data->user_data);
>>>> -}
>>>> -
>>>> LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus
>>>> *dbus,
>>>> const char *name,
>>>> l_dbus_watch_func_t
>>>> disconnect_func,
>>>> @@ -1846,56 +1885,103 @@ LIB_EXPORT unsigned int
>>>> l_dbus_add_disconnect_watch(struct l_dbus *dbus,
>>>> user_data, destroy);
>>>> }
>>>>
>>>> -unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
>>>> +LIB_EXPORT unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
>>>> const char *name,
>>>> l_dbus_watch_func_t
>>>> connect_func,
>>>> l_dbus_watch_func_t
>>>> disconnect_func,
>>>> void *user_data,
>>>> l_dbus_destroy_func_t destroy)
>>>> {
>>>> - struct dbus1_filter_data *data;
>>>> + struct service_watch_data *watch, *first;
>>>>
>>>> if (!name)
>>>> return 0;
>>>>
>>>> - data = _dbus1_filter_data_get(dbus,
>>>> _dbus1_name_owner_changed_filter,
>>>> - DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
>>>> - L_DBUS_INTERFACE_DBUS,
>>>> "NameOwnerChanged",
>>>> - name,
>>>> - disconnect_func,
>>>> - user_data,
>>>> - destroy);
>>>> - if (!data)
>>>> - return 0;
>>>> + if (!dbus->services_watched)
>>>> + dbus->services_watched = l_hashmap_string_new();
>>>> +
>>>> + watch = l_new(struct service_watch_data, 1);
>>>> + watch->bus_name = l_strdup(name);
>>>> + watch->id = ++dbus->last_service_watch_id;
>>>> + watch->connect_func = connect_func;
>>>> + watch->disconnect_func = disconnect_func;
>>>> + watch->destroy_func = destroy;
>>>> +
>>>> + /*
>>>> + * Make sure that all the entries for the same bus name are
>>>> + * grouped together on the list and that the first entry for the
>>>> + * name is in the hashmap for easy lookups.
>>>> + */
>>>> + first = l_hashmap_lookup(dbus->services_watched, name);
>>>> + if (first) {
>>>> + watch->next = first->next;
>>>> + first->next = watch;
>>>> + } else {
>>>> + l_hashmap_insert(dbus->services_watched, name, watch);
>>>>
>>>> - add_match(data);
>>>> + watch->next = dbus->service_watch_head;
>>>> + dbus->service_watch_head = watch;
>>>> + }
>>>>
>>>> if (connect_func) {
>>>> - struct l_dbus_message *message;
>>>> + watch->pending_get_name_owner = true;
>>>>
>>>> - data->connect_func = connect_func;
>>>> + dbus->driver->filter_ops.get_name_owner(dbus, name);
>>>> + }
>>>>
>>>> - message = l_dbus_message_new_method_call(dbus,
>>>> -
>>>> DBUS_SERVICE_DBUS,
>>>> - DBUS_PATH_DBUS,
>>>> -
>>>> L_DBUS_INTERFACE_DBUS,
>>>> - "GetNameOwner");
>>>> + return watch->id;
>>>> +}
>>>>
>>>> - l_dbus_message_set_arguments(message, "s", name);
>>>> +static void service_watch_remove(struct l_idle *idle, void *user_data)
>>>> +{
>>>> + struct l_dbus *dbus = user_data;
>>>> + struct service_watch_data **watch, *tmp;
>>>>
>>>> - data->get_name_owner_id =
>>>> - l_dbus_send_with_reply(dbus, message,
>>>> -
>>>> _dbus1_get_name_owner_reply,
>>>> - data, NULL);
>>>> - }
>>>> + l_idle_remove(dbus->service_watch_remove_work);
>>>> + dbus->service_watch_remove_work = NULL;
>>>> +
>>>> + for (watch = &dbus->service_watch_head; *watch;
>>>> + watch = &(*watch)->next) {
>>>> + if (!(*watch)->removed)
>>>> + continue;
>>>> +
>>>> + tmp = *watch;
>>>> + *watch = tmp->next;
>>>> +
>>>> + /* Check if this was the first entry for the bus name */
>>>> + if (l_hashmap_lookup(dbus->services_watched,
>>>> tmp->bus_name) ==
>>>> + tmp) {
>>>> + l_hashmap_remove(dbus->services_watched,
>>>> tmp->bus_name);
>>>> +
>>>> + if (tmp->next && !strcmp(tmp->bus_name,
>>>> + tmp->next->bus_name))
>>>> + l_hashmap_insert(dbus->services_watched,
>>>> + tmp->bus_name,
>>>> tmp->next);
>>>
>>>
>>>
>>> I really wonder if we should just add l_hashmap_find or something here
>>> and
>>> avoid the linked-list on the side. It feels a bit kludgy to me.
>>> Alternatively, let the user remove the service watch by name instead of
>>> messing with ids.
>>
>>
>> They'd have to also pass the callbacks and/or user_data to make sure
>> we don't remove other watches for the same service.
>>
>> Do you think it would be cleaner if we mapped the service names to
>> individual, normal linked lists instead of having one linked list? I
>> noticed afterwards that this wouldn't be any worse than what I did
>> here.
>
>
> Yes, I think it would be a bit more readable.
>
>>
>>>
>>>> + }
>>>>
>>>> - return l_dbus_register(dbus, _dbus1_signal_dispatcher, data,
>>>> -
>>>> filter_data_destroy);
>>>> + service_watch_data_free(tmp);
>>>> + }
>>>> }
>>>>
>>>> LIB_EXPORT bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int
>>>> id)
>>>> {
>>>> - return l_dbus_unregister(dbus, id);
>>>> + struct service_watch_data *watch;
>>>> +
>>>> + for (watch = dbus->service_watch_head; watch; watch =
>>>> watch->next)
>>>> + if (watch->id == id && !watch->removed)
>>>> + break;
>>>> +
>>>> + if (!watch)
>>>> + return false;
>>>> +
>>>> + watch->removed = true;
>>>
>>>
>>>
>>> Do you also want to make sure that the callbacks are not called by
>>> setting
>>> them to NULL or something? Otherwise you can remove a watch, and it will
>>> still be called due to pending events...
>>
>>
>> name_owner_notify should already skip over entries that have removed ==
>> true.
>
>
> Yep, I see that now. Nevermind :)
>
>
> Regards,
> -Denis
> _______________________________________________
> ell mailing list
> ell(a)lists.01.org
> https://lists.01.org/mailman/listinfo/ell
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API
2016-03-26 15:50 ` Andrzej Zaborowski
@ 2016-03-26 22:18 ` Denis Kenzior
2016-03-28 14:30 ` Andrzej Zaborowski
0 siblings, 1 reply; 16+ messages in thread
From: Denis Kenzior @ 2016-03-26 22:18 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 9889 bytes --]
Hi Andrew,
On 03/26/2016 10:50 AM, Andrzej Zaborowski wrote:
> in the cur would Hi Denis,
>
> On 26 March 2016 at 04:01, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 03/25/2016 07:06 PM, Andrzej Zaborowski wrote:
>>> On 25 March 2016 at 03:55, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> On 03/23/2016 09:07 PM, Andrew Zaborowski wrote:
>>>>>
>>>>> @@ -363,6 +379,35 @@ static void bus_ready(struct l_dbus *dbus)
>>>>> l_io_set_write_handler(dbus->io, message_write_handler, dbus,
>>>>> NULL);
>>>>> }
>>>>>
>>>>> +static void name_owner_notify(struct l_dbus *dbus, const char *name,
>>>>> + const char *old, const char *new)
>>>>> +{
>>>>> + struct service_watch_data *watch;
>>>>> + bool connect = old && new && !*old && *new;
>>>>> + bool disconnect = old && new && *old && !*new;
>>>>
>>>>
>>>>
>>>> This makes my brain hurt. I also don't think this works properly. Half
>>>> the
>>>> functions call name_owner_notify with empty strings and the other half
>>>> calls
>>>> it with NULLs.
>>>>
>>>> The disconnect can be expressed much simpler as *new == '\0'.
>>>
>>>
>>> Disconnects yes, the NULLs are a different case where we don't know
>>> the old value. We can't do *old == '\0' without checking that it
>>> isn't NULL.
>>>
>>> But, I can change this function's signature to be something like void
>>> name_owner_notify(dbus, const char *name, bool connect, bool
>>> disconnect, const char *owner)
>>>
>>
>> Nah, the signature is fine. I think you need to make the flow a bit
>> simpler. The current logic listens to NameOwnerChanged and also queues up
>> the GetNameOwner call. If the NameOwnerChanged signal wins the race,
>> GetNameOwner call is canceled, connect is called. If GetNameOwner returns
>> first, connect is called and subsequent NameOwnerChanged (if it arrives at
>> all) doesn't do anything.
>
> Yep, that's also what I think this patch does, I'm not sure if you're
> saying there's any issue here. We don't cancel the GetnameOnwer
> though.
I'm just wondering why we aren't doing the same thing in the new world
order? Canceling GetNameOwner calls would make things much simpler.
>
>>
>> You try to do something similar with pending_get_name_owner flag. So lets
>> use it to our advantage...
>
> pending_get_name_owner is necessary to implement the logic above
> because if the watch has just been added (flag is true), we have to
> call connect regardless of whether the service has just popped up or
> has already existed. If the flag is false, we only call connect if
> the service had been disconnected before.
>
>>
>>>>
>>>>
>>>>> +
>>>>> + _dbus_filter_name_owner_notify(dbus->filter, name, new);
>>>>> +
>>>>> + watch = l_hashmap_lookup(dbus->services_watched, name);
>>>>> +
>>>>> + while (watch && !strcmp(watch->bus_name, name)) {
>>>>> + if (watch->removed)
>>>>> + goto next;
>>>>> +
>>>>> + if (watch->pending_get_name_owner) {
>>>>> + watch->pending_get_name_owner = false;
>>>>> + if (new && *new)
>>>>> + watch->connect_func(dbus,
>>>>> watch->user_data);
>>
>>
>> In case new is non-null, reset pending_get_name_owner to false and call
>> connect.
>
> We have to additionally check that either the service had been
> disconneted until now (old was empty) or pending_get_name_owner is
> true.
>
So I think the real issue is that you're trying to take care of two
unrelated code-flows (that perform similar operations) which makes
things pretty hard to understand. I'd be in favor of not hi-jacking the
filtering operations for service watch things. Keep the two path flows
separate.
And then we should probably add a proper WellKnownName -> UniqueName
cache abstraction, so that we avoid GetNameOwner calls all over the place.
>>
>>>>> + } else if (connect && watch->connect_func)
>>>>> + watch->connect_func(dbus, watch->user_data);
>>>>> + else if (disconnect && watch->disconnect_func)
>>>>> + watch->disconnect_func(dbus, watch->user_data);
>>>>> +
>>>>> +next:
>>>>> + watch = watch->next;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> static void hello_callback(struct l_dbus_message *message, void
>>>>> *user_data)
>>>>> {
>>>>> struct l_dbus *dbus = user_data;
>>>>> @@ -752,7 +797,7 @@ static void get_name_owner_reply_cb(struct
>>>>> l_dbus_message *reply,
>>>>> if (!l_dbus_message_get_arguments(req->message, "s", &name))
>>>>> return;
>>>>>
>>>>> - _dbus_filter_name_owner_notify(req->dbus->filter, name, owner);
>>>>> + name_owner_notify(req->dbus, name, NULL, owner);
>>
>>
>> Then here, if pending_get_name_owner is false, simply return.
>> get_name_owner_reply lost the race.
>
> We don't itereate over all the watches here. We could do that but I
> wanted to extract the common driver-independent logic out to a common
> function, name_owner_notify.
Which would iterate anyway, but fair point.
>
>>
>> Otherwise, just call with "" instead of NULL and simplify your life in
>> name_owner_notify.
>
> But then again there may be watches for the same name that should not
> have their "connect" called, so we still need some way to convey that.
> Besides that "simplification" removes a single NULL check at a cost of
> triplicated code.
>
>>
>>>>
>>>>
>>>> should this use "" instead of NULL?
>>>
>>>
>>> If we pass "" then name_owner_notify will have to assume this is a new
>>> connection and will issue spurious connect calls. The NULL here says
>>> to not treat this as new connection or disconnection.
>>>
>>
>> And by the way, we might want to handle the case where a service name is
>> changed from one unique name to another. E.g. due to RequestName being
>> queued or DBUS_NAME_FLAG_REPLACE_EXISTING somehow succeeding so that the
>> name is passed from one service to another.
>>
>> For our purposes, calling disconnect and then immediately calling connect
>> would probably be okay for now.
>
> Ah again good point, I'll add this.
>
>>
>>
>>>>
>>>>> }
>>>>>
>>>>> static bool classic_get_name_owner(struct l_dbus *bus, const char
>>>>> *name)
>>>>> @@ -800,7 +845,7 @@ static void name_owner_changed_cb(struct
>>>>> l_dbus_message *message,
>>>>> if (!l_dbus_message_get_arguments(message, "sss", &name, &old,
>>>>> &new))
>>>>> return;
>>>>>
>>>>> - _dbus_filter_name_owner_notify(dbus->filter, name, new);
>>>>> + name_owner_notify(dbus, name, old, new);
>>>>> }
>>>>>
>>>>> static struct l_dbus *setup_dbus1(int fd, const char *guid)
>>>>> @@ -1000,15 +1045,22 @@ static void kdbus_name_owner_change_func(const
>>>>> char *name, uint64_t old_owner,
>>>>> void *user_data)
>>>>> {
>>>>> struct kdbus_message_recv_data *recv_data = user_data;
>>>>> - char owner[32];
>>>>> + char old[32], new[32];
>>>>>
>>>>> l_util_debug(recv_data->dbus->debug_handler,
>>>>> recv_data->dbus->debug_data,
>>>>> "Read KDBUS Name Owner Change notification");
>>>>>
>>>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
>>>>> + if (old_owner)
>>>>> + snprintf(old, sizeof(old), ":1.%" PRIu64, new_owner);
>>>>
>>>>
>>>>
>>>> old_owner here, not new_owner?
>>>
>>>
>>> Yes, thanks for catching this.
>>>
>>>>
>>>>> + else
>>>>> + *old = '\0';
>>>>> + if (new_owner)
>>>>> + snprintf(new, sizeof(new), ":1.%" PRIu64, new_owner);
>>>>> + else
>>>>> + *new = '\0';
>>>>>
>>>>> - _dbus_filter_name_owner_notify(recv_data->dbus->filter, name,
>>>>> owner);
>>>>> + name_owner_notify(recv_data->dbus, name, old, new);
>>>>> }
>>>>>
>>>>> static struct l_dbus_message *kdbus_recv_message(struct l_dbus *dbus)
>>>>> @@ -1070,15 +1122,13 @@ static bool kdbus_get_name_owner(struct l_dbus
>>>>> *dbus, const char *name)
>>>>> char owner[32];
>>>>>
>>>>> owner_id = _dbus_kernel_get_name_owner(fd, kdbus->kdbus_pool,
>>>>> name);
>>>>> - if (!owner_id) {
>>>>> - l_util_debug(dbus->debug_handler,
>>>>> - dbus->debug_data, "Error getting name
>>>>> owner");
>>>>> - return false;
>>>>> - }
>>>>>
>>>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>>>>> + if (owner_id)
>>>>> + snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>>>>> + else
>>>>> + *owner = '\0';
>>>>>
>>>>> - _dbus_filter_name_owner_notify(dbus->filter, name, owner);
>>>>> + name_owner_notify(dbus, name, NULL, owner);
>>>>
>>>>
>>>>
>>>> "" instead of NULL?
>>>>
>>
>> With kdbus you don't even have a race, so using "" should be safe, no?
>
> Well, name_owner_notify in the current form may call connect for other
> watches for the same service. We'd have to call the connect callback
> right from this function and reset pending_get_name_owner. I
> personally like to keep the whole logic contained in name_owner_notify
> and not have the driver functions do the work, but I could do that.
Why not just do the _dbus_kernel_get_name_owner thing right away for
kdbus transports? I think this goes along with what I'm saying above
about keeping the two paths separate.
>>
>>
>>>>
>>>>>
>>>>> return true;
>>>>> }
Regards,
-Denis
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API
2016-03-26 22:18 ` Denis Kenzior
@ 2016-03-28 14:30 ` Andrzej Zaborowski
0 siblings, 0 replies; 16+ messages in thread
From: Andrzej Zaborowski @ 2016-03-28 14:30 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 9527 bytes --]
Hi Denis,
On 26 March 2016 at 23:18, Denis Kenzior <denkenz@gmail.com> wrote:
> On 03/26/2016 10:50 AM, Andrzej Zaborowski wrote:
>> On 26 March 2016 at 04:01, Denis Kenzior <denkenz@gmail.com> wrote:
>>> Nah, the signature is fine. I think you need to make the flow a bit
>>> simpler. The current logic listens to NameOwnerChanged and also queues
>>> up
>>> the GetNameOwner call. If the NameOwnerChanged signal wins the race,
>>> GetNameOwner call is canceled, connect is called. If GetNameOwner
>>> returns
>>> first, connect is called and subsequent NameOwnerChanged (if it arrives
>>> at
>>> all) doesn't do anything.
>>
>>
>> Yep, that's also what I think this patch does, I'm not sure if you're
>> saying there's any issue here. We don't cancel the GetnameOnwer
>> though.
>
>
> I'm just wondering why we aren't doing the same thing in the new world
> order? Canceling GetNameOwner calls would make things much simpler.
Let's do it, but I'm not sure about simpler. We're replacing a bool
variable with an uint32_t to save the call id, we're issueing an
addidional call to l_dbus_cancel and we save a process wakeup, there's
little change.
>
>
>>
>>>
>>> You try to do something similar with pending_get_name_owner flag. So
>>> lets
>>> use it to our advantage...
>>
>>
>> pending_get_name_owner is necessary to implement the logic above
>> because if the watch has just been added (flag is true), we have to
>> call connect regardless of whether the service has just popped up or
>> has already existed. If the flag is false, we only call connect if
>> the service had been disconnected before.
>>
>>>
>>>>>
>>>>>
>>>>>> +
>>>>>> + _dbus_filter_name_owner_notify(dbus->filter, name, new);
>>>>>> +
>>>>>> + watch = l_hashmap_lookup(dbus->services_watched, name);
>>>>>> +
>>>>>> + while (watch && !strcmp(watch->bus_name, name)) {
>>>>>> + if (watch->removed)
>>>>>> + goto next;
>>>>>> +
>>>>>> + if (watch->pending_get_name_owner) {
>>>>>> + watch->pending_get_name_owner = false;
>>>>>> + if (new && *new)
>>>>>> + watch->connect_func(dbus,
>>>>>> watch->user_data);
>>>
>>>
>>>
>>> In case new is non-null, reset pending_get_name_owner to false and call
>>> connect.
>>
>>
>> We have to additionally check that either the service had been
>> disconneted until now (old was empty) or pending_get_name_owner is
>> true.
>>
>
> So I think the real issue is that you're trying to take care of two
> unrelated code-flows (that perform similar operations) which makes things
> pretty hard to understand. I'd be in favor of not hi-jacking the filtering
> operations for service watch things. Keep the two path flows separate.
Do you mean that _dbus_filter_name_owner_notify is in the same
function as the watch handling? I'll move this to a subfunction then.
>
> And then we should probably add a proper WellKnownName -> UniqueName cache
> abstraction, so that we avoid GetNameOwner calls all over the place.
>
>>>
>>>>>> + } else if (connect && watch->connect_func)
>>>>>> + watch->connect_func(dbus, watch->user_data);
>>>>>> + else if (disconnect && watch->disconnect_func)
>>>>>> + watch->disconnect_func(dbus,
>>>>>> watch->user_data);
>>>>>> +
>>>>>> +next:
>>>>>> + watch = watch->next;
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> static void hello_callback(struct l_dbus_message *message, void
>>>>>> *user_data)
>>>>>> {
>>>>>> struct l_dbus *dbus = user_data;
>>>>>> @@ -752,7 +797,7 @@ static void get_name_owner_reply_cb(struct
>>>>>> l_dbus_message *reply,
>>>>>> if (!l_dbus_message_get_arguments(req->message, "s", &name))
>>>>>> return;
>>>>>>
>>>>>> - _dbus_filter_name_owner_notify(req->dbus->filter, name,
>>>>>> owner);
>>>>>> + name_owner_notify(req->dbus, name, NULL, owner);
>>>
>>>
>>>
>>> Then here, if pending_get_name_owner is false, simply return.
>>> get_name_owner_reply lost the race.
>>
>>
>> We don't itereate over all the watches here. We could do that but I
>> wanted to extract the common driver-independent logic out to a common
>> function, name_owner_notify.
>
>
> Which would iterate anyway, but fair point.
>
>
>>
>>>
>>> Otherwise, just call with "" instead of NULL and simplify your life in
>>> name_owner_notify.
>>
>>
>> But then again there may be watches for the same name that should not
>> have their "connect" called, so we still need some way to convey that.
>> Besides that "simplification" removes a single NULL check at a cost of
>> triplicated code.
>>
>>>
>>>>>
>>>>>
>>>>> should this use "" instead of NULL?
>>>>
>>>>
>>>>
>>>> If we pass "" then name_owner_notify will have to assume this is a new
>>>> connection and will issue spurious connect calls. The NULL here says
>>>> to not treat this as new connection or disconnection.
>>>>
>>>
>>> And by the way, we might want to handle the case where a service name is
>>> changed from one unique name to another. E.g. due to RequestName being
>>> queued or DBUS_NAME_FLAG_REPLACE_EXISTING somehow succeeding so that the
>>> name is passed from one service to another.
>>>
>>> For our purposes, calling disconnect and then immediately calling connect
>>> would probably be okay for now.
>>
>>
>> Ah again good point, I'll add this.
>>
>>>
>>>
>>>>>
>>>>>> }
>>>>>>
>>>>>> static bool classic_get_name_owner(struct l_dbus *bus, const char
>>>>>> *name)
>>>>>> @@ -800,7 +845,7 @@ static void name_owner_changed_cb(struct
>>>>>> l_dbus_message *message,
>>>>>> if (!l_dbus_message_get_arguments(message, "sss", &name,
>>>>>> &old,
>>>>>> &new))
>>>>>> return;
>>>>>>
>>>>>> - _dbus_filter_name_owner_notify(dbus->filter, name, new);
>>>>>> + name_owner_notify(dbus, name, old, new);
>>>>>> }
>>>>>>
>>>>>> static struct l_dbus *setup_dbus1(int fd, const char *guid)
>>>>>> @@ -1000,15 +1045,22 @@ static void kdbus_name_owner_change_func(const
>>>>>> char *name, uint64_t old_owner,
>>>>>> void *user_data)
>>>>>> {
>>>>>> struct kdbus_message_recv_data *recv_data = user_data;
>>>>>> - char owner[32];
>>>>>> + char old[32], new[32];
>>>>>>
>>>>>> l_util_debug(recv_data->dbus->debug_handler,
>>>>>> recv_data->dbus->debug_data,
>>>>>> "Read KDBUS Name Owner Change
>>>>>> notification");
>>>>>>
>>>>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
>>>>>> + if (old_owner)
>>>>>> + snprintf(old, sizeof(old), ":1.%" PRIu64, new_owner);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> old_owner here, not new_owner?
>>>>
>>>>
>>>>
>>>> Yes, thanks for catching this.
>>>>
>>>>>
>>>>>> + else
>>>>>> + *old = '\0';
>>>>>> + if (new_owner)
>>>>>> + snprintf(new, sizeof(new), ":1.%" PRIu64, new_owner);
>>>>>> + else
>>>>>> + *new = '\0';
>>>>>>
>>>>>> - _dbus_filter_name_owner_notify(recv_data->dbus->filter, name,
>>>>>> owner);
>>>>>> + name_owner_notify(recv_data->dbus, name, old, new);
>>>>>> }
>>>>>>
>>>>>> static struct l_dbus_message *kdbus_recv_message(struct l_dbus
>>>>>> *dbus)
>>>>>> @@ -1070,15 +1122,13 @@ static bool kdbus_get_name_owner(struct l_dbus
>>>>>> *dbus, const char *name)
>>>>>> char owner[32];
>>>>>>
>>>>>> owner_id = _dbus_kernel_get_name_owner(fd,
>>>>>> kdbus->kdbus_pool,
>>>>>> name);
>>>>>> - if (!owner_id) {
>>>>>> - l_util_debug(dbus->debug_handler,
>>>>>> - dbus->debug_data, "Error getting name
>>>>>> owner");
>>>>>> - return false;
>>>>>> - }
>>>>>>
>>>>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
>>>>>> + if (owner_id)
>>>>>> + snprintf(owner, sizeof(owner), ":1.%" PRIu64,
>>>>>> owner_id);
>>>>>> + else
>>>>>> + *owner = '\0';
>>>>>>
>>>>>> - _dbus_filter_name_owner_notify(dbus->filter, name, owner);
>>>>>> + name_owner_notify(dbus, name, NULL, owner);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> "" instead of NULL?
>>>>>
>>>
>>> With kdbus you don't even have a race, so using "" should be safe, no?
>>
>>
>> Well, name_owner_notify in the current form may call connect for other
>> watches for the same service. We'd have to call the connect callback
>> right from this function and reset pending_get_name_owner. I
>> personally like to keep the whole logic contained in name_owner_notify
>> and not have the driver functions do the work, but I could do that.
>
>
> Why not just do the _dbus_kernel_get_name_owner thing right away for kdbus
> transports? I think this goes along with what I'm saying above about
> keeping the two paths separate.
So we're calling dbus->driver->filter_ops.get_name_owner which calls
_dbus_kernel_get_name_owner right away, do you mean we should check if
dbus->driver == kdbus_ops and then call _dbus_kernel_get_name_owner
directly? Or we could add a .get_name_owner method in l_dbus_ops too?
Best regards
^ permalink raw reply [flat|nested] 16+ messages in thread