[PATCH 1/2] dbus: Add private _dbus_message_new_error

[PATCH 1/2] dbus: Add private _dbus_message_new_error

[Andrew Zaborowski]

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

---
 ell/dbus-message.c | 47 +++++++++++++++++++++++++++++++----------------
 ell/dbus-private.h |  5 +++++
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index a409e53..86cec96 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -328,33 +328,32 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_new_method_return(
 	return message;
 }
 
-LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
-					struct l_dbus_message *method_call,
-					const char *name,
-					const char *format, va_list args)
+struct l_dbus_message *_dbus_message_new_error(uint8_t version,
+						uint32_t reply_serial,
+						const char *destination,
+						const char *name,
+						const char *error)
 {
-	char str[1024];
 	struct l_dbus_message *reply;
-	struct dbus_header *hdr = method_call->header;
-	const char *sender;
+	bool r;
 
 	if (!_dbus_valid_interface(name))
 		return NULL;
 
-	vsnprintf(str, sizeof(str), format, args);
 	reply = message_new_common(DBUS_MESSAGE_TYPE_ERROR,
 					DBUS_MESSAGE_FLAG_NO_REPLY_EXPECTED,
-					hdr->version);
-
-	reply->reply_serial = _dbus_message_get_serial(method_call);
-
-	sender = l_dbus_message_get_sender(method_call);
-	if (sender)
-		reply->destination = l_strdup(sender);
+					version);
 
 	reply->error_name = l_strdup(name);
+	reply->destination = l_strdup(destination);
+	reply->reply_serial = reply_serial;
 
-	if (!l_dbus_message_set_arguments(reply, "s", str)) {
+	if (error)
+		r = l_dbus_message_set_arguments(reply, "s", error);
+	else
+		r = l_dbus_message_set_arguments(reply, "");
+
+	if (!r) {
 		l_dbus_message_unref(reply);
 		return NULL;
 	}
@@ -362,6 +361,22 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
 	return reply;
 }
 
+LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
+					struct l_dbus_message *method_call,
+					const char *name,
+					const char *format, va_list args)
+{
+	char str[1024];
+	struct dbus_header *hdr = method_call->header;
+
+	vsnprintf(str, sizeof(str), format, args);
+
+	return _dbus_message_new_error(hdr->version,
+					_dbus_message_get_serial(method_call),
+					l_dbus_message_get_sender(method_call),
+					name, str);
+}
+
 LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error(
 					struct l_dbus_message *method_call,
 					const char *name,
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 63c239b..030ce0b 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -141,6 +141,11 @@ struct l_dbus_message *_dbus_message_new_signal(uint8_t version,
 						const char *path,
 						const char *interface,
 						const char *name);
+struct l_dbus_message *_dbus_message_new_error(uint8_t version,
+						uint32_t reply_serial,
+						const char *destination,
+						const char *name,
+						const char *error);
 
 struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size);
 struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
-- 
2.5.0

[PATCH 2/2] dbus: Handle kdbus KDBUS_ITEM_REPLY_TIMEOUT / _DEAD

[Andrew Zaborowski]

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

Generate DBus1-like error messages on kernel error notifications.
---
 ell/dbus-kernel.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/ell/dbus-kernel.c b/ell/dbus-kernel.c
index b5a07a5..4fd2f29 100644
--- a/ell/dbus-kernel.c
+++ b/ell/dbus-kernel.c
@@ -559,6 +559,7 @@ int _dbus_kernel_recv(int fd, void *kdbus_pool,
 	struct kdbus_item *item;
 	int r;
 	size_t min_size;
+	const char *error;
 
 	memset(&recv_cmd, 0, sizeof(recv_cmd));
 
@@ -601,6 +602,23 @@ int _dbus_kernel_recv(int fd, void *kdbus_pool,
 						user_data);
 			break;
 
+		case KDBUS_ITEM_REPLY_TIMEOUT:
+		case KDBUS_ITEM_REPLY_DEAD:
+			if (item->type == KDBUS_ITEM_REPLY_TIMEOUT)
+				error = "Did not receive a reply.";
+			else
+				error = "Message recipient disconnected from "
+					"message bus without replying.";
+
+			dbus_msg = _dbus_message_new_error(
+					2, msg->cookie_reply, NULL,
+					"org.freedesktop.DBus.Error.NoReply",
+					error);
+			if (dbus_msg)
+				message_func(dbus_msg, user_data);
+
+			break;
+
 		default:
 			break;
 		}
-- 
2.5.0

[PATCH] dbus: Check message->sealed before get_header_field()

[Andrew Zaborowski]

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

Also make sure _dbus_message_get_reply_serial on kdbus doesn't overwrite
message->reply_serial with uninitialised value.
---
 ell/dbus-message.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 86cec96..aecf398 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -1336,7 +1336,7 @@ LIB_EXPORT const char *l_dbus_message_get_path(struct l_dbus_message *message)
 	if (unlikely(!message))
 		return NULL;
 
-	if (!message->path)
+	if (!message->path && message->sealed)
 		get_header_field(message, DBUS_MESSAGE_FIELD_PATH, 'o',
 					&message->path);
 
