[PATCH v3 0/4] Add DBus disconnect watch support

[PATCH v3 0/4] Add DBus disconnect watch support

[Jukka Rissanen]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

Hi,

v3:
- reworked the unit test

v2:
- moved the code to dbus.c
- reworked the logic in the caller of the watcher so no need to
  remove l_dbus_register() and l_dbus_unregister()

this set allows user to monitor the disconnect status
of a service.

Cheers,
Jukka


Jukka Rissanen (4):
  dbus: Add AddMatch and RemoveMatch support
  gvariant: dbus.h need to be included before private header
  dbus: Add disconnect watch support
  unit: dbus: Add test for disconnect watch API

 Makefile.am            |   3 +
 ell/dbus-private.h     |  35 ++++++++
 ell/dbus.c             | 221 +++++++++++++++++++++++++++++++++++++++++++++++++
 ell/dbus.h             |   9 ++
 ell/gvariant-util.c    |   2 +-
 unit/test-dbus-watch.c | 153 ++++++++++++++++++++++++++++++++++
 6 files changed, 422 insertions(+), 1 deletion(-)
 create mode 100644 unit/test-dbus-watch.c

-- 
2.1.0

[PATCH v3 1/4] dbus: Add AddMatch and RemoveMatch support

[Jukka Rissanen]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

We do not check for errors as they are typically unrecoverable
in this case and in most cases ignored anyway.

This is the comment from libdbus API documentation for
dbus_bus_add_match() about error checking:

"If you pass NULL for the error, this function will not block;
the match thus won't be added until you flush the connection,
and if there's an error adding the match you won't find out
about it. This is generally acceptable, since the possible
errors (including a lack of resources in the bus, the connection
having exceeded its quota of active match rules, or the match
rule being unparseable) are generally unrecoverable."
---
 ell/dbus.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/ell/dbus.c b/ell/dbus.c
index 4c4ba2d..c10e0ed 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -1227,3 +1227,29 @@ LIB_EXPORT bool l_dbus_unregister_interface(struct l_dbus *dbus,
 
 	return _dbus_object_tree_unregister(dbus->tree, path, interface);
 }
+
+static void 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,
+						DBUS_INTERFACE_DBUS,
+						method);
+
+	l_dbus_message_set_arguments(message, "s", rule);
+
+	send_message(dbus, false, message, NULL, NULL, NULL);
+}
+
+static void _dbus_bus_add_match(struct l_dbus *dbus, const char *rule)
+{
+	send_match(dbus, rule, "AddMatch");
+}
+
+static void _dbus_bus_remove_match(struct l_dbus *dbus, const char *rule)
+{
+	send_match(dbus, rule, "RemoveMatch");
+}
-- 
2.1.0

[PATCH v3 2/4] gvariant: dbus.h need to be included before private header

[Jukka Rissanen]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

Needed by dbus watcher unit tests which add stuff to private
header file.
---
 ell/gvariant-util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 8fb32c7..7534509 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -37,9 +37,9 @@
 #include "queue.h"
 #include "string.h"
 #include "log.h"
+#include "dbus.h"
 #include "dbus-private.h"
 #include "gvariant-private.h"
-#include "dbus.h"
 
 static const char *simple_types = "sogybnqiuxtdh";
 static const char *variable_types = "sogav";
-- 
2.1.0

[PATCH v3 3/4] dbus: Add disconnect watch support

[Jukka Rissanen]

