[PATCH v2 0/3] Add DBus disconnect watch support

[PATCH v2 0/3] Add DBus disconnect watch support

[Jukka Rissanen]

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

Hi,

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 (3):
  dbus: Add AddMatch and RemoveMatch support
  dbus: Add disconnect watch support
  unit: dbus: Add test for disconnect watch API

 Makefile.am            |   3 +
 ell/dbus.c             | 253 ++++++++++++++++++++++++++++++++
 ell/dbus.h             |  19 +++
 unit/test-dbus-watch.c | 388 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 663 insertions(+)
 create mode 100644 unit/test-dbus-watch.c

-- 
2.1.0

[PATCH v2 1/3] dbus: Add AddMatch and RemoveMatch support

[Jukka Rissanen]

[-- Attachment #1: Type: text/plain, Size: 2210 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 ++++++++++++++++++++++++++
 ell/dbus.h |  4 ++++
 2 files changed, 30 insertions(+)

diff --git a/ell/dbus.c b/ell/dbus.c
index 4c4ba2d..96a1230 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);
+}
+
+LIB_EXPORT void l_dbus_bus_add_match(struct l_dbus *dbus, const char *rule)
+{
+	send_match(dbus, rule, "AddMatch");
+}
+
+LIB_EXPORT void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule)
+{
+	send_match(dbus, rule, "RemoveMatch");
+}
diff --git a/ell/dbus.h b/ell/dbus.h
index a6ad109..d5890a5 100644
--- a/ell/dbus.h
+++ b/ell/dbus.h
@@ -196,6 +196,10 @@ 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);
+
+void l_dbus_bus_add_match(struct l_dbus *dbus, const char *rule);
+void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.1.0

[PATCH v2 2/3] dbus: Add disconnect watch support

[Jukka Rissanen]

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

Allow caller to monitor the disconnect status of a service.
---
 ell/dbus.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/dbus.h |  15 ++++
 2 files changed, 242 insertions(+)

diff --git a/ell/dbus.c b/ell/dbus.c
index 96a1230..f98b042 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"
@@ -53,6 +54,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,
@@ -122,6 +125,21 @@ struct signal_callback {
 	void *user_data;
 };
 
+struct 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 *name;
+	char *owner;
+	char *path;
+	char *interface;
+	char *member;
+	char *argument;
+	void *user_data;
+	l_dbus_destroy_func_t destroy_func;
+};
+
 static void message_queue_destroy(void *data)
 {
 	struct message_callback *callback = data;
@@ -1253,3 +1271,212 @@ LIB_EXPORT void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule)
 {
 	send_match(dbus, rule, "RemoveMatch");
 }
+
+static void format_rule(struct filter_data *data, char *rule, size_t size)
+{
+	const char *sender;
+	int offset;
+
+	offset = snprintf(rule, size, "type='signal'");
+	sender = data->name ? : data->owner;
+
+	if (sender)
+		offset += snprintf(rule + offset, size - offset,
+				",sender='%s'", 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 filter_data *data, l_dbus_message_func_t filter)
+{
+	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
+
+	format_rule(data, rule, sizeof(rule));
+
+	l_dbus_bus_add_match(data->dbus, rule);
+
+	data->handle_func = filter;
+}
+
+static void remove_match(struct filter_data *data)
+{
+	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
+
+	format_rule(data, rule, sizeof(rule));
+
+	l_dbus_bus_remove_match(data->dbus, rule);
+}
+
+static struct filter_data *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 connect_func,
+					l_dbus_watch_func_t disconnect_func,
+					void *user_data,
+					l_dbus_destroy_func_t destroy)
+{
+	struct filter_data *data;
+	const char *name = NULL, *owner = NULL;
+
+	if (sender) {
+		if (sender[0] == ':')
+			owner = sender;
+		else
+			name = sender;
+	}
+
+	data = l_new(struct filter_data, 1);
+
+	data->dbus = dbus;
+	data->handle_func = filter;
+	data->connect_func = connect_func;
+	data->disconnect_func = disconnect_func;
+	data->name = l_strdup(name);
+	data->owner = l_strdup(owner);
+	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;
+}
+
+static void filter_data_destroy(void *user_data)
+{
+	struct filter_data *data = user_data;
+
+	remove_match(data);
+
+	l_free(data->name);
+	l_free(data->owner);
+	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 message_filter(struct l_dbus_message *message, void *user_data)
+{
+	struct filter_data *data = user_data;
+	const char *sender, *path, *iface, *member, *arg = NULL;
+
+	if (_dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_SIGNAL)
+		return;
+
+	sender = l_dbus_message_get_sender(message);
+	if (!sender)
+		return;
+
+	path = l_dbus_message_get_path(message);
+	iface = l_dbus_message_get_interface(message);
+	member = l_dbus_message_get_member(message);
+
+	l_dbus_message_get_arguments(message, "s", &arg);
+
+	if (data->owner && strcmp(sender, data->owner))
+		goto out;
+
+	if (data->path && strcmp(path, data->path))
+		goto out;
+
+	if (data->interface && strcmp(iface, data->interface))
+		goto out;
+
+	if (data->member && strcmp(member, data->member))
+		goto out;
+
+	if (arg && data->argument && strcmp(arg, data->argument))
+		goto out;
+
+	if (data->handle_func)
+		data->handle_func(message, data);
+
+out:
+	return;
+}
+
+static void service_filter(struct l_dbus_message *message, void *user_data)
+{
+	struct filter_data *data = user_data;
+	char *name, *old, *new;
+
+	if (!l_dbus_message_get_arguments(message, "sss",
+						&name, &old, &new))
+		return;
+
+	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_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 filter_data *data;
+
+	if (!name)
+		return 0;
+
+	if (connect_func)
+		return -EOPNOTSUPP;
+
+	data = filter_data_get(dbus, service_filter,
+				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
+				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
+				name,
+				connect_func,
+				disconnect_func,
+				user_data,
+				destroy);
+	if (!data)
+		return 0;
+
+	return l_dbus_register(dbus, message_filter, 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, NULL, 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 d5890a5..4a17de7 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);
@@ -200,6 +202,19 @@ bool l_dbus_unregister_interface(struct l_dbus *dbus, const char *path,
 void l_dbus_bus_add_match(struct l_dbus *dbus, const char *rule);
 void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule);
 
+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);
+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 v2 3/3] unit: dbus: Add test for disconnect watch API