@@ -1348,7 +1348,7 @@ LIB_EXPORT const char *l_dbus_message_get_interface(struct l_dbus_message *messa
 	if (unlikely(!message))
 		return NULL;
 
-	if (!message->interface)
+	if (!message->interface && message->sealed)
 		get_header_field(message, DBUS_MESSAGE_FIELD_INTERFACE, 's',
 					&message->interface);
 
@@ -1360,7 +1360,7 @@ LIB_EXPORT const char *l_dbus_message_get_member(struct l_dbus_message *message)
 	if (unlikely(!message))
 		return NULL;
 
-	if (!message->member)
+	if (!message->member && message->sealed)
 		get_header_field(message, DBUS_MESSAGE_FIELD_MEMBER, 's',
 					&message->member);
 
@@ -1372,7 +1372,7 @@ LIB_EXPORT const char *l_dbus_message_get_destination(struct l_dbus_message *mes
 	if (unlikely(!message))
 		return NULL;
 
-	if (!message->destination)
+	if (!message->destination && message->sealed)
 		get_header_field(message, DBUS_MESSAGE_FIELD_DESTINATION, 's',
 							&message->destination);
 
@@ -1384,7 +1384,7 @@ LIB_EXPORT const char *l_dbus_message_get_sender(struct l_dbus_message *message)
 	if (unlikely(!message))
 		return NULL;
 
-	if (!message->sender)
+	if (!message->sender && message->sealed)
 		get_header_field(message, DBUS_MESSAGE_FIELD_SENDER, 's',
 					&message->sender);
 
@@ -1405,9 +1405,9 @@ uint32_t _dbus_message_get_reply_serial(struct l_dbus_message *message)
 	if (unlikely(!message))
 		return 0;
 
-	if (message->reply_serial == 0) {
+	if (message->reply_serial == 0 && message->sealed) {
 		if (_dbus_message_is_gvariant(message)) {
-			uint64_t reply_serial;
+			uint64_t reply_serial = 0;
 
 			get_header_field(message,
 					DBUS_MESSAGE_FIELD_REPLY_SERIAL, 't',
-- 
2.5.0

Re: [PATCH] dbus: Check message->sealed before get_header_field()

[Denis Kenzior]

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

Hi Andrew,

On 04/18/2016 09:31 AM, Andrew Zaborowski wrote:
> Also make sure _dbus_message_get_reply_serial on kdbus doesn't overwrite
> message->reply_serial with uninitialised value.
> ---
>   ell/dbus-message.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)

Applied, thanks.

Regards,
-Denis

[PATCH] dbus: Don't send replies to messages with no reply flag

[Andrew Zaborowski]

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

Kdbus doesn't store the cookies for messages that have the no reply flag
and throws error when a reply is sent with reply_cookie that it doesn't
know.  It's not fatal, but we also save some cycles by not sending the
message.

We mark those replies with reply_serial == 0, rather than setting a
"discard" flag on the message, because the method call's serial is
effectively an invalid reply_serial value.
---
 ell/dbus-message.c | 10 +++++++---
 ell/dbus.c         | 10 ++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index aecf398..9286f0e 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -319,7 +319,8 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_new_method_return(
 					DBUS_MESSAGE_FLAG_NO_REPLY_EXPECTED,
 					hdr->version);
 
-	message->reply_serial = _dbus_message_get_serial(method_call);
+	if (!l_dbus_message_get_no_reply(method_call))
+		message->reply_serial = _dbus_message_get_serial(method_call);
 
 	sender = l_dbus_message_get_sender(method_call);
 	if (sender)
@@ -368,11 +369,14 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
 {
 	char str[1024];
 	struct dbus_header *hdr = method_call->header;
+	uint32_t reply_serial = 0;
 
 	vsnprintf(str, sizeof(str), format, args);
 
-	return _dbus_message_new_error(hdr->version,
-					_dbus_message_get_serial(method_call),
+	if (!l_dbus_message_get_no_reply(method_call))
+		reply_serial = _dbus_message_get_serial(method_call);
+
+	return _dbus_message_new_error(hdr->version, reply_serial,
 					l_dbus_message_get_sender(method_call),
 					name, str);
 }
diff --git a/ell/dbus.c b/ell/dbus.c
index 9bb47ae..84b3fa1 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -325,6 +325,16 @@ static uint32_t send_message(struct l_dbus *dbus, bool priority,
 				void *user_data, l_dbus_destroy_func_t destroy)
 {
 	struct message_callback *callback;
+	enum dbus_message_type type;
+
+	type = _dbus_message_get_type(message);
+
+	if ((type == DBUS_MESSAGE_TYPE_METHOD_RETURN ||
+				type == DBUS_MESSAGE_TYPE_ERROR) &&
+			_dbus_message_get_reply_serial(message) == 0) {
+		l_dbus_message_unref(message);
+		return 0;
+	}
 
 	callback = l_new(struct message_callback, 1);
 
-- 
2.5.0

[PATCH] unit: Add test-dbus-properties --kdbus option

[Andrew Zaborowski]

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

---
 unit/test-dbus-properties.c | 49 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/unit/test-dbus-properties.c b/unit/test-dbus-properties.c
index c7ed384..3238cf6 100644
--- a/unit/test-dbus-properties.c
+++ b/unit/test-dbus-properties.c
@@ -27,13 +27,19 @@
 #include <unistd.h>
 #include <stdlib.h>
 #include <sys/wait.h>
+#include <stdio.h>
 
 #include <ell/ell.h>
+#include <ell/dbus-private.h>
 
 #define TEST_BUS_ADDRESS "unix:path=/tmp/ell-test-bus"
 
 static pid_t dbus_daemon_pid = -1;
 
+static int kdbus_fd = -1;
+
+static char bus_address[128];
+
 static void start_dbus_daemon(void)
 {
 	char *prg_argv[6];
@@ -65,6 +71,24 @@ static void start_dbus_daemon(void)
 	l_info("dbus-daemon process %d created", pid);
 
 	dbus_daemon_pid = pid;
+
+	strcpy(bus_address, TEST_BUS_ADDRESS);
+}
+
+static void create_kdbus(void)
+{
+	char bus_name[64];
+
+	snprintf(bus_name, sizeof(bus_name), "%u-ell-test", getuid());
+
+	kdbus_fd = _dbus_kernel_create_bus(bus_name);
+	if (kdbus_fd < 0) {
+		l_warn("kdbus not available");
+		return;
+	}
+
+	snprintf(bus_address, sizeof(bus_address),
+				"kernel:path=/dev/kdbus/%s/bus", bus_name);
 }
 
 static void signal_handler(struct l_signal *signal, uint32_t signo,
@@ -164,6 +188,9 @@ static void request_name_callback(struct l_dbus *dbus, bool success,
 static void ready_callback(void *user_data)
 {
 	l_info("ready");
+
+	l_dbus_name_acquire(dbus, "org.test", false, false, false,
+				request_name_callback, NULL);
 }
 
 static void disconnect_callback(void *user_data)
@@ -894,6 +921,12 @@ int main(int argc, char *argv[])
 	struct l_signal *signal;
 	sigset_t mask;
 	int i;
+	bool kdbus = false;
+
+	for (i = 1; i < argc; i++) {
+		if (!strcmp(argv[i], "--kdbus"))
+			kdbus = true;
+	}
 
 	sigemptyset(&mask);
 	sigaddset(&mask, SIGINT);
@@ -904,12 +937,18 @@ int main(int argc, char *argv[])
 
 	l_log_set_stderr();
 
-	start_dbus_daemon();
+	if (kdbus)
+		create_kdbus();
+	else
+		start_dbus_daemon();
+
+	if (!bus_address[0])
+		return -1;
 
 	for (i = 0; i < 10; i++) {
 		usleep(200 * 1000);
 
-		dbus = l_dbus_new(TEST_BUS_ADDRESS);
+		dbus = l_dbus_new(bus_address);
 		if (dbus)
 			break;
 	}
@@ -917,9 +956,6 @@ int main(int argc, char *argv[])
 	l_dbus_set_ready_handler(dbus, ready_callback, dbus, NULL);
 	l_dbus_set_disconnect_handler(dbus, disconnect_callback, NULL, NULL);
 
-	l_dbus_name_acquire(dbus, "org.test", false, false, false,
-				request_name_callback, NULL);
-
 	if (!l_dbus_register_interface(dbus, "org.test", setup_test_interface,
 					NULL, true)) {
 		l_info("Unable to register interface");
@@ -977,6 +1013,9 @@ done:
 	if (dbus_daemon_pid > 0)
 		kill(dbus_daemon_pid, SIGKILL);
 
+	if (kdbus_fd >= 0)
+		close(kdbus_fd);
+
 	l_signal_remove(signal);
 
 	if (!success)
-- 
2.5.0

Re: [PATCH] unit: Add test-dbus-properties --kdbus option

[Denis Kenzior]

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

Hi Andrew,

On 04/18/2016 09:31 AM, Andrew Zaborowski wrote:
> ---
>   unit/test-dbus-properties.c | 49 ++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 44 insertions(+), 5 deletions(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH 1/5] dbus: Add a Bus Name cache

[Andrew Zaborowski]

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

---
 Makefile.am           |   1 +
 ell/dbus-name-cache.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/dbus-private.h    |  18 ++++++
 ell/dbus.c            |  35 +++++++++---
 4 files changed, 196 insertions(+), 7 deletions(-)
 create mode 100644 ell/dbus-name-cache.c

diff --git a/Makefile.am b/Makefile.am
index 7283a7e..3e8e3ef 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -75,6 +75,7 @@ ell_libell_la_SOURCES = $(linux_headers) \
 			ell/dbus-message.c \
 			ell/dbus-util.c \
 			ell/dbus-service.c \
+			ell/dbus-name-cache.c \
 			ell/dbus-filter.c \
 			ell/gvariant-private.h \
 			ell/gvariant-util.c \
diff --git a/ell/dbus-name-cache.c b/ell/dbus-name-cache.c
new file mode 100644
index 0000000..76b1792
--- /dev/null
+++ b/ell/dbus-name-cache.c
@@ -0,0 +1,149 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2016  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
+
+#define _GNU_SOURCE
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "util.h"
+#include "hashmap.h"
+#include "dbus.h"
+#include "dbus-private.h"
+
+struct _dbus_name_cache {
+	struct l_dbus *bus;
+	struct l_hashmap *names;
+	const struct _dbus_name_ops *driver;
+};
+
+struct name_cache_entry {
+	int ref_count;
+	char *unique_name;
+};
+
+struct _dbus_name_cache *_dbus_name_cache_new(struct l_dbus *bus,
+					const struct _dbus_name_ops *driver)
+{
+	struct _dbus_name_cache *cache;
+
+	cache = l_new(struct _dbus_name_cache, 1);
+
+	cache->bus = bus;
+	cache->driver = driver;
+
+	return cache;
+}
+
+static void name_cache_entry_destroy(void *data)
+{
+	struct name_cache_entry *entry = data;
+
+	l_free(entry->unique_name);
+
+	l_free(entry);
+}
+
+void _dbus_name_cache_free(struct _dbus_name_cache *cache)
+{
+	l_hashmap_destroy(cache->names, name_cache_entry_destroy);
+
+	l_free(cache);
+}
+
+bool _dbus_name_cache_add(struct _dbus_name_cache *cache, const char *name)
+{
+	struct name_cache_entry *entry;
+
+	if (!_dbus_valid_bus_name(name))
+		return false;
+
+	if (!cache->names)
+		cache->names = l_hashmap_string_new();
+
+	entry = l_hashmap_lookup(cache->names, name);
+
+	if (!entry) {
+		entry = l_new(struct name_cache_entry, 1);
+
+		l_hashmap_insert(cache->names, name, entry);
+
+		cache->driver->get_name_owner(cache->bus, name);
+	}
+
+	entry->ref_count++;
+
+	return true;
+}
+
+bool _dbus_name_cache_remove(struct _dbus_name_cache *cache, const char *name)
+{
+	struct name_cache_entry *entry;
+
+	entry = l_hashmap_lookup(cache->names, name);
+
+	if (!entry)
+		return false;
+
+	if (--entry->ref_count)
+		return true;
+
+	l_hashmap_remove(cache->names, name);
+
+	name_cache_entry_destroy(entry);
+
+	return true;
+}
+
+const char *_dbus_name_cache_lookup(struct _dbus_name_cache *cache,
+					const char *name)
+{
+	struct name_cache_entry *entry;
+
+	entry = l_hashmap_lookup(cache->names, name);
+
+	if (!entry)
+		return NULL;
+
+	return entry->unique_name;
+}
+
+void _dbus_name_cache_notify(struct _dbus_name_cache *cache,
+				const char *name, const char *owner)
+{
+	struct name_cache_entry *entry;
+
+	if (_dbus_parse_unique_name(name, NULL))
+		return;
+
+	entry = l_hashmap_lookup(cache->names, name);
+
+	if (!entry)
+		return;
+
+	l_free(entry->unique_name);
+
+	entry->unique_name = (owner && *owner != '\0') ? l_strdup(owner) : NULL;
+}
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 030ce0b..327f0e3 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -273,6 +273,24 @@ uint8_t _dbus_get_version(struct l_dbus *dbus);
 int _dbus_get_fd(struct l_dbus *dbus);
 struct _dbus_object_tree *_dbus_get_tree(struct l_dbus *dbus);
 
+struct _dbus_name_ops {
+	bool (*get_name_owner)(struct l_dbus *bus, const char *name);
+};
+
+struct _dbus_name_cache;
+
+struct _dbus_name_cache *_dbus_name_cache_new(struct l_dbus *bus,
+					const struct _dbus_name_ops *driver);
+void _dbus_name_cache_free(struct _dbus_name_cache *cache);
+
+bool _dbus_name_cache_add(struct _dbus_name_cache *cache, const char *name);
+bool _dbus_name_cache_remove(struct _dbus_name_cache *cache, const char *name);
+const char *_dbus_name_cache_lookup(struct _dbus_name_cache *cache,
+					const char *name);
+
+void _dbus_name_cache_notify(struct _dbus_name_cache *cache,
+				const char *name, const char *owner);
+
 struct _dbus_filter_condition {
 	enum l_dbus_match_type type;
 	const char *value;
diff --git a/ell/dbus.c b/ell/dbus.c
index 84b3fa1..529a26a 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -64,6 +64,7 @@ struct l_dbus_ops {
 				struct l_dbus_message *message);
 	struct l_dbus_message *(*recv_message)(struct l_dbus *bus);
 	void (*free)(struct l_dbus *bus);
+	struct _dbus_name_ops name_ops;
 	struct _dbus_filter_ops filter_ops;
 	uint32_t (*name_acquire)(struct l_dbus *dbus, const char *name,
 				bool allow_replacement, bool replace_existing,
@@ -93,6 +94,7 @@ struct l_dbus {
 	l_dbus_destroy_func_t debug_destroy;
 	void *debug_data;
 	struct _dbus_object_tree *tree;
+	struct _dbus_name_cache *name_cache;
 	struct _dbus_filter *filter;
 
 	const struct l_dbus_ops *driver;
@@ -571,6 +573,8 @@ static void dbus_init(struct l_dbus *dbus, int fd)
 
 	dbus->tree = _dbus_object_tree_new();
 
+	dbus->name_cache = _dbus_name_cache_new(dbus, &dbus->driver->name_ops);
+
 	dbus->filter = _dbus_filter_new(dbus, &dbus->driver->filter_ops);
 }
 
@@ -768,6 +772,8 @@ static void get_name_owner_reply_cb(struct l_dbus_message *reply,
 		return;
 
 	_dbus_filter_name_owner_notify(req->dbus->filter, name, owner);
+
+	_dbus_name_cache_notify(req->dbus->name_cache, name, owner);
 }
 
 static bool classic_get_name_owner(struct l_dbus *bus, const char *name)
@@ -879,6 +885,9 @@ static const struct l_dbus_ops classic_ops = {
 	.send_message = classic_send_message,
 	.recv_message = classic_recv_message,
 	.free = classic_free,
+	.name_ops = {
+		.get_name_owner = classic_get_name_owner,
+	},
 	.filter_ops = {
 		.add_match = classic_add_match,
 		.remove_match = classic_remove_match,
@@ -897,6 +906,8 @@ static void name_owner_changed_cb(struct l_dbus_message *message,
 		return;
 
 	_dbus_filter_name_owner_notify(dbus->filter, name, new);
+
+	_dbus_name_cache_notify(dbus->name_cache, name, new);
 }
 
 static struct l_dbus *setup_dbus1(int fd, const char *guid)
@@ -1102,9 +1113,14 @@ static void kdbus_name_owner_change_func(const char *name, uint64_t old_owner,
 			recv_data->dbus->debug_data,
 			"Read KDBUS Name Owner Change notification");
 
-	snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
+	if (new_owner)
+		snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
+	else
+		owner[0] = '\0';
 
 	_dbus_filter_name_owner_notify(recv_data->dbus->filter, name, owner);
+
+	_dbus_name_cache_notify(recv_data->dbus->name_cache, name, owner);
 }
 
 static struct l_dbus_message *kdbus_recv_message(struct l_dbus *dbus)
@@ -1166,16 +1182,16 @@ static bool kdbus_get_name_owner(struct l_dbus *dbus, const char *name)
 	char owner[32];
 
 	owner_id = _dbus_kernel_get_name_owner(fd, kdbus->kdbus_pool, name);
-	if (!owner_id) {
-		l_util_debug(dbus->debug_handler,
-				dbus->debug_data, "Error getting name owner");
-		return false;
-	}
 
-	snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
+	if (owner_id)
+		snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
+	else
+		owner[0] = '\0';
 
 	_dbus_filter_name_owner_notify(dbus->filter, name, owner);
 
+	_dbus_name_cache_notify(dbus->name_cache, name, owner);
+
 	return true;
 }
 
@@ -1210,6 +1226,9 @@ static const struct l_dbus_ops kdbus_ops = {
 	.free = kdbus_free,
 	.send_message = kdbus_send_message,
 	.recv_message = kdbus_recv_message,
+	.name_ops = {
+		.get_name_owner = kdbus_get_name_owner,
+	},
 	.filter_ops = {
 		.add_match = kdbus_add_match,
 		.remove_match = kdbus_remove_match,
@@ -1355,6 +1374,8 @@ LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
 
 	_dbus_filter_free(dbus->filter);
 
+	_dbus_name_cache_free(dbus->name_cache);
+
 	l_hashmap_destroy(dbus->signal_list, signal_list_destroy);
 	l_hashmap_destroy(dbus->message_list, message_list_destroy);
 	l_queue_destroy(dbus->message_queue, message_queue_destroy);
-- 
2.5.0

Re: [PATCH 1/5] dbus: Add a Bus Name cache

[Denis Kenzior]

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

Hi Andrew,

On 04/18/2016 09:31 AM, Andrew Zaborowski wrote:
> ---
>   Makefile.am           |   1 +
>   ell/dbus-name-cache.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/dbus-private.h    |  18 ++++++
>   ell/dbus.c            |  35 +++++++++---
>   4 files changed, 196 insertions(+), 7 deletions(-)
>   create mode 100644 ell/dbus-name-cache.c
>

All 5 applied, thanks.

Regards,
-Denis

[PATCH 2/5] dbus: Use the name cache API in _dbus_filter

[Andrew Zaborowski]

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

---
 ell/dbus-filter.c  | 122 +++++++++--------------------------------------------
 ell/dbus-private.h |   6 +--
 ell/dbus.c         |  13 +-----
 3 files changed, 24 insertions(+), 117 deletions(-)

diff --git a/ell/dbus-filter.c b/ell/dbus-filter.c
index 59059cb..42755a9 100644
--- a/ell/dbus-filter.c
+++ b/ell/dbus-filter.c
@@ -62,12 +62,7 @@ struct _dbus_filter {
 	unsigned int signal_id;
 	unsigned int last_id;
 	const struct _dbus_filter_ops *driver;
-	struct l_hashmap *unique_names;
-};
-
-struct unique_name_record {
-	int ref_count;
-	char *unique_name;
+	struct _dbus_name_cache *name_cache;
 };
 
 static void filter_subtree_free(struct filter_node *node)
@@ -92,14 +87,6 @@ static void filter_subtree_free(struct filter_node *node)
 	}
 }
 
-static void unique_name_record_free(void *data)
-{
-	struct unique_name_record *name_rec = data;
-
-	l_free(name_rec->unique_name);
-	l_free(name_rec);
-}
-
 static void dbus_filter_destroy(void *data)
 {
 	struct _dbus_filter *filter = data;
@@ -107,10 +94,6 @@ static void dbus_filter_destroy(void *data)
 	if (filter->root)
 		filter_subtree_free(filter->root);
 
-	if (filter->unique_names)
-		l_hashmap_destroy(filter->unique_names,
-					unique_name_record_free);
-
 	l_free(filter);
 }
 
@@ -120,7 +103,6 @@ static void filter_dispatch_match_recurse(struct _dbus_filter *filter,
 {
 	const char *value = NULL;
 	const char *alt_value = NULL;
-	const struct unique_name_record *name_rec;
 	struct filter_node *child;
 
 	switch ((int) node->type) {
@@ -157,13 +139,9 @@ static void filter_dispatch_match_recurse(struct _dbus_filter *filter,
 	if (!value)
 		return;
 
-	if (node->type == L_DBUS_MATCH_SENDER && filter->unique_names) {
-		name_rec = l_hashmap_lookup(filter->unique_names,
-						node->match.value);
-
-		if (name_rec)
-			alt_value = name_rec->unique_name;
-	}
+	if (node->type == L_DBUS_MATCH_SENDER && filter->name_cache)
+		alt_value = _dbus_name_cache_lookup(filter->name_cache,
+							node->match.value);
 
 	if (strcmp(value, node->match.value) &&
 			(!alt_value || strcmp(value, alt_value)))
@@ -180,28 +158,9 @@ void _dbus_filter_dispatch(struct l_dbus_message *message, void *user_data)
 	filter_dispatch_match_recurse(filter, filter->root, message);
 }
 
-void _dbus_filter_name_owner_notify(struct _dbus_filter *filter,
-					const char *name, const char *owner)
-{
-	struct unique_name_record *name_rec;
-
-	if (!filter)
-		return;
-
-	if (_dbus_parse_unique_name(name, NULL))
-		return;
-
-	name_rec = l_hashmap_lookup(filter->unique_names, name);
-	if (!name_rec)
-		return;
-
-	l_free(name_rec->unique_name);
-
-	name_rec->unique_name = (owner && *owner) ? l_strdup(owner) : NULL;
-}
-
 struct _dbus_filter *_dbus_filter_new(struct l_dbus *dbus,
-					const struct _dbus_filter_ops *driver)
+					const struct _dbus_filter_ops *driver,
+					struct _dbus_name_cache *name_cache)
 {
 	struct _dbus_filter *filter;
 
@@ -209,15 +168,13 @@ struct _dbus_filter *_dbus_filter_new(struct l_dbus *dbus,
 
 	filter->dbus = dbus;
 	filter->driver = driver;
+	filter->name_cache = name_cache;
 
 	if (!filter->driver->skip_register)
 		filter->signal_id = l_dbus_register(dbus, _dbus_filter_dispatch,
 							filter,
 							dbus_filter_destroy);
 
-	if (filter->driver->get_name_owner)
-		filter->unique_names = l_hashmap_string_new();
-
 	return filter;
 }
 
@@ -232,54 +189,6 @@ void _dbus_filter_free(struct _dbus_filter *filter)
 		dbus_filter_destroy(filter);
 }
 
-static bool filter_add_bus_name(struct _dbus_filter *filter, const char *name)
-{
-	struct unique_name_record *name_rec;
-
-	if (!filter->unique_names)
-		return true;
-
-	if (_dbus_parse_unique_name(name, NULL))
-		return true;
-
-	if (!_dbus_valid_bus_name(name))
-		return false;
-
-	name_rec = l_hashmap_lookup(filter->unique_names, name);
-	if (!name_rec) {
-		name_rec = l_new(struct unique_name_record, 1);
-
-		l_hashmap_insert(filter->unique_names, name, name_rec);
-
-		filter->driver->get_name_owner(filter->dbus, name);
-	}
-
-	name_rec->ref_count++;
-
-	return true;
-}
-
-static void filter_remove_bus_name(struct _dbus_filter *filter,
-					const char *name)
-{
-	struct unique_name_record *name_rec;
-
-	if (!filter->unique_names)
-		return;
-
-	if (_dbus_parse_unique_name(name, NULL))
-		return;
-
-	name_rec = l_hashmap_lookup(filter->unique_names, name);
-
-	if (--name_rec->ref_count)
-		return;
-
-	l_hashmap_remove(filter->unique_names, name);
-
-	unique_name_record_free(name_rec);
-}
-
 static int condition_compare(const void *a, const void *b)
 {
 	const struct _dbus_filter_condition *condition_a = a, *condition_b = b;
@@ -312,8 +221,11 @@ static bool remove_recurse(struct _dbus_filter *filter,
 		if (tmp->match.remote_rule)
 			filter->driver->remove_match(filter->dbus, tmp->id);
 
-		if (tmp->type == L_DBUS_MATCH_SENDER)
-			filter_remove_bus_name(filter, tmp->match.value);
+		if (tmp->type == L_DBUS_MATCH_SENDER && filter->name_cache &&
+				!_dbus_parse_unique_name(tmp->match.value,
+								NULL))
+			_dbus_name_cache_remove(filter->name_cache,
+						tmp->match.value);
 
 		filter_subtree_free(tmp);
 	}
@@ -356,8 +268,14 @@ unsigned int _dbus_filter_add_rule(struct _dbus_filter *filter,
 
 			*node_ptr = node;
 
-			if (node->type == L_DBUS_MATCH_SENDER)
-				filter_add_bus_name(filter, node->match.value);
+			if (node->type == L_DBUS_MATCH_SENDER &&
+					filter->name_cache &&
+					!_dbus_parse_unique_name(
+							node->match.value,
+							NULL))
+				_dbus_name_cache_add(filter->name_cache,
+							node->match.value);
+
 		}
 
 		node_ptr = &node->match.children;
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 327f0e3..1ade3e0 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -302,11 +302,11 @@ struct _dbus_filter_ops {
 				const struct _dbus_filter_condition *rule,
 				int rule_len);
 	bool (*remove_match)(struct l_dbus *bus, unsigned int id);
-	bool (*get_name_owner)(struct l_dbus *bus, const char *name);
 };
 
 struct _dbus_filter *_dbus_filter_new(struct l_dbus *dbus,
-					const struct _dbus_filter_ops *driver);
+					const struct _dbus_filter_ops *driver,
+					struct _dbus_name_cache *name_cache);
 void _dbus_filter_free(struct _dbus_filter *filter);
 
 unsigned int _dbus_filter_add_rule(struct _dbus_filter *filter,
@@ -320,8 +320,6 @@ char *_dbus_filter_rule_to_str(const struct _dbus_filter_condition *rule,
 				int rule_len);
 
 void _dbus_filter_dispatch(struct l_dbus_message *message, void *user_data);
-void _dbus_filter_name_owner_notify(struct _dbus_filter *filter,
-					const char *name, const char *owner);
 
 struct dbus1_filter_data;
 
diff --git a/ell/dbus.c b/ell/dbus.c
index 529a26a..bc6aaed 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -575,7 +575,8 @@ static void dbus_init(struct l_dbus *dbus, int fd)
 
 	dbus->name_cache = _dbus_name_cache_new(dbus, &dbus->driver->name_ops);
 
-	dbus->filter = _dbus_filter_new(dbus, &dbus->driver->filter_ops);
+	dbus->filter = _dbus_filter_new(dbus, &dbus->driver->filter_ops,
+					dbus->name_cache);
 }
 
 static void classic_free(struct l_dbus *dbus)
@@ -771,8 +772,6 @@ static void get_name_owner_reply_cb(struct l_dbus_message *reply,
 	if (!l_dbus_message_get_arguments(req->message, "s", &name))
 		return;
 
-	_dbus_filter_name_owner_notify(req->dbus->filter, name, owner);
-
 	_dbus_name_cache_notify(req->dbus->name_cache, name, owner);
 }
 
@@ -891,7 +890,6 @@ static const struct l_dbus_ops classic_ops = {
 	.filter_ops = {
 		.add_match = classic_add_match,
 		.remove_match = classic_remove_match,
-		.get_name_owner = classic_get_name_owner,
 	},
 	.name_acquire = classic_name_acquire,
 };
@@ -905,8 +903,6 @@ static void name_owner_changed_cb(struct l_dbus_message *message,
 	if (!l_dbus_message_get_arguments(message, "sss", &name, &old, &new))
 		return;
 
-	_dbus_filter_name_owner_notify(dbus->filter, name, new);
-
 	_dbus_name_cache_notify(dbus->name_cache, name, new);
 }
 
@@ -1118,8 +1114,6 @@ static void kdbus_name_owner_change_func(const char *name, uint64_t old_owner,
 	else
 		owner[0] = '\0';
 
-	_dbus_filter_name_owner_notify(recv_data->dbus->filter, name, owner);
-
 	_dbus_name_cache_notify(recv_data->dbus->name_cache, name, owner);
 }
 
@@ -1188,8 +1182,6 @@ static bool kdbus_get_name_owner(struct l_dbus *dbus, const char *name)
 	else
 		owner[0] = '\0';
 
-	_dbus_filter_name_owner_notify(dbus->filter, name, owner);
-
 	_dbus_name_cache_notify(dbus->name_cache, name, owner);
 
 	return true;
@@ -1232,7 +1224,6 @@ static const struct l_dbus_ops kdbus_ops = {
 	.filter_ops = {
 		.add_match = kdbus_add_match,
 		.remove_match = kdbus_remove_match,
-		.get_name_owner = kdbus_get_name_owner,
 	},
 	.name_acquire = kdbus_name_acquire,
 };
-- 
2.5.0

[PATCH 3/5] unit: Update _dbus_filter usage in test-dbus-watch

[Andrew Zaborowski]

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

Also fix what seems to be incorrect use of l_dbus_message_set_arguments
---
 unit/test-dbus-watch.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/unit/test-dbus-watch.c b/unit/test-dbus-watch.c
index 684ac34..1d8f94c 100644
--- a/unit/test-dbus-watch.c
+++ b/unit/test-dbus-watch.c
@@ -387,7 +387,7 @@ static void test_filter_tree(const void *test_data)
 		{ L_DBUS_MATCH_PATH, DBUS_PATH_DBUS },
 		{ L_DBUS_MATCH_INTERFACE, DBUS_INTERFACE_DBUS },
 		{ L_DBUS_MATCH_MEMBER, "NameOwnerChanged" },
-		{ L_DBUS_MATCH_ARGUMENT(0), ":1.101" }
+		{ L_DBUS_MATCH_ARGUMENT(0), "org.test" }
 	};
 	static struct _dbus_filter_condition rule4[] = {
 		{ L_DBUS_MATCH_TYPE, "signal" },
@@ -396,7 +396,7 @@ static void test_filter_tree(const void *test_data)
 	unsigned int id1, id2, id3, id4, internal_id1, internal_id4;
 	struct l_dbus_message *message;
 
-	filter = _dbus_filter_new(&test.dbus, &filter_ops);
+	filter = _dbus_filter_new(&test.dbus, &filter_ops, NULL);
 	assert(filter);
 
 	test.expected_rule = rule123;
@@ -423,7 +423,8 @@ static void test_filter_tree(const void *test_data)
 	message = _dbus_message_new_signal(2, DBUS_PATH_DBUS,
 						DBUS_INTERFACE_DBUS,
 						"NameOwnerChanged");
-	l_dbus_message_set_arguments(message, "sss", ":1.101", "", ":1.101");
+	l_dbus_message_set_arguments(message, "sss", "org.test",
+					":1.101", "", ":1.101");
 	_dbus_message_set_sender(message, DBUS_SERVICE_DBUS);
 	_dbus_filter_dispatch(message, filter);
 	l_dbus_message_unref(message);
-- 
2.5.0

[PATCH 4/5] dbus: Add service watch API based on the name cache

[Andrew Zaborowski]

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

---
 ell/dbus-name-cache.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++-
 ell/dbus-private.h    |   9 +++
 2 files changed, 157 insertions(+), 1 deletion(-)

diff --git a/ell/dbus-name-cache.c b/ell/dbus-name-cache.c
index 76b1792..f7d5e50 100644
--- a/ell/dbus-name-cache.c
+++ b/ell/dbus-name-cache.c
@@ -30,6 +30,7 @@
 
 #include "util.h"
 #include "hashmap.h"
+#include "idle.h"
 #include "dbus.h"
 #include "dbus-private.h"
 
@@ -37,11 +38,24 @@ struct _dbus_name_cache {
 	struct l_dbus *bus;
 	struct l_hashmap *names;
 	const struct _dbus_name_ops *driver;
+	unsigned int last_watch_id;
+	struct l_idle *watch_remove_work;
+};
+
+struct service_watch {
+	l_dbus_watch_func_t connect_func;
+	l_dbus_watch_func_t disconnect_func;
+	l_dbus_destroy_func_t destroy;
+	void *user_data;
+	unsigned int id;
+	bool removed;
+	struct service_watch *next;
 };
 
 struct name_cache_entry {
 	int ref_count;
 	char *unique_name;
+	struct service_watch *watches;
 };
 
 struct _dbus_name_cache *_dbus_name_cache_new(struct l_dbus *bus,
@@ -57,9 +71,27 @@ struct _dbus_name_cache *_dbus_name_cache_new(struct l_dbus *bus,
 	return cache;
 }
 
+static void service_watch_destroy(void *data)
+{
+	struct service_watch *watch = data;
+
+	if (watch->destroy)
+		watch->destroy(watch->user_data);
+
+	l_free(watch);
+}
+
 static void name_cache_entry_destroy(void *data)
 {
 	struct name_cache_entry *entry = data;
+	struct service_watch *watch;
+
+	while (entry->watches) {
+		watch = entry->watches;
+		entry->watches = watch->next;
+
+		service_watch_destroy(watch);
+	}
 
 	l_free(entry->unique_name);
 
@@ -68,6 +100,9 @@ static void name_cache_entry_destroy(void *data)
 
 void _dbus_name_cache_free(struct _dbus_name_cache *cache)
 {
+	if (cache->watch_remove_work)
+		l_idle_remove(cache->watch_remove_work);
+
 	l_hashmap_destroy(cache->names, name_cache_entry_destroy);
 
 	l_free(cache);
@@ -134,6 +169,8 @@ void _dbus_name_cache_notify(struct _dbus_name_cache *cache,
 				const char *name, const char *owner)
 {
 	struct name_cache_entry *entry;
+	struct service_watch *watch;
+	bool prev_connected, connected;
 
 	if (_dbus_parse_unique_name(name, NULL))
 		return;
@@ -143,7 +180,117 @@ void _dbus_name_cache_notify(struct _dbus_name_cache *cache,
 	if (!entry)
 		return;
 
+	prev_connected = !!entry->unique_name;
+	connected = owner && *owner != '\0';
+
 	l_free(entry->unique_name);
 
-	entry->unique_name = (owner && *owner != '\0') ? l_strdup(owner) : NULL;
+	entry->unique_name = connected ? l_strdup(owner) : NULL;
+
+	/*
+	 * This check also means we notify all watchers who have a connected
+	 * callback when we first learn that the service is in fact connected.
+	 */
+	if (connected == prev_connected)
+		return;
+
+	for (watch = entry->watches; watch; watch = watch->next)
+		if (connected && watch->connect_func)
+			watch->connect_func(cache->bus, watch->user_data);
+		else if (!connected && watch->disconnect_func)
+			watch->disconnect_func(cache->bus, watch->user_data);
+}
+
+unsigned int _dbus_name_cache_add_watch(struct _dbus_name_cache *cache,
+					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 name_cache_entry *entry;
+	struct service_watch *watch;
+
+	if (!_dbus_name_cache_add(cache, name))
+		return 0;
+
+	watch = l_new(struct service_watch, 1);
+	watch->id = ++cache->last_watch_id;
+	watch->connect_func = connect_func;
+	watch->disconnect_func = disconnect_func;
+	watch->user_data = user_data;
+	watch->destroy = destroy;
+
+	entry = l_hashmap_lookup(cache->names, name);
+
+	watch->next = entry->watches;
+	entry->watches = watch;
+
+ 	if (entry->unique_name && connect_func)
+		watch->connect_func(cache->bus, watch->user_data);
+
+	return watch->id;
+}
+
+static void service_watch_remove(const void *key, void *value, void *user_data)
+{
+	struct _dbus_name_cache *cache = user_data;
+	struct name_cache_entry *entry = value;
+	struct service_watch **watch, *tmp;
+
+	for (watch = &entry->watches; *watch;) {
+		if (!(*watch)->removed) {
+			watch = &(*watch)->next;
+			continue;
+		}
+
+		tmp = *watch;
+		*watch = tmp->next;
+
+		service_watch_destroy(tmp);
+
+		_dbus_name_cache_remove(cache, key);
+	}
+}
+
+static void service_watch_remove_all(struct l_idle *idle, void *user_data)
+{
+	struct _dbus_name_cache *cache = user_data;
+
+	l_idle_remove(cache->watch_remove_work);
+	cache->watch_remove_work = NULL;
+
+	l_hashmap_foreach(cache->names, service_watch_remove, cache);
+}
+
+static void service_watch_mark(const void *key, void *value, void *user_data)
+{
+	struct name_cache_entry *entry = value;
+	struct service_watch *watch;
+	unsigned int *id = user_data;
+
+	for (watch = entry->watches; watch; watch = watch->next)
+		if (watch->id == *id) {
+			watch->removed = true;
+			watch->connect_func = NULL;
+			watch->disconnect_func = NULL;
+			*id = 0;
+			break;
+		}
+}
+
+bool _dbus_name_cache_remove_watch(struct _dbus_name_cache *cache,
+					unsigned int id)
+{
+	l_hashmap_foreach(cache->names, service_watch_mark, &id);
+
+	if (id)
+		return false;
+
+	if (!cache->watch_remove_work)
+		cache->watch_remove_work = l_idle_create(
+						service_watch_remove_all,
+						cache, NULL);
+
+	return true;
 }
diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 1ade3e0..48541e0 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -291,6 +291,15 @@ const char *_dbus_name_cache_lookup(struct _dbus_name_cache *cache,
 void _dbus_name_cache_notify(struct _dbus_name_cache *cache,
 				const char *name, const char *owner);
 
+unsigned int _dbus_name_cache_add_watch(struct _dbus_name_cache *cache,
+					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);
+bool _dbus_name_cache_remove_watch(struct _dbus_name_cache *cache,
+					unsigned int id);
+
 struct _dbus_filter_condition {
 	enum l_dbus_match_type type;
 	const char *value;
-- 
2.5.0

[PATCH 5/5] dbus: Use the name cache for public service watches

[Andrew Zaborowski]

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

This also removes the now unused dbus.c functions to fix compilation but
doesn't remove the rest of the logic which is now only used by the unit
test I think and can be removed separately.
---
 ell/dbus.c | 120 +++----------------------------------------------------------
 1 file changed, 5 insertions(+), 115 deletions(-)

diff --git a/ell/dbus.c b/ell/dbus.c
index bc6aaed..af0000d 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -1822,57 +1822,6 @@ void _dbus1_filter_data_destroy(void *user_data)
 	l_free(data);
 }
 
-static void dbus1_send_match(struct l_dbus *dbus, const char *rule,
-						const char *method)
-{
-	struct l_dbus_message *message;
-
-	message = l_dbus_message_new_method_call(dbus,
-						DBUS_SERVICE_DBUS,
-						DBUS_PATH_DBUS,
-						L_DBUS_INTERFACE_DBUS,
-						method);
-
-	l_dbus_message_set_arguments(message, "s", rule);
-
-	send_message(dbus, false, message, NULL, NULL, NULL);
-}
-
-static void dbus1_bus_add_match(struct l_dbus *dbus, const char *rule)
-{
-	dbus1_send_match(dbus, rule, "AddMatch");
-}
-
-static void dbus1_bus_remove_match(struct l_dbus *dbus, const char *rule)
-{
-	dbus1_send_match(dbus, rule, "RemoveMatch");
-}
-
-static void add_match(struct dbus1_filter_data *data)
-{
-	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
-
-	_dbus1_filter_format_match(data, rule, sizeof(rule));
-
-	dbus1_bus_add_match(data->dbus, rule);
-}
-
-static void remove_match(struct dbus1_filter_data *data)
-{
-	char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH];
-
-	_dbus1_filter_format_match(data, rule, sizeof(rule));
-
-	dbus1_bus_remove_match(data->dbus, rule);
-}
-
-static void filter_data_destroy(void *user_data)
-{
-	remove_match(user_data);
-
-	_dbus1_filter_data_destroy(user_data);
-}
-
 void _dbus1_signal_dispatcher(struct l_dbus_message *message, void *user_data)
 {
 	struct dbus1_filter_data *data = user_data;
@@ -1939,33 +1888,6 @@ void _dbus1_name_owner_changed_filter(struct l_dbus_message *message,
 	}
 }
 
-static void _dbus1_get_name_owner_reply(struct l_dbus_message *message,
-							void *user_data)
-{
-	struct dbus1_filter_data *data = user_data;
-	const char *name;
-
-	data->get_name_owner_id = 0;
-
-	/* No name owner yet */
-	if (l_dbus_message_is_error(message))
-		return;
-
-	/* Shouldn't happen */
-	if (!l_dbus_message_get_arguments(message, "s", &name))
-		return;
-
-	/* If the signal arrived before the reply, don't do anything */
-	if (data->owner && !strcmp(data->owner, name))
-		return;
-
-	l_free(data->owner);
-	data->owner = l_strdup(name);
-
-	if (data->connect_func)
-		data->connect_func(data->dbus, data->user_data);
-}
-
 LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
 					const char *name,
 					l_dbus_watch_func_t disconnect_func,
@@ -1976,56 +1898,24 @@ LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
 					user_data, destroy);
 }
 
-unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
+LIB_EXPORT unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
 					const char *name,
 					l_dbus_watch_func_t connect_func,
 					l_dbus_watch_func_t disconnect_func,
 					void *user_data,
 					l_dbus_destroy_func_t destroy)
 {
-	struct dbus1_filter_data *data;
-
 	if (!name)
 		return 0;
 
-	data = _dbus1_filter_data_get(dbus, _dbus1_name_owner_changed_filter,
-				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
-				L_DBUS_INTERFACE_DBUS, "NameOwnerChanged",
-				name,
-				disconnect_func,
-				user_data,
-				destroy);
-	if (!data)
-		return 0;
-
-	add_match(data);
-
-	if (connect_func) {
-		struct l_dbus_message *message;
-
-		data->connect_func = connect_func;
-
-		message = l_dbus_message_new_method_call(dbus,
-							DBUS_SERVICE_DBUS,
-							DBUS_PATH_DBUS,
-							L_DBUS_INTERFACE_DBUS,
-							"GetNameOwner");
-
-		l_dbus_message_set_arguments(message, "s", name);
-
-		data->get_name_owner_id =
-			l_dbus_send_with_reply(dbus, message,
-						_dbus1_get_name_owner_reply,
-						data, NULL);
-	}
-
-	return l_dbus_register(dbus, _dbus1_signal_dispatcher, data,
-							filter_data_destroy);
+	return _dbus_name_cache_add_watch(dbus->name_cache, name, connect_func,
+						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);
+	return _dbus_name_cache_remove_watch(dbus->name_cache, id);
 }
 
 /**
-- 
2.5.0

Re: [PATCH 1/2] dbus: Add private _dbus_message_new_error

[Denis Kenzior]

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

Hi Andrew,

On 04/18/2016 09:31 AM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-message.c | 47 +++++++++++++++++++++++++++++++----------------
>   ell/dbus-private.h |  5 +++++
>   2 files changed, 36 insertions(+), 16 deletions(-)
>
> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
> index a409e53..86cec96 100644
> --- a/ell/dbus-message.c
> +++ b/ell/dbus-message.c
> @@ -328,33 +328,32 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_new_method_return(
>   	return message;
>   }
>
> -LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
> -					struct l_dbus_message *method_call,
> -					const char *name,
> -					const char *format, va_list args)
> +struct l_dbus_message *_dbus_message_new_error(uint8_t version,
> +						uint32_t reply_serial,
> +						const char *destination,
> +						const char *name,
> +						const char *error)
>   {
> -	char str[1024];
>   	struct l_dbus_message *reply;
> -	struct dbus_header *hdr = method_call->header;
> -	const char *sender;
> +	bool r;
>
>   	if (!_dbus_valid_interface(name))
>   		return NULL;
>
> -	vsnprintf(str, sizeof(str), format, args);
>   	reply = message_new_common(DBUS_MESSAGE_TYPE_ERROR,
>   					DBUS_MESSAGE_FLAG_NO_REPLY_EXPECTED,
> -					hdr->version);
> -
> -	reply->reply_serial = _dbus_message_get_serial(method_call);
> -
> -	sender = l_dbus_message_get_sender(method_call);
> -	if (sender)
> -		reply->destination = l_strdup(sender);
> +					version);
>
>   	reply->error_name = l_strdup(name);
> +	reply->destination = l_strdup(destination);
> +	reply->reply_serial = reply_serial;
>
> -	if (!l_dbus_message_set_arguments(reply, "s", str)) {
> +	if (error)
> +		r = l_dbus_message_set_arguments(reply, "s", error);
> +	else
> +		r = l_dbus_message_set_arguments(reply, "");

Under what circumstances would we want to create an error without at 
least a single string argument?

> +
> +	if (!r) {
>   		l_dbus_message_unref(reply);
>   		return NULL;
>   	}
> @@ -362,6 +361,22 @@ LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
>   	return reply;
>   }
>
> +LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
> +					struct l_dbus_message *method_call,
> +					const char *name,
> +					const char *format, va_list args)
> +{
> +	char str[1024];
> +	struct dbus_header *hdr = method_call->header;
> +
> +	vsnprintf(str, sizeof(str), format, args);
> +
> +	return _dbus_message_new_error(hdr->version,
> +					_dbus_message_get_serial(method_call),
> +					l_dbus_message_get_sender(method_call),
> +					name, str);
> +}
> +
>   LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error(
>   					struct l_dbus_message *method_call,
>   					const char *name,
> diff --git a/ell/dbus-private.h b/ell/dbus-private.h
> index 63c239b..030ce0b 100644
> --- a/ell/dbus-private.h
> +++ b/ell/dbus-private.h
> @@ -141,6 +141,11 @@ struct l_dbus_message *_dbus_message_new_signal(uint8_t version,
>   						const char *path,
>   						const char *interface,
>   						const char *name);
> +struct l_dbus_message *_dbus_message_new_error(uint8_t version,
> +						uint32_t reply_serial,
> +						const char *destination,
> +						const char *name,
> +						const char *error);
>
>   struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size);
>   struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
>

Regards,
-Denis

Re: [PATCH 1/2] dbus: Add private _dbus_message_new_error

[Andrzej Zaborowski]

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

Hi Denis,

On 19 April 2016 at 04:37, Denis Kenzior <denkenz@gmail.com> wrote:
> On 04/18/2016 09:31 AM, Andrew Zaborowski wrote:
>>
>> ---
>>   ell/dbus-message.c | 47 +++++++++++++++++++++++++++++++----------------
>>   ell/dbus-private.h |  5 +++++
>>   2 files changed, 36 insertions(+), 16 deletions(-)
>>
>> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
>> index a409e53..86cec96 100644
>> --- a/ell/dbus-message.c
>> +++ b/ell/dbus-message.c
>> @@ -328,33 +328,32 @@ LIB_EXPORT struct l_dbus_message
>> *l_dbus_message_new_method_return(
>>         return message;
>>   }
>>
>> -LIB_EXPORT struct l_dbus_message *l_dbus_message_new_error_valist(
>> -                                       struct l_dbus_message
>> *method_call,
>> -                                       const char *name,
>> -                                       const char *format, va_list args)
>> +struct l_dbus_message *_dbus_message_new_error(uint8_t version,
>> +                                               uint32_t reply_serial,
>> +                                               const char *destination,
>> +                                               const char *name,
>> +                                               const char *error)
>>   {
>> -       char str[1024];
>>         struct l_dbus_message *reply;
>> -       struct dbus_header *hdr = method_call->header;
>> -       const char *sender;
>> +       bool r;
>>
>>         if (!_dbus_valid_interface(name))
>>                 return NULL;
>>
>> -       vsnprintf(str, sizeof(str), format, args);
>>         reply = message_new_common(DBUS_MESSAGE_TYPE_ERROR,
>>
>> DBUS_MESSAGE_FLAG_NO_REPLY_EXPECTED,
>> -                                       hdr->version);
>> -
>> -       reply->reply_serial = _dbus_message_get_serial(method_call);
>> -
>> -       sender = l_dbus_message_get_sender(method_call);
>> -       if (sender)
>> -               reply->destination = l_strdup(sender);
>> +                                       version);
>>
>>         reply->error_name = l_strdup(name);
>> +       reply->destination = l_strdup(destination);
>> +       reply->reply_serial = reply_serial;
>>
>> -       if (!l_dbus_message_set_arguments(reply, "s", str)) {
>> +       if (error)
>> +               r = l_dbus_message_set_arguments(reply, "s", error);
>> +       else
>> +               r = l_dbus_message_set_arguments(reply, "");
>
>
> Under what circumstances would we want to create an error without at least a
> single string argument?

I don't know but didn't want to exclude that possibility since it's
legal.  Let me remove the check.

Best regards