[-- Attachment #1: Type: text/plain, Size: 8402 bytes --]

Allow caller to monitor the disconnect status of a service.
---
 ell/dbus-private.h |  35 ++++++++++
 ell/dbus.c         | 195 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/dbus.h         |   9 +++
 3 files changed, 239 insertions(+)

diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 9987294..cbde2fd 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -212,3 +212,38 @@ int _dbus_kernel_remove_match(int fd, uint64_t cookie);
 
 uint8_t _dbus_get_version(struct l_dbus *dbus);
 int _dbus_get_fd(struct l_dbus *dbus);
+
+struct _dbus_filter_data {
+	struct l_dbus *dbus;
+	l_dbus_message_func_t handle_func;
+	l_dbus_watch_func_t connect_func;
+	l_dbus_watch_func_t disconnect_func;
+	char *sender;
+	char *path;
+	char *interface;
+	char *member;
+	char *argument;
+	void *user_data;
+	l_dbus_destroy_func_t destroy_func;
+};
+
+typedef void (*l_dbus_match_func_t) (struct _dbus_filter_data *data,
+					l_dbus_message_func_t filter);
+
+void _dbus_message_filter_dispatcher(struct l_dbus_message *message,
+				     void *user_data);
+void _dbus1_format_rule(struct _dbus_filter_data *data, char *rule,
+			size_t size);
+struct _dbus_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,
+					l_dbus_match_func_t match);
+void _dbus1_filter_data_destroy(void *user_data, l_dbus_match_func_t match);
+void _dbus1_service_filter(struct l_dbus_message *message, void *user_data);
diff --git a/ell/dbus.c b/ell/dbus.c
index c10e0ed..8596449 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -53,6 +53,8 @@
 #define DBUS_INTERFACE_INTROSPECTABLE	"org.freedesktop.DBus.Introspectable"
 #define DBUS_INTERFACE_PROPERTIES	"org.freedesktop.DBus.Properties"
 
+#define DBUS_MAXIMUM_MATCH_RULE_LENGTH	1024
+
 enum auth_state {
 	WAITING_FOR_OK,
 	WAITING_FOR_AGREE_UNIX_FD,
@@ -1253,3 +1255,196 @@ static void _dbus_bus_remove_match(struct l_dbus *dbus, const char *rule)
 {
 	send_match(dbus, rule, "RemoveMatch");
 }
+
+void _dbus1_format_rule(struct _dbus_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);
+}
+
+static void add_match(struct _dbus_filter_data *data,
+						l_dbus_message_func_t filter)
+{
+	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
+
+	_dbus1_format_rule(data, rule, sizeof(rule));
+
+	_dbus_bus_add_match(data->dbus, rule);
+
+	data->handle_func = filter;
+}
+
+static void remove_match(struct _dbus_filter_data *data,
+						l_dbus_message_func_t filter)
+{
+	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
+
+	_dbus1_format_rule(data, rule, sizeof(rule));
+
+	_dbus_bus_remove_match(data->dbus, rule);
+}
+
+struct _dbus_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,
+					l_dbus_match_func_t add_match)
+{
+	struct _dbus_filter_data *data;
+
+	data = l_new(struct _dbus_filter_data, 1);
+
+	data->dbus = dbus;
+	data->handle_func = filter;
+	data->disconnect_func = disconnect_func;
+	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;
+
+	add_match(data, filter);
+
+	return data;
+}
+
+void _dbus1_filter_data_destroy(void *user_data,
+				l_dbus_match_func_t remove_match)
+{
+	struct _dbus_filter_data *data = user_data;
+
+	remove_match(data, NULL);
+
+	l_free(data->sender);
+	l_free(data->path);
+	l_free(data->interface);
+	l_free(data->member);
+	l_free(data->argument);
+
+	if (data->destroy_func)
+		data->destroy_func(data->user_data);
+
+	l_free(data);
+}
+
+static void filter_data_destroy(void *user_data)
+{
+	_dbus1_filter_data_destroy(user_data, remove_match);
+}
+
+void _dbus_message_filter_dispatcher(struct l_dbus_message *message,
+				     void *user_data)
+{
+	struct _dbus_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_service_filter(struct l_dbus_message *message, void *user_data)
+{
+	struct _dbus_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 (*new == '\0') {
+		if (data->disconnect_func)
+			data->disconnect_func(data->dbus, data->user_data);
+	}
+}
+
+static unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
+					const char *name,
+					l_dbus_watch_func_t disconnect_func,
+					void *user_data,
+					l_dbus_destroy_func_t destroy)
+{
+	struct _dbus_filter_data *data;
+
+	if (!name)
+		return 0;
+
+	data = _dbus1_filter_data_get(dbus, _dbus1_service_filter,
+				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
+				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
+				name,
+				disconnect_func,
+				user_data,
+				destroy,
+				add_match);
+	if (!data)
+		return 0;
+
+	return l_dbus_register(dbus, _dbus_message_filter_dispatcher, data,
+							filter_data_destroy);
+}
+
+LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
+					const char *name,
+					l_dbus_watch_func_t disconnect_func,
+					void *user_data,
+					l_dbus_destroy_func_t destroy)
+{
+	return l_dbus_add_service_watch(dbus, name, disconnect_func,
+							user_data, destroy);
+}
+
+LIB_EXPORT bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int id)
+{
+	return l_dbus_unregister(dbus, id);
+}
diff --git a/ell/dbus.h b/ell/dbus.h
index a6ad109..39f926b 100644
--- a/ell/dbus.h
+++ b/ell/dbus.h
@@ -48,6 +48,8 @@ typedef void (*l_dbus_debug_func_t) (const char *str, void *user_data);
 typedef void (*l_dbus_destroy_func_t) (void *user_data);
 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);