[Jukka Rissanen]

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

Test the dbus disconnect watch support.
---
 Makefile.am            |   3 +
 unit/test-dbus-watch.c | 388 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 391 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..6c93a87
--- /dev/null
+++ b/unit/test-dbus-watch.c
@@ -0,0 +1,388 @@
+/*
+ *
+ *  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 <ell/ell.h>
+
+#define TEST_BUS_ADDRESS "unix:path=/tmp/ell-watch-test-bus"
+
+#define SERVICE_TO_WATCH "org.test.watch.service"
+#define SERVICE_MONITOR "org.test.main.service"
+
+static pid_t dbus_daemon_pid = -1;
+static pid_t test_daemon_pid = -1;
+
+static struct l_timeout *timeout = NULL;
+static bool failure = false;
+static struct tm *tm = NULL;
+
+static pid_t launch(char *name, char **argv, char **envp)
+{
+	pid_t pid;
+
+	l_info("launching %s", name);
+
+	pid = fork();
+	if (pid < 0) {
+		l_error("failed to fork new process");
+		return -1;
+	}
+
+	if (pid == 0) {
+		execve(argv[0], argv, envp);
+		exit(EXIT_SUCCESS);
+	}
+
+	l_info("%s process %d created", name, pid);
+
+	return pid;
+}
+
+static void start_dbus_daemon(void)
+{
+	char *prg_argv[6];
+	char *prg_envp[1];
+
+	prg_argv[0] = "/usr/bin/dbus-daemon";
+	prg_argv[1] = "--session";
+	prg_argv[2] = "--address=" TEST_BUS_ADDRESS;
+	prg_argv[3] = "--nopidfile";
+	prg_argv[4] = "--nofork";
+	prg_argv[5] = NULL;
+
+	prg_envp[0] = NULL;
+
+	dbus_daemon_pid = launch("dbus-daemon",
+				prg_argv, prg_envp);
+}
+
+static void start_test_daemon(char *argv0, char *argv1)
+{
+	char *prg_argv[3];
+	char *prg_envp[1];
+
+	prg_argv[0] = argv0;
+	prg_argv[1] = argv1;
+	prg_argv[2] = NULL;
+
+	prg_envp[0] = NULL;
+
+	test_daemon_pid = launch("test daemon",
+				prg_argv, prg_envp);
+}
+
+static void signal_handler(struct l_signal *signal, uint32_t signo,
+							void *user_data)
+{
+	switch (signo) {
+	case SIGINT:
+	case SIGTERM:
+		l_info("Terminate");
+		l_main_quit();
+		break;
+	case SIGCHLD:
+		while (1) {
+			pid_t pid;
+			int status;
+
+			pid = waitpid(WAIT_ANY, &status, WNOHANG);
+			if (pid < 0 || pid == 0)
+				break;
+
+			l_info("process %d terminated with status=%d\n",
+								pid, status);
+
+			if (pid == dbus_daemon_pid) {
+				dbus_daemon_pid = -1;
+				l_main_quit();
+			}
+		}
+		break;
+	}
+}
+
+static void ready_callback(void *user_data)
+{
+	char *argv0 = user_data;
+
+	l_info("test app ready");
+
+	start_test_daemon(argv0, "test");
+}
+
+static void ready_tester(void *user_data)
+{
+	l_info("tester ready");
+
+	return;
+}
+
+struct data {
+	char *name;
+	struct l_dbus *dbus;
+};
+
+static void release_name_setup(struct l_dbus_message *message, void *user_data)
+{
+	struct data *data = user_data;
+
+	l_dbus_message_set_arguments(message, "s", data->name);
+}
+
+static void release_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);
+		goto done;
+	}
+
+	if (!l_dbus_message_get_arguments(message, "u", &result))
+		goto done;
+
+	l_info("release name result = %d", result);
+
+done:
+	l_main_quit();
+}
+
+static void request_name_setup(struct l_dbus_message *message,
+				void *user_data)
+{
+	struct data *data = user_data;
+
+	l_dbus_message_set_arguments(message, "su", data->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);
+		goto done;
+	}
+
+	if (!l_dbus_message_get_arguments(message, "u", &result))
+		goto done;
+
+	l_info("request name result = %d", result);
+
+done:
+	return;
+}
+
+static void request_name_tester_callback(struct l_dbus_message *message,
+							void *user_data)
+{
+	const char *error, *text;
+	uint32_t result;
+	struct data *data = user_data;
+
+	if (l_dbus_message_get_error(message, &error, &text)) {
+		l_error("error=%s", error);
+		l_error("message=%s", text);
+		goto done;
+	}
+
+	if (!l_dbus_message_get_arguments(message, "u", &result))
+		goto done;
+
+	l_info("request tester name result = %d", result);
+
+	l_dbus_method_call(data->dbus, "org.freedesktop.DBus",
+			"/org/freedesktop/DBus",
+			"org.freedesktop.DBus", "ReleaseName",
+			release_name_setup,
+			release_name_callback, data, NULL);
+done:
+	return;
+}
+
+static void disconnected(struct l_dbus *dbus, void *user_data)
+{
+	struct tm *test_tm = user_data;
+
+	l_main_quit();
+
+	l_info("client disconnected");
+
+	l_timeout_remove(timeout);
+	timeout = NULL;
+
+	assert(test_tm->tm_sec == 1);
+	assert(test_tm->tm_min == 2);
+	assert(test_tm->tm_hour == 3);
+	assert(test_tm->tm_year == 1970);
+	assert(test_tm->tm_isdst == 0);
+
+	assert(test_tm == tm);
+
+	failure = false;
+}
+
+static void destroy(void *user_data)
+{
+	struct tm *test_tm = user_data;
+
+	assert(test_tm == tm);
+
+	l_free(test_tm);
+}
+
+static void timeout_cb(struct l_timeout *timeout, void *user_data)
+{
+	failure = true;
+
+	l_main_quit();
+}
+
+static struct l_dbus *get_bus(void)
+{
+	struct l_dbus *dbus = NULL;
+	int i;
+
+	for (i = 0; i < 10; i++) {
+		usleep(200 * 1000);
+
+		dbus = l_dbus_new(TEST_BUS_ADDRESS);
+		if (dbus)
+			break;
+	}
+
+	return dbus;
+}
+
+static void request_tester()
+{
+	struct l_dbus *dbus = get_bus();
+	struct data data;
+
+	l_dbus_set_ready_handler(dbus, ready_tester, NULL, NULL);
+
+	data.name = SERVICE_TO_WATCH;
+	data.dbus = dbus;
+
+	l_dbus_method_call(dbus, "org.freedesktop.DBus",
+			"/org/freedesktop/DBus",
+			"org.freedesktop.DBus", "RequestName",
+			request_name_setup,
+			request_name_tester_callback, &data, NULL);
+
+	l_main_run();
+
+	l_dbus_destroy(dbus);
+
+	exit(1);
+}
+
+int main(int argc, char *argv[])
+{
+	struct l_dbus *dbus;
+	struct l_signal *signal;
+	sigset_t mask;
+	unsigned int watch_id;
+	struct data data;
+
+	l_log_set_stderr();
+
+	if (argc > 1 && !strcmp(argv[1], "test"))
+		request_tester();
+
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGINT);
+	sigaddset(&mask, SIGTERM);
+	sigaddset(&mask, SIGCHLD);
+
+	signal = l_signal_create(&mask, signal_handler, NULL, NULL);
+
+	start_dbus_daemon();
+
+	dbus = get_bus();
+
+	l_dbus_set_ready_handler(dbus, ready_callback, argv[0], NULL);
+
+	tm = l_new(struct tm, 1);
+
+	tm->tm_sec = 1;
+	tm->tm_min = 2;
+	tm->tm_hour = 3;
+	tm->tm_year = 1970;
+	tm->tm_isdst = 0;
+
+	failure = true;
+
+	watch_id = l_dbus_add_disconnect_watch(dbus,
+					SERVICE_TO_WATCH,
+					disconnected,
+					tm,
+					destroy);
+
+	data.name = SERVICE_MONITOR;
+	data.dbus = dbus;
+
+	l_dbus_method_call(dbus, "org.freedesktop.DBus",
+			"/org/freedesktop/DBus",
+			"org.freedesktop.DBus", "RequestName",
+			request_name_setup,
+			request_name_callback, &data, NULL);
+
+	timeout = l_timeout_create(5, timeout_cb, dbus, NULL);
+
+	l_main_run();
+
+	l_dbus_remove_watch(dbus, watch_id);
+
+	if (dbus_daemon_pid > 0)
+		kill(dbus_daemon_pid, SIGKILL);
+
+	if (test_daemon_pid > 0)
+		kill(test_daemon_pid, SIGKILL);
+
+	l_dbus_destroy(dbus);
+
+	l_signal_remove(signal);
+
+	l_timeout_remove(timeout);
+
+	assert(failure == false);
+
+	l_info("dbus watch test passed");
+
+	return 0;
+}
-- 
2.1.0

Re: [PATCH v2 1/3] dbus: Add AddMatch and RemoveMatch support

[Denis Kenzior]

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

Hi Jukka,

On 02/12/2015 05:48 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 ++++++++++++++++++++++++++
>   ell/dbus.h |  4 ++++
>   2 files changed, 30 insertions(+)
>
> diff --git a/ell/dbus.c b/ell/dbus.c
> index 4c4ba2d..96a1230 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);
> +}
> +
> +LIB_EXPORT void l_dbus_bus_add_match(struct l_dbus *dbus, const char *rule)

These functions should not be exported / become private.

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

As above

> +	send_match(dbus, rule, "RemoveMatch");
> +}
> diff --git a/ell/dbus.h b/ell/dbus.h
> index a6ad109..d5890a5 100644
> --- a/ell/dbus.h
> +++ b/ell/dbus.h
> @@ -196,6 +196,10 @@ 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);
> +
> +void l_dbus_bus_add_match(struct l_dbus *dbus, const char *rule);
> +void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule);
> +
>   #ifdef __cplusplus
>   }
>   #endif
>

Regards,
-Denis

Re: [PATCH v2 2/3] dbus: Add disconnect watch support

[Denis Kenzior]

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

Hi Jukka,

On 02/12/2015 05:48 AM, Jukka Rissanen wrote:
> Allow caller to monitor the disconnect status of a service.
> ---
>   ell/dbus.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/dbus.h |  15 ++++
>   2 files changed, 242 insertions(+)
>
> diff --git a/ell/dbus.c b/ell/dbus.c
> index 96a1230..f98b042 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"
> @@ -53,6 +54,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,
> @@ -122,6 +125,21 @@ struct signal_callback {
>   	void *user_data;
>   };
>
> +struct 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 *name;
> +	char *owner;

What is the exact difference between name and owner?  I don't see a 
distinction...

> +	char *path;
> +	char *interface;
> +	char *member;
> +	char *argument;
> +	void *user_data;
> +	l_dbus_destroy_func_t destroy_func;
> +};
> +
>   static void message_queue_destroy(void *data)
>   {
>   	struct message_callback *callback = data;
> @@ -1253,3 +1271,212 @@ LIB_EXPORT void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule)
>   {
>   	send_match(dbus, rule, "RemoveMatch");
>   }
> +
> +static void format_rule(struct filter_data *data, char *rule, size_t size)
> +{
> +	const char *sender;
> +	int offset;
> +
> +	offset = snprintf(rule, size, "type='signal'");
> +	sender = data->name ? : data->owner;
> +
> +	if (sender)
> +		offset += snprintf(rule + offset, size - offset,
> +				",sender='%s'", 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 filter_data *data, l_dbus_message_func_t filter)
> +{
> +	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
> +
> +	format_rule(data, rule, sizeof(rule));
> +
> +	l_dbus_bus_add_match(data->dbus, rule);
> +
> +	data->handle_func = filter;
> +}
> +
> +static void remove_match(struct filter_data *data)
> +{
> +	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
> +
> +	format_rule(data, rule, sizeof(rule));
> +
> +	l_dbus_bus_remove_match(data->dbus, rule);
> +}
> +
> +static struct filter_data *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 connect_func,
> +					l_dbus_watch_func_t disconnect_func,
> +					void *user_data,
> +					l_dbus_destroy_func_t destroy)
> +{
> +	struct filter_data *data;
> +	const char *name = NULL, *owner = NULL;
> +
> +	if (sender) {
> +		if (sender[0] == ':')
> +			owner = sender;
> +		else
> +			name = sender;
> +	}

Why is this needed?

> +
> +	data = l_new(struct filter_data, 1);
> +
> +	data->dbus = dbus;
> +	data->handle_func = filter;
> +	data->connect_func = connect_func;
> +	data->disconnect_func = disconnect_func;
> +	data->name = l_strdup(name);
> +	data->owner = l_strdup(owner);
> +	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;
> +}
> +
> +static void filter_data_destroy(void *user_data)
> +{
> +	struct filter_data *data = user_data;
> +
> +	remove_match(data);
> +
> +	l_free(data->name);
> +	l_free(data->owner);
> +	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 message_filter(struct l_dbus_message *message, void *user_data)
> +{
> +	struct filter_data *data = user_data;
> +	const char *sender, *path, *iface, *member, *arg = NULL;
> +
> +	if (_dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_SIGNAL)
> +		return;
> +
> +	sender = l_dbus_message_get_sender(message);
> +	if (!sender)
> +		return;
> +
> +	path = l_dbus_message_get_path(message);
> +	iface = l_dbus_message_get_interface(message);
> +	member = l_dbus_message_get_member(message);
> +
> +	l_dbus_message_get_arguments(message, "s", &arg);
> +
> +	if (data->owner && strcmp(sender, data->owner))
> +		goto out;
> +
> +	if (data->path && strcmp(path, data->path))
> +		goto out;
> +
> +	if (data->interface && strcmp(iface, data->interface))
> +		goto out;
> +
> +	if (data->member && strcmp(member, data->member))
> +		goto out;
> +
> +	if (arg && data->argument && strcmp(arg, data->argument))
> +		goto out;

This looks wrong

> +
> +	if (data->handle_func)
> +		data->handle_func(message, data);
> +
> +out:
> +	return;

Seriously?  Why bother?  Just return straight away.

> +}
> +
> +static void service_filter(struct l_dbus_message *message, void *user_data)
> +{
> +	struct filter_data *data = user_data;
> +	char *name, *old, *new;
> +
> +	if (!l_dbus_message_get_arguments(message, "sss",
> +						&name, &old, &new))
> +		return;
> +
> +	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_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 filter_data *data;
> +
> +	if (!name)
> +		return 0;
> +
> +	if (connect_func)
> +		return -EOPNOTSUPP;

Why? You seem to be handling that just fine?

> +
> +	data = filter_data_get(dbus, service_filter,
> +				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
> +				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
> +				name,
> +				connect_func,
> +				disconnect_func,
> +				user_data,
> +				destroy);
> +	if (!data)
> +		return 0;
> +
> +	return l_dbus_register(dbus, message_filter, 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, NULL, 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 d5890a5..4a17de7 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);
> @@ -200,6 +202,19 @@ bool l_dbus_unregister_interface(struct l_dbus *dbus, const char *path,
>   void l_dbus_bus_add_match(struct l_dbus *dbus, const char *rule);
>   void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule);
>
> +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);
> +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 v2 3/3] unit: dbus: Add test for disconnect watch API

[Denis Kenzior]

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

On 02/12/2015 05:48 AM, Jukka Rissanen wrote:
> Test the dbus disconnect watch support.
> ---
>   Makefile.am            |   3 +
>   unit/test-dbus-watch.c | 388 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 391 insertions(+)
>   create mode 100644 unit/test-dbus-watch.c
>

That seems like an awful lot of copy-paste. Can we isolate the actual 
signal dispatcher and unit test it properly?  For example, you can 
always make the message_filter function private (e.g. without 
LIB_EXPORT) instead of static and feed it signals created using e.g. 
l_dbus_message_new_signal or dbus_message_from_blob

Alternatively, fold this into test-dbus.c if possible.

Regards,
-Denis

Re: [PATCH v2 2/3] dbus: Add disconnect watch support

[Jukka Rissanen]

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

Hi Denis,

On to, 2015-02-12 at 21:25 -0600, Denis Kenzior wrote:
> Hi Jukka,
> 
> On 02/12/2015 05:48 AM, Jukka Rissanen wrote:
> > Allow caller to monitor the disconnect status of a service.
> > ---
> >   ell/dbus.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   ell/dbus.h |  15 ++++
> >   2 files changed, 242 insertions(+)
> >
> > diff --git a/ell/dbus.c b/ell/dbus.c
> > index 96a1230..f98b042 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"
> > @@ -53,6 +54,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,
> > @@ -122,6 +125,21 @@ struct signal_callback {
> >   	void *user_data;
> >   };
> >
> > +struct 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 *name;
> > +	char *owner;
> 
> What is the exact difference between name and owner?  I don't see a 
> distinction...

name is something like "org.foobar" and owner is ":1.101"

User registers watch using a name and dbus-daemon sends its
notifications about NameOwnerChanged signals using the owner format so
we need to track both formats.

> 
> > +	char *path;
> > +	char *interface;
> > +	char *member;
> > +	char *argument;
> > +	void *user_data;
> > +	l_dbus_destroy_func_t destroy_func;
> > +};
> > +
> >   static void message_queue_destroy(void *data)
> >   {
> >   	struct message_callback *callback = data;
> > @@ -1253,3 +1271,212 @@ LIB_EXPORT void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule)
> >   {
> >   	send_match(dbus, rule, "RemoveMatch");
> >   }
> > +
> > +static void format_rule(struct filter_data *data, char *rule, size_t size)
> > +{
> > +	const char *sender;
> > +	int offset;
> > +
> > +	offset = snprintf(rule, size, "type='signal'");
> > +	sender = data->name ? : data->owner;
> > +
> > +	if (sender)
> > +		offset += snprintf(rule + offset, size - offset,
> > +				",sender='%s'", 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 filter_data *data, l_dbus_message_func_t filter)
> > +{
> > +	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
> > +
> > +	format_rule(data, rule, sizeof(rule));
> > +
> > +	l_dbus_bus_add_match(data->dbus, rule);
> > +
> > +	data->handle_func = filter;
> > +}
> > +
> > +static void remove_match(struct filter_data *data)
> > +{
> > +	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
> > +
> > +	format_rule(data, rule, sizeof(rule));
> > +
> > +	l_dbus_bus_remove_match(data->dbus, rule);
> > +}
> > +
> > +static struct filter_data *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 connect_func,
> > +					l_dbus_watch_func_t disconnect_func,
> > +					void *user_data,
> > +					l_dbus_destroy_func_t destroy)
> > +{
> > +	struct filter_data *data;
> > +	const char *name = NULL, *owner = NULL;
> > +
> > +	if (sender) {
> > +		if (sender[0] == ':')
> > +			owner = sender;
> > +		else
> > +			name = sender;
> > +	}
> 
> Why is this needed?

See the answer above.

> 
> > +
> > +	data = l_new(struct filter_data, 1);
> > +
> > +	data->dbus = dbus;
> > +	data->handle_func = filter;
> > +	data->connect_func = connect_func;
> > +	data->disconnect_func = disconnect_func;
> > +	data->name = l_strdup(name);
> > +	data->owner = l_strdup(owner);
> > +	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;
> > +}
> > +
> > +static void filter_data_destroy(void *user_data)
> > +{
> > +	struct filter_data *data = user_data;
> > +
> > +	remove_match(data);
> > +
> > +	l_free(data->name);
> > +	l_free(data->owner);
> > +	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 message_filter(struct l_dbus_message *message, void *user_data)
> > +{
> > +	struct filter_data *data = user_data;
> > +	const char *sender, *path, *iface, *member, *arg = NULL;
> > +
> > +	if (_dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_SIGNAL)
> > +		return;
> > +
> > +	sender = l_dbus_message_get_sender(message);
> > +	if (!sender)
> > +		return;
> > +
> > +	path = l_dbus_message_get_path(message);
> > +	iface = l_dbus_message_get_interface(message);
> > +	member = l_dbus_message_get_member(message);
> > +
> > +	l_dbus_message_get_arguments(message, "s", &arg);
> > +
> > +	if (data->owner && strcmp(sender, data->owner))
> > +		goto out;
> > +
> > +	if (data->path && strcmp(path, data->path))
> > +		goto out;
> > +
> > +	if (data->interface && strcmp(iface, data->interface))
> > +		goto out;
> > +
> > +	if (data->member && strcmp(member, data->member))
> > +		goto out;
> > +
> > +	if (arg && data->argument && strcmp(arg, data->argument))
> > +		goto out;
> 
> This looks wrong

Please elaborate more?

> 
> > +
> > +	if (data->handle_func)
> > +		data->handle_func(message, data);
> > +
> > +out:
> > +	return;
> 
> Seriously?  Why bother?  Just return straight away.

Sure.

> 
> > +}
> > +
> > +static void service_filter(struct l_dbus_message *message, void *user_data)
> > +{
> > +	struct filter_data *data = user_data;
> > +	char *name, *old, *new;
> > +
> > +	if (!l_dbus_message_get_arguments(message, "sss",
> > +						&name, &old, &new))
> > +		return;
> > +
> > +	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_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 filter_data *data;
> > +
> > +	if (!name)
> > +		return 0;
> > +
> > +	if (connect_func)
> > +		return -EOPNOTSUPP;
> 
> Why? You seem to be handling that just fine?

No, connect watch needs more work as more code needs to be done. I only
needed the disconnect watch for my app so in order to speed things up,
left the connect to be done later when such need arises. The connect
pointer is in the API so that we do not need to change the API later.

> 
> > +
> > +	data = filter_data_get(dbus, service_filter,
> > +				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
> > +				DBUS_INTERFACE_DBUS, "NameOwnerChanged",
> > +				name,
> > +				connect_func,
> > +				disconnect_func,
> > +				user_data,
> > +				destroy);
> > +	if (!data)
> > +		return 0;
> > +
> > +	return l_dbus_register(dbus, message_filter, 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, NULL, 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 d5890a5..4a17de7 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);
> > @@ -200,6 +202,19 @@ bool l_dbus_unregister_interface(struct l_dbus *dbus, const char *path,
> >   void l_dbus_bus_add_match(struct l_dbus *dbus, const char *rule);
> >   void l_dbus_bus_remove_match(struct l_dbus *dbus, const char *rule);
> >
> > +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);
> > +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


Jukka

Re: [PATCH v2 3/3] unit: dbus: Add test for disconnect watch API

[Jukka Rissanen]

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

On to, 2015-02-12 at 21:36 -0600, Denis Kenzior wrote:
> On 02/12/2015 05:48 AM, Jukka Rissanen wrote:
> > Test the dbus disconnect watch support.
> > ---
> >   Makefile.am            |   3 +
> >   unit/test-dbus-watch.c | 388 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 391 insertions(+)
> >   create mode 100644 unit/test-dbus-watch.c
> >
> 
> That seems like an awful lot of copy-paste. Can we isolate the actual 
> signal dispatcher and unit test it properly?  For example, you can 
> always make the message_filter function private (e.g. without 
> LIB_EXPORT) instead of static and feed it signals created using e.g. 
> l_dbus_message_new_signal or dbus_message_from_blob

I did not quite understand your comment about signal dispatcher and
message_filter here, please elaborate a bit more?

> 
> Alternatively, fold this into test-dbus.c if possible.

I thought about that but test-dbus.c was already quite big. But if you
are ok with that I can certainly do that too.

> 
> Regards,
> -Denis

Jukka

Re: [PATCH v2 2/3] dbus: Add disconnect watch support

[Denis Kenzior]

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

Hi Jukka,

<snip>

 >>> +struct 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 *name;
>>> +	char *owner;
>>
>> What is the exact difference between name and owner?  I don't see a
>> distinction...
>
> name is something like "org.foobar" and owner is ":1.101"
>
> User registers watch using a name and dbus-daemon sends its
> notifications about NameOwnerChanged signals using the owner format so
> we need to track both formats.

Yes, but you don't actually handle that anywhere.  So I don't see a 
distinction in the current code.  If this is TBD, then simply don't 
include this and mention somewhere that only unique bus names are supported.

<snip>

>>> +	if (arg && data->argument && strcmp(arg, data->argument))
>>> +		goto out;
>>
>> This looks wrong
>
> Please elaborate more?
>

If arg is NULL, the matching continues.  I do not think that is what you 
want, but I've not read the DBus spec lately.  This needs a unit test case.

<snip>

>>> +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 filter_data *data;
>>> +
>>> +	if (!name)
>>> +		return 0;
>>> +
>>> +	if (connect_func)
>>> +		return -EOPNOTSUPP;
>>
>> Why? You seem to be handling that just fine?
>
> No, connect watch needs more work as more code needs to be done. I only
> needed the disconnect watch for my app so in order to speed things up,
> left the connect to be done later when such need arises. The connect
> pointer is in the API so that we do not need to change the API later.
>

Okay.  But this is still wrong.  The return value is an unsigned int. 
You're returning a negative errno.

Might want to make this into an internal private function for now.

Regards,
-Denis

Re: [PATCH v2 3/3] unit: dbus: Add test for disconnect watch API

[Denis Kenzior]

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

Hi Jukka,

On 02/13/2015 02:40 AM, Jukka Rissanen wrote:
> On to, 2015-02-12 at 21:36 -0600, Denis Kenzior wrote:
>> On 02/12/2015 05:48 AM, Jukka Rissanen wrote:
>>> Test the dbus disconnect watch support.
>>> ---
>>>    Makefile.am            |   3 +
>>>    unit/test-dbus-watch.c | 388 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 391 insertions(+)
>>>    create mode 100644 unit/test-dbus-watch.c
>>>
>>
>> That seems like an awful lot of copy-paste. Can we isolate the actual
>> signal dispatcher and unit test it properly?  For example, you can
>> always make the message_filter function private (e.g. without
>> LIB_EXPORT) instead of static and feed it signals created using e.g.
>> l_dbus_message_new_signal or dbus_message_from_blob
>
> I did not quite understand your comment about signal dispatcher and
> message_filter here, please elaborate a bit more?
>

Look at how unit/test-dbus-service.c is done.  Almost every part of the 
dbus-service code can be tested standalone.  You need to think about how 
to do the same.

For example:
	static void message_filter(...) -> void _dbus_message_filter(...)
	static void format_rule(...) -> void _dbus1_format_rule()

Or maybe even allow the entire handle_signal() path to be tested.

Then test this functionality in the unit test.  If you're relying on 
semi-manual tests for this stuff, your test coverage will be pretty small.

Regards,
-Denis

>>
>> Alternatively, fold this into test-dbus.c if possible.
>
> I thought about that but test-dbus.c was already quite big. But if you
> are ok with that I can certainly do that too.
>

This is the least preferred option.  Only do this for stuff that can't 
be tested via the unit test.

Regards,
-Denis

Re: [PATCH v2 2/3] dbus: Add disconnect watch support

[Jukka Rissanen]

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

On pe, 2015-02-13 at 08:01 -0600, Denis Kenzior wrote:
> Hi Jukka,
> 
> <snip>
> 
>  >>> +struct 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 *name;
> >>> +	char *owner;
> >>
> >> What is the exact difference between name and owner?  I don't see a
> >> distinction...
> >
> > name is something like "org.foobar" and owner is ":1.101"
> >
> > User registers watch using a name and dbus-daemon sends its
> > notifications about NameOwnerChanged signals using the owner format so
> > we need to track both formats.
> 
> Yes, but you don't actually handle that anywhere.  So I don't see a 
> distinction in the current code.  If this is TBD, then simply don't 
> include this and mention somewhere that only unique bus names are supported.

Hmm, owner and name are handled differently in the code. For example see
the format_rule() function.

> 
> <snip>
> 
> >>> +	if (arg && data->argument && strcmp(arg, data->argument))
> >>> +		goto out;
> >>
> >> This looks wrong
> >
> > Please elaborate more?
> >
> 
> If arg is NULL, the matching continues.  I do not think that is what you 
> want, but I've not read the DBus spec lately.  This needs a unit test case.
> 
> <snip>
> 
> >>> +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 filter_data *data;
> >>> +
> >>> +	if (!name)
> >>> +		return 0;
> >>> +
> >>> +	if (connect_func)
> >>> +		return -EOPNOTSUPP;
> >>
> >> Why? You seem to be handling that just fine?
> >
> > No, connect watch needs more work as more code needs to be done. I only
> > needed the disconnect watch for my app so in order to speed things up,
> > left the connect to be done later when such need arises. The connect
> > pointer is in the API so that we do not need to change the API later.
> >
> 
> Okay.  But this is still wrong.  The return value is an unsigned int. 
> You're returning a negative errno.

Yep, clearly a bug.

> 
> Might want to make this into an internal private function for now.

Ok.

> 
> Regards,
> -Denis


Jukka

Re: [PATCH v2 3/3] unit: dbus: Add test for disconnect watch API

[Jukka Rissanen]

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

On pe, 2015-02-13 at 08:10 -0600, Denis Kenzior wrote:
> Hi Jukka,
> 
> On 02/13/2015 02:40 AM, Jukka Rissanen wrote:
> > On to, 2015-02-12 at 21:36 -0600, Denis Kenzior wrote:
> >> On 02/12/2015 05:48 AM, Jukka Rissanen wrote:
> >>> Test the dbus disconnect watch support.
> >>> ---
> >>>    Makefile.am            |   3 +
> >>>    unit/test-dbus-watch.c | 388 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>    2 files changed, 391 insertions(+)
> >>>    create mode 100644 unit/test-dbus-watch.c
> >>>
> >>
> >> That seems like an awful lot of copy-paste. Can we isolate the actual
> >> signal dispatcher and unit test it properly?  For example, you can
> >> always make the message_filter function private (e.g. without
> >> LIB_EXPORT) instead of static and feed it signals created using e.g.
> >> l_dbus_message_new_signal or dbus_message_from_blob
> >
> > I did not quite understand your comment about signal dispatcher and
> > message_filter here, please elaborate a bit more?
> >
> 
> Look at how unit/test-dbus-service.c is done.  Almost every part of the 
> dbus-service code can be tested standalone.  You need to think about how 
> to do the same.
> 
> For example:
> 	static void message_filter(...) -> void _dbus_message_filter(...)
> 	static void format_rule(...) -> void _dbus1_format_rule()
> 
> Or maybe even allow the entire handle_signal() path to be tested.
> 
> Then test this functionality in the unit test.  If you're relying on 
> semi-manual tests for this stuff, your test coverage will be pretty small.
> 

But don't we want to test this against dbus-daemon? Testing this watch
API without dbus-daemon interaction seems pointless to me.

> Regards,
> -Denis
> 
> >>
> >> Alternatively, fold this into test-dbus.c if possible.
> >
> > I thought about that but test-dbus.c was already quite big. But if you
> > are ok with that I can certainly do that too.
> >
> 
> This is the least preferred option.  Only do this for stuff that can't 
> be tested via the unit test.
> 
> Regards,
> -Denis
> 


Jukka

Re: [PATCH v2 2/3] dbus: Add disconnect watch support

[Denis Kenzior]

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

Hi Jukka,

On 02/13/2015 08:23 AM, Jukka Rissanen wrote:
> On pe, 2015-02-13 at 08:01 -0600, Denis Kenzior wrote:
>> Hi Jukka,
>>
>> <snip>
>>
>>   >>> +struct 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 *name;
>>>>> +	char *owner;
>>>>
>>>> What is the exact difference between name and owner?  I don't see a
>>>> distinction...
>>>
>>> name is something like "org.foobar" and owner is ":1.101"
>>>
>>> User registers watch using a name and dbus-daemon sends its
>>> notifications about NameOwnerChanged signals using the owner format so
>>> we need to track both formats.
>>
>> Yes, but you don't actually handle that anywhere.  So I don't see a
>> distinction in the current code.  If this is TBD, then simply don't
>> include this and mention somewhere that only unique bus names are supported.
>
> Hmm, owner and name are handled differently in the code. For example see
> the format_rule() function.
>

Then help me understand.  You have:

in format_rule you have:
+       offset = snprintf(rule, size, "type='signal'");
+       sender = data->name ? : data->owner;
+
+       if (sender)
+               offset += snprintf(rule + offset, size - offset,
+                               ",sender='%s'", sender);

in filter_data_get you have:
+       if (sender) {
+               if (sender[0] == ':')
+                       owner = sender;
+               else
+                       name = sender;
+       }

So why not replace everything by simply sender?  Right now none of this 
makes sense to me since you're not traking the well-known-name -> unique 
name mapping.

If you only support unique names, just say so and make the code reflect 
that.  Don't try to sneak code in that is part of some future 
functionality.  I'm not a mind reader.

Regards,
-Denis

Re: [PATCH v2 3/3] unit: dbus: Add test for disconnect watch API

[Denis Kenzior]

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

Hi Jukka,

 >>
>> Then test this functionality in the unit test.  If you're relying on
>> semi-manual tests for this stuff, your test coverage will be pretty small.
>>
>
> But don't we want to test this against dbus-daemon? Testing this watch
> API without dbus-daemon interaction seems pointless to me.

Why?  In this case, what's the difference between getting a signal from 
DBus Daemon vs simply making one up yourself?  Isolating the code and 
unit testing the hell out of it is much simpler without dbus-daemon 
involved.  Much faster and more compact too.

Regards,
-Denis

Re: [PATCH v2 2/3] dbus: Add disconnect watch support

[Jukka Rissanen]

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

On pe, 2015-02-13 at 09:05 -0600, Denis Kenzior wrote:
> Hi Jukka,
> 
> On 02/13/2015 08:23 AM, Jukka Rissanen wrote:
> > On pe, 2015-02-13 at 08:01 -0600, Denis Kenzior wrote:
> >> Hi Jukka,
> >>
> >> <snip>
> >>
> >>   >>> +struct 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 *name;
> >>>>> +	char *owner;
> >>>>
> >>>> What is the exact difference between name and owner?  I don't see a
> >>>> distinction...
> >>>
> >>> name is something like "org.foobar" and owner is ":1.101"
> >>>
> >>> User registers watch using a name and dbus-daemon sends its
> >>> notifications about NameOwnerChanged signals using the owner format so
> >>> we need to track both formats.
> >>
> >> Yes, but you don't actually handle that anywhere.  So I don't see a
> >> distinction in the current code.  If this is TBD, then simply don't
> >> include this and mention somewhere that only unique bus names are supported.
> >
> > Hmm, owner and name are handled differently in the code. For example see
> > the format_rule() function.
> >
> 
> Then help me understand.  You have:
> 
> in format_rule you have:
> +       offset = snprintf(rule, size, "type='signal'");
> +       sender = data->name ? : data->owner;
> +
> +       if (sender)
> +               offset += snprintf(rule + offset, size - offset,
> +                               ",sender='%s'", sender);
> 
> in filter_data_get you have:
> +       if (sender) {
> +               if (sender[0] == ':')
> +                       owner = sender;
> +               else
> +                       name = sender;
> +       }
> 
> So why not replace everything by simply sender?  Right now none of this 
> makes sense to me since you're not traking the well-known-name -> unique 
> name mapping.
> 
> If you only support unique names, just say so and make the code reflect 
> that.  Don't try to sneak code in that is part of some future 
> functionality.  I'm not a mind reader.

The owner/name distinction is indeed needed only for watching dbus
connects, and as we do not implement the connect support yet I will
remove this code.


Jukka

Re: [PATCH v2 3/3] unit: dbus: Add test for disconnect watch API

[Jukka Rissanen]

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

On pe, 2015-02-13 at 09:09 -0600, Denis Kenzior wrote:
> Hi Jukka,
> 
>  >>
> >> Then test this functionality in the unit test.  If you're relying on
> >> semi-manual tests for this stuff, your test coverage will be pretty small.
> >>
> >
> > But don't we want to test this against dbus-daemon? Testing this watch
> > API without dbus-daemon interaction seems pointless to me.
> 
> Why?  In this case, what's the difference between getting a signal from 
> DBus Daemon vs simply making one up yourself?  Isolating the code and 
> unit testing the hell out of it is much simpler without dbus-daemon 
> involved.  Much faster and more compact too.

We have already tests for receiving signals so it seems pointless to
test them again. To me it looks more useful to test that we really get
NameOwnerChanged signal from dbus-daemon when we quit the test app.
Testing real interaction is more complex (as the code shows) but at
least we know that the watch support works in real life.


Jukka

Re: [PATCH v2 3/3] unit: dbus: Add test for disconnect watch API

[Denis Kenzior]

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

Hi Jukka,

On 02/16/2015 04:53 AM, Jukka Rissanen wrote:
> On pe, 2015-02-13 at 09:09 -0600, Denis Kenzior wrote:
>> Hi Jukka,
>>
>>   >>
>>>> Then test this functionality in the unit test.  If you're relying on
>>>> semi-manual tests for this stuff, your test coverage will be pretty small.
>>>>
>>>
>>> But don't we want to test this against dbus-daemon? Testing this watch
>>> API without dbus-daemon interaction seems pointless to me.
>>
>> Why?  In this case, what's the difference between getting a signal from
>> DBus Daemon vs simply making one up yourself?  Isolating the code and
>> unit testing the hell out of it is much simpler without dbus-daemon
>> involved.  Much faster and more compact too.
>
> We have already tests for receiving signals so it seems pointless to
> test them again. To me it looks more useful to test that we really get

That is not what I told you to test ;)  I told you to unit test the 
match creation functionality and the signal match / dispatch code.

> NameOwnerChanged signal from dbus-daemon when we quit the test app.

You just said yourself that signal reception works.  So why are you 
testing this?  Are you testing the DBus Daemon or ell?

> Testing real interaction is more complex (as the code shows) but at
> least we know that the watch support works in real life.
>

Unit test the individual pieces first.  If you know the individual parts 
are working well, then you can move on to whole system testing, if needed.

Regards,
-Denis