+
 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);
@@ -196,6 +198,13 @@ bool l_dbus_register_interface(struct l_dbus *dbus,
 				l_dbus_destroy_func_t destroy);
 bool l_dbus_unregister_interface(struct l_dbus *dbus, const char *path,
 					const char *interface);
+unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
+					const char *name,
+					l_dbus_watch_func_t disconnect_func,
+					void *user_data,
+					l_dbus_destroy_func_t destroy);
+bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int id);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.1.0

[PATCH v3 4/4] unit: dbus: Add test for disconnect watch API

[Jukka Rissanen]

[-- Attachment #1: Type: text/plain, Size: 5343 bytes --]

Test the dbus disconnect watch support.
---
 Makefile.am            |   3 +
 unit/test-dbus-watch.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 156 insertions(+)
 create mode 100644 unit/test-dbus-watch.c

diff --git a/Makefile.am b/Makefile.am
index 50f81eb..eff88f7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -102,6 +102,7 @@ unit_tests = unit/test-unit \
 			unit/test-dbus-util \
 			unit/test-dbus-message \
 			unit/test-dbus-service \
+			unit/test-dbus-watch \
 			unit/test-gvariant-util \
 			unit/test-gvariant-message \
 			unit/test-siphash \
@@ -147,6 +148,8 @@ unit_test_dbus_util_LDADD = ell/libell-private.la
 
 unit_test_dbus_service_LDADD = ell/libell-private.la
 
+unit_test_dbus_watch_LDADD = ell/libell-private.la
+
 unit_test_gvariant_util_LDADD = ell/libell-private.la
 
 unit_test_gvariant_message_LDADD = ell/libell-private.la
diff --git a/unit/test-dbus-watch.c b/unit/test-dbus-watch.c
new file mode 100644
index 0000000..5e7cf60
--- /dev/null
+++ b/unit/test-dbus-watch.c
@@ -0,0 +1,153 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2011-2015  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#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"
+
+#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 *expected;
+	int count;
+};
+
+static struct watch_test match_test = {
+	.name = ":1.101",
+	.expected = "type='signal',"
+		"sender='org.freedesktop.DBus',"
+		"path='/org/freedesktop/DBus',"
+		"interface='org.freedesktop.DBus',"
+		"member='NameOwnerChanged',"
+		"arg0=':1.101'",
+};
+
+static struct watch_test disconnect_test = {
+	.name = ":101.1",
+	.count = 0,
+};
+
+static void verify_match(struct _dbus_filter_data *data,
+						l_dbus_message_func_t filter)
+{
+	struct watch_test *test = data->user_data;
+	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
+
+	_dbus1_format_rule(data, rule, sizeof(rule));
+
+	assert(strcmp(rule, test->expected) == 0);
+}
+
+static void test_match(const void *test_data)
+{
+	struct watch_test *test = (struct watch_test *)test_data;
+	struct _dbus_filter_data *data;
+
+	data = _dbus1_filter_data_get(NULL, _dbus1_service_filter,
+				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
+				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
+				test->name,
+				NULL,
+				test,
+				NULL,
+				verify_match);
+
+	_dbus1_filter_data_destroy(data, verify_match);
+}
+
+static void match(struct _dbus_filter_data *data, l_dbus_message_func_t filter)
+{
+	data->handle_func = filter;
+}
+
+static void disconnect_cb(struct l_dbus *dbus, void *user_data)
+{
+	struct watch_test *data = user_data;
+
+	data->count++;
+}
+
+static void send_filter_signal(struct _dbus_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);
+	_dbus_message_filter_dispatcher(message, data);
+	l_dbus_message_unref(message);
+}
+
+static void test_disconnect_watch(const void *test_data)
+{
+	struct watch_test *test = (struct watch_test *)test_data;
+	struct _dbus_filter_data *data;
+
+	data = _dbus1_filter_data_get(NULL, _dbus1_service_filter,
+				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
+				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
+				test->name,
+				disconnect_cb,
+				test,
+				NULL,
+				match);
+
+	send_filter_signal(data, ":0.1", ":0.1", "");
+	assert(test->count == 0);
+
+	send_filter_signal(data, ":0.1", ":0.1", ":0.2");
+	assert(test->count == 0);
+
+	send_filter_signal(data, ":101.1", ":101.1", "");
+	assert(test->count == 1);
+
+	_dbus1_filter_data_destroy(data, match);
+}
+
+int main(int argc, char *argv[])
+{
+	l_test_init(&argc, &argv);
+
+	l_test_add("DBus filter", test_match, &match_test);
+
+	l_test_add("DBus disconnect watch", test_disconnect_watch,
+			&disconnect_test);
+
+	return l_test_run();
+}
-- 
2.1.0

Re: [PATCH v3 1/4] dbus: Add AddMatch and RemoveMatch support

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]

Hi Jukka,

On 02/18/2015 10:08 AM, Jukka Rissanen wrote:
> We do not check for errors as they are typically unrecoverable
> in this case and in most cases ignored anyway.
>
> This is the comment from libdbus API documentation for
> dbus_bus_add_match() about error checking:
>
> "If you pass NULL for the error, this function will not block;
> the match thus won't be added until you flush the connection,
> and if there's an error adding the match you won't find out
> about it. This is generally acceptable, since the possible
> errors (including a lack of resources in the bus, the connection
> having exceeded its quota of active match rules, or the match
> rule being unparseable) are generally unrecoverable."
> ---
>   ell/dbus.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>

Mostly fine with this one, but please tweak the naming to be consistent 
with what we already have.

> diff --git a/ell/dbus.c b/ell/dbus.c
> index 4c4ba2d..c10e0ed 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -1227,3 +1227,29 @@ LIB_EXPORT bool l_dbus_unregister_interface(struct l_dbus *dbus,
>
>   	return _dbus_object_tree_unregister(dbus->tree, path, interface);
>   }
> +
> +static void send_match(struct l_dbus *dbus, const char *rule,
> +						const char *method)

So dbus1_send_match

> +{
> +	struct l_dbus_message *message;
> +
> +	message = l_dbus_message_new_method_call(dbus,
> +						DBUS_SERVICE_DBUS,
> +						DBUS_PATH_DBUS,
> +						DBUS_INTERFACE_DBUS,
> +						method);
> +
> +	l_dbus_message_set_arguments(message, "s", rule);
> +
> +	send_message(dbus, false, message, NULL, NULL, NULL);
> +}
> +
> +static void _dbus_bus_add_match(struct l_dbus *dbus, const char *rule)

The '_' prefix is reserved to exported functions that are private. 
Since this function is static, do not use the '_' prefix.

dbus1_bus_add_match

> +{
> +	send_match(dbus, rule, "AddMatch");
> +}
> +
> +static void _dbus_bus_remove_match(struct l_dbus *dbus, const char *rule)
> +{

dbus1_remove_match

> +	send_match(dbus, rule, "RemoveMatch");
> +}
>

Regards,
-Denis

Re: [PATCH v3 3/4] dbus: Add disconnect watch support

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 10160 bytes --]

Hi Jukka,

On 02/18/2015 10:08 AM, Jukka Rissanen wrote:
> Allow caller to monitor the disconnect status of a service.
> ---
>   ell/dbus-private.h |  35 ++++++++++
>   ell/dbus.c         | 195 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/dbus.h         |   9 +++
>   3 files changed, 239 insertions(+)
>
> diff --git a/ell/dbus-private.h b/ell/dbus-private.h
> index 9987294..cbde2fd 100644
> --- a/ell/dbus-private.h
> +++ b/ell/dbus-private.h
> @@ -212,3 +212,38 @@ int _dbus_kernel_remove_match(int fd, uint64_t cookie);
>
>   uint8_t _dbus_get_version(struct l_dbus *dbus);
>   int _dbus_get_fd(struct l_dbus *dbus);
> +
> +struct _dbus_filter_data {
> +	struct l_dbus *dbus;
> +	l_dbus_message_func_t handle_func;
> +	l_dbus_watch_func_t connect_func;

This member isn't used

> +	l_dbus_watch_func_t disconnect_func;
> +	char *sender;
> +	char *path;
> +	char *interface;
> +	char *member;
> +	char *argument;
> +	void *user_data;
> +	l_dbus_destroy_func_t destroy_func;
> +};

Please encapsulate this into the source file.

> +
> +typedef void (*l_dbus_match_func_t) (struct _dbus_filter_data *data,
> +					l_dbus_message_func_t filter);
> +
> +void _dbus_message_filter_dispatcher(struct l_dbus_message *message,
> +				     void *user_data);

Whitespace violation on the line above.

> +void _dbus1_format_rule(struct _dbus_filter_data *data, char *rule,
> +			size_t size);

Break this function into a separate patch, then submit a unit test for 
this in a separate patch. Both can be part of the v4 series.

> +struct _dbus_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,
> +					l_dbus_match_func_t match);
> +void _dbus1_filter_data_destroy(void *user_data, l_dbus_match_func_t match);
> +void _dbus1_service_filter(struct l_dbus_message *message, void *user_data);
> diff --git a/ell/dbus.c b/ell/dbus.c
> index c10e0ed..8596449 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -53,6 +53,8 @@
>   #define DBUS_INTERFACE_INTROSPECTABLE	"org.freedesktop.DBus.Introspectable"
>   #define DBUS_INTERFACE_PROPERTIES	"org.freedesktop.DBus.Properties"
>
> +#define DBUS_MAXIMUM_MATCH_RULE_LENGTH	1024
> +
>   enum auth_state {
>   	WAITING_FOR_OK,
>   	WAITING_FOR_AGREE_UNIX_FD,
> @@ -1253,3 +1255,196 @@ static void _dbus_bus_remove_match(struct l_dbus *dbus, const char *rule)
>   {
>   	send_match(dbus, rule, "RemoveMatch");
>   }
> +
> +void _dbus1_format_rule(struct _dbus_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);
> +}
> +
> +static void add_match(struct _dbus_filter_data *data,
> +						l_dbus_message_func_t filter)
> +{
> +	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
> +
> +	_dbus1_format_rule(data, rule, sizeof(rule));
> +
> +	_dbus_bus_add_match(data->dbus, rule);
> +
> +	data->handle_func = filter;

Why is this being set here, it is also being set in 
_dbus1_filter_data_get.  So you end up setting it twice.

> +}
> +
> +static void remove_match(struct _dbus_filter_data *data,
> +						l_dbus_message_func_t filter)

What's the reason for the filter argument?  It isn't used as far as I 
can tell.

> +{
> +	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
> +
> +	_dbus1_format_rule(data, rule, sizeof(rule));
> +
> +	_dbus_bus_remove_match(data->dbus, rule);
> +}
> +
> +struct _dbus_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,
> +					l_dbus_match_func_t add_match)

Why pass add_match in as the argument?  It also matches the a function 
name just above.  That is a good way to get people confused.  If you 
need to avoid calling add_match for unit test purposes, just make the 
caller of this function call add_match.

> +{
> +	struct _dbus_filter_data *data;
> +
> +	data = l_new(struct _dbus_filter_data, 1);
> +
> +	data->dbus = dbus;
> +	data->handle_func = filter;
> +	data->disconnect_func = disconnect_func;
> +	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;
> +
> +	add_match(data, filter);
> +
> +	return data;
> +}
> +
> +void _dbus1_filter_data_destroy(void *user_data,
> +				l_dbus_match_func_t remove_match)

Same question as above?

> +{
> +	struct _dbus_filter_data *data = user_data;
> +
> +	remove_match(data, NULL);
> +
> +	l_free(data->sender);
> +	l_free(data->path);
> +	l_free(data->interface);
> +	l_free(data->member);
> +	l_free(data->argument);
> +
> +	if (data->destroy_func)
> +		data->destroy_func(data->user_data);
> +
> +	l_free(data);
> +}
> +
> +static void filter_data_destroy(void *user_data)
> +{
> +	_dbus1_filter_data_destroy(user_data, remove_match);
> +}
> +
> +void _dbus_message_filter_dispatcher(struct l_dbus_message *message,
> +				     void *user_data)

Since everything used by this is _dbus1_*, use _dbus1_signal_dispatcher

> +{
> +	struct _dbus_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_service_filter(struct l_dbus_message *message, void *user_data)

void _dbus1_name_owner_changed_filter()

> +{
> +	struct _dbus_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 (*new == '\0') {
> +		if (data->disconnect_func)
> +			data->disconnect_func(data->dbus, data->user_data);
> +	}
> +}
> +
> +static unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
> +					const char *name,
> +					l_dbus_watch_func_t disconnect_func,
> +					void *user_data,
> +					l_dbus_destroy_func_t destroy)

Don't bother adding this function as static.  That is just confusing. 
If you don't support service watches, just move this functionality into 
add_disconnect_watch below and get rid of this function completely.

> +{
> +	struct _dbus_filter_data *data;
> +
> +	if (!name)
> +		return 0;
> +
> +	data = _dbus1_filter_data_get(dbus, _dbus1_service_filter,
> +				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
> +				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
> +				name,
> +				disconnect_func,
> +				user_data,
> +				destroy,
> +				add_match);
> +	if (!data)
> +		return 0;
> +
> +	return l_dbus_register(dbus, _dbus_message_filter_dispatcher, data,
> +							filter_data_destroy);
> +}
> +
> +LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
> +					const char *name,
> +					l_dbus_watch_func_t disconnect_func,
> +					void *user_data,
> +					l_dbus_destroy_func_t destroy)
> +{
> +	return l_dbus_add_service_watch(dbus, name, disconnect_func,
> +							user_data, destroy);
> +}
> +
> +LIB_EXPORT bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int id)
> +{
> +	return l_dbus_unregister(dbus, id);
> +}
> diff --git a/ell/dbus.h b/ell/dbus.h
> index a6ad109..39f926b 100644
> --- a/ell/dbus.h
> +++ b/ell/dbus.h
> @@ -48,6 +48,8 @@ typedef void (*l_dbus_debug_func_t) (const char *str, void *user_data);
>   typedef void (*l_dbus_destroy_func_t) (void *user_data);
>   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);
> +
>   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);
> @@ -196,6 +198,13 @@ bool l_dbus_register_interface(struct l_dbus *dbus,
>   				l_dbus_destroy_func_t destroy);
>   bool l_dbus_unregister_interface(struct l_dbus *dbus, const char *path,
>   					const char *interface);
> +unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
> +					const char *name,
> +					l_dbus_watch_func_t disconnect_func,
> +					void *user_data,
> +					l_dbus_destroy_func_t destroy);
> +bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int id);
> +
>   #ifdef __cplusplus
>   }
>   #endif
>

Regards,
-Denis

Re: [PATCH v3 4/4] unit: dbus: Add test for disconnect watch API

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 6266 bytes --]

Hi Jukka,

On 02/18/2015 10:08 AM, Jukka Rissanen wrote:
> Test the dbus disconnect watch support.
> ---
>   Makefile.am            |   3 +
>   unit/test-dbus-watch.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 156 insertions(+)
>   create mode 100644 unit/test-dbus-watch.c
>

Okay, now we're getting much closer to what I had in mind.

> diff --git a/Makefile.am b/Makefile.am
> index 50f81eb..eff88f7 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -102,6 +102,7 @@ unit_tests = unit/test-unit \
>   			unit/test-dbus-util \
>   			unit/test-dbus-message \
>   			unit/test-dbus-service \
> +			unit/test-dbus-watch \
>   			unit/test-gvariant-util \
>   			unit/test-gvariant-message \
>   			unit/test-siphash \
> @@ -147,6 +148,8 @@ unit_test_dbus_util_LDADD = ell/libell-private.la
>
>   unit_test_dbus_service_LDADD = ell/libell-private.la
>
> +unit_test_dbus_watch_LDADD = ell/libell-private.la
> +
>   unit_test_gvariant_util_LDADD = ell/libell-private.la
>
>   unit_test_gvariant_message_LDADD = ell/libell-private.la
> diff --git a/unit/test-dbus-watch.c b/unit/test-dbus-watch.c
> new file mode 100644
> index 0000000..5e7cf60
> --- /dev/null
> +++ b/unit/test-dbus-watch.c
> @@ -0,0 +1,153 @@
> +/*
> + *
> + *  Embedded Linux library
> + *
> + *  Copyright (C) 2011-2015  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#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"
> +
> +#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 *expected;
> +	int count;
> +};

Split this up into two structures and test things separately.

One structure and test to handle the actual AddMatch/RemoveMatch 
strings.  These should be in a separate patch like I already mentioned.

The other tests to cover the actual dispatcher.

> +
> +static struct watch_test match_test = {
> +	.name = ":1.101",
> +	.expected = "type='signal',"
> +		"sender='org.freedesktop.DBus',"
> +		"path='/org/freedesktop/DBus',"
> +		"interface='org.freedesktop.DBus',"
> +		"member='NameOwnerChanged',"
> +		"arg0=':1.101'",
> +};
> +
> +static struct watch_test disconnect_test = {
> +	.name = ":101.1",
> +	.count = 0,
> +};
> +
> +static void verify_match(struct _dbus_filter_data *data,
> +						l_dbus_message_func_t filter)
> +{
> +	struct watch_test *test = data->user_data;
> +	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
> +
> +	_dbus1_format_rule(data, rule, sizeof(rule));
> +
> +	assert(strcmp(rule, test->expected) == 0);
> +}
> +
> +static void test_match(const void *test_data)
> +{
> +	struct watch_test *test = (struct watch_test *)test_data;
> +	struct _dbus_filter_data *data;
> +
> +	data = _dbus1_filter_data_get(NULL, _dbus1_service_filter,
> +				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
> +				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
> +				test->name,
> +				NULL,
> +				test,
> +				NULL,
> +				verify_match);
> +
> +	_dbus1_filter_data_destroy(data, verify_match);

Just call _dbus1_format_rule directly here.

> +}
> +
> +static void match(struct _dbus_filter_data *data, l_dbus_message_func_t filter)
> +{
> +	data->handle_func = filter;
> +}
> +
> +static void disconnect_cb(struct l_dbus *dbus, void *user_data)
> +{
> +	struct watch_test *data = user_data;
> +
> +	data->count++;

count is not part of the test data, so just pass in a pointer to some 
arbitrary integer here.

> +}
> +
> +static void send_filter_signal(struct _dbus_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);
> +	_dbus_message_filter_dispatcher(message, data);
> +	l_dbus_message_unref(message);
> +}
> +
> +static void test_disconnect_watch(const void *test_data)
> +{
> +	struct watch_test *test = (struct watch_test *)test_data;
> +	struct _dbus_filter_data *data;
> +
> +	data = _dbus1_filter_data_get(NULL, _dbus1_service_filter,
> +				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
> +				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
> +				test->name,
> +				disconnect_cb,
> +				test,
> +				NULL,
> +				match);
> +
> +	send_filter_signal(data, ":0.1", ":0.1", "");
> +	assert(test->count == 0);
> +
> +	send_filter_signal(data, ":0.1", ":0.1", ":0.2");
> +	assert(test->count == 0);
> +
> +	send_filter_signal(data, ":101.1", ":101.1", "");
> +	assert(test->count == 1);
> +
> +	_dbus1_filter_data_destroy(data, match);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	l_test_init(&argc, &argv);
> +
> +	l_test_add("DBus filter", test_match, &match_test);
> +
> +	l_test_add("DBus disconnect watch", test_disconnect_watch,
> +			&disconnect_test);
> +
> +	return l_test_run();
> +}
>

Regards,
-Denis