[PATCH] Fix signal watch when a service name is given

[PATCH] Fix signal watch when a service name is given

[Luiz Augusto von Dentz]

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

Hi,

This should fix g_dbus_add_signal_watch when a service like org.bluez is given.

-- 
Luiz Augusto von Dentz
Computer Engineer

[-- Attachment #2: 0002-Fix-signal-watch-when-a-service-name-is-given.patch --]
[-- Type: text/x-patch, Size: 9342 bytes --]

From dfceaaa8e068f4703dcad27db71bec9d649c7267 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Tue, 16 Feb 2010 16:55:35 +0200
Subject: [PATCH 2/2] Fix signal watch when a service name is given

The bus name should be resolved when adding a watch by service name since
messages do always come with sender set to owner's bus name, also it
should listen to owner updates since it can change without invalidating
the watch.
---
 gdbus/watch.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 117 insertions(+), 22 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 1d479fa..23c2b55 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -55,19 +55,22 @@ struct filter_callback {
 struct filter_data {
 	DBusConnection *connection;
 	DBusHandleMessageFunction handle_func;
-	char *sender;
+	char *name;
+	char *owner;
 	char *path;
 	char *interface;
 	char *member;
 	char *argument;
 	GSList *callbacks;
 	GSList *processed;
+	guint name_watch;
 	gboolean lock;
 	gboolean registered;
 };
 
 static struct filter_data *filter_data_find(DBusConnection *connection,
-							const char *sender,
+							const char *name,
+							const char *owner,
 							const char *path,
 							const char *interface,
 							const char *member,
@@ -82,8 +85,12 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 		if (connection != data->connection)
 			continue;
 
-		if (sender && data->sender &&
-				g_str_equal(sender, data->sender) == FALSE)
+		if (name && data->name &&
+				g_str_equal(name, data->name) == FALSE)
+			continue;
+
+		if (owner && data->owner &&
+				g_str_equal(owner, data->owner) == FALSE)
 			continue;
 
 		if (path && data->path &&
@@ -110,13 +117,15 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 
 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 (data->sender)
+	if (sender)
 		offset += snprintf(rule + offset, size - offset,
-				",sender='%s'", data->sender);
+				",sender='%s'", sender);
 	if (data->path)
 		offset += snprintf(rule + offset, size - offset,
 				",path='%s'", data->path);
@@ -183,8 +192,10 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 					const char *argument)
 {
 	struct filter_data *data;
+	const char *name = NULL, *owner = NULL;
 
-	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL)) {
+	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+				NULL)) {
 		if (!dbus_connection_add_filter(connection,
 					message_filter, NULL, NULL)) {
 			error("dbus_connection_add_filter() failed");
@@ -192,15 +203,25 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 		}
 	}
 
-	data = filter_data_find(connection, sender, path, interface, member,
-					argument);
+	if (sender == NULL)
+		goto proceed;
+
+	if (sender[0] == ':')
+		owner = sender;
+	else
+		name = sender;
+
+proceed:
+	data = filter_data_find(connection, name, owner, path, interface,
+					member, argument);
 	if (data)
 		return data;
 
 	data = g_new0(struct filter_data, 1);
 
 	data->connection = dbus_connection_ref(connection);
-	data->sender = g_strdup(sender);
+	data->name = name ? g_strdup(name) : NULL;
+	data->owner = owner ? g_strdup(owner) : NULL;
 	data->path = g_strdup(path);
 	data->interface = g_strdup(interface);
 	data->member = g_strdup(member);
@@ -244,7 +265,9 @@ static void filter_data_free(struct filter_data *data)
 		g_free(l->data);
 
 	g_slist_free(data->callbacks);
-	g_free(data->sender);
+	g_dbus_remove_watch(data->connection, data->name_watch);
+	g_free(data->name);
+	g_free(data->owner);
 	g_free(data->path);
 	g_free(data->interface);
 	g_free(data->member);
@@ -322,7 +345,8 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	filter_data_free(data);
 
 	/* Remove filter if there are no listeners left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -359,6 +383,37 @@ static DBusHandlerResult signal_filter(DBusConnection *connection,
 	return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
+static void update_name_cache(const char *service, const char *name)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, service) != 0)
+			continue;
+
+		g_free(data->name);
+		data->owner = g_strdup(name);
+	}
+}
+
+static const char *check_name_cache(const char *service)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, service) != 0)
+			continue;
+
+		return data->name;
+	}
+
+	return NULL;
+}
+
 static DBusHandlerResult service_filter(DBusConnection *connection,
 					DBusMessage *message, void *user_data)
 {
@@ -375,6 +430,8 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 	}
 
+	update_name_cache(name, new);
+
 	while (data->callbacks) {
 		cb = data->callbacks->data;
 
@@ -421,7 +478,9 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	member = dbus_message_get_member(message);
 	dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &arg, DBUS_TYPE_INVALID);
 
-	data = filter_data_find(connection, sender, path, iface, member, arg);
+	/* Sender is always bus name */
+	data = filter_data_find(connection, NULL, sender, path, iface, member,
+					arg);
 	if (!data) {
 		error("Got %s.%s signal which has no listeners", iface, member);
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
@@ -446,7 +505,8 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	filter_data_free(data);
 
 	/* Remove filter if there no listener left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -456,16 +516,37 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 
 struct service_data {
 	DBusConnection *conn;
+	char *name;
+	const char *owner;
 	GDBusWatchFunction conn_func;
 	void *user_data;
 };
 
+static void service_data_free(struct service_data *data)
+{
+	dbus_connection_unref(data->conn);
+	g_free(data->name);
+	g_free(data);
+}
+
+static gboolean update_service(void *user_data)
+{
+	struct service_data *data = user_data;
+
+	update_name_cache(data->name, data->owner);
+	if (data->conn_func)
+		data->conn_func(data->conn, data->user_data);
+
+	service_data_free(data);
+
+	return FALSE;
+}
+
 static void service_reply(DBusPendingCall *call, void *user_data)
 {
 	struct service_data *data = user_data;
 	DBusMessage *reply;
 	DBusError error;
-	dbus_bool_t has_owner;
 
 	reply = dbus_pending_call_steal_reply(call);
 	if (reply == NULL)
@@ -474,22 +555,23 @@ static void service_reply(DBusPendingCall *call, void *user_data)
 	dbus_error_init(&error);
 
 	if (dbus_message_get_args(reply, &error,
-					DBUS_TYPE_BOOLEAN, &has_owner,
+					DBUS_TYPE_STRING, &data->owner,
 						DBUS_TYPE_INVALID) == FALSE) {
 		if (dbus_error_is_set(&error) == TRUE) {
 			error("%s", error.message);
 			dbus_error_free(&error);
 		} else {
-			error("Wrong arguments for NameHasOwner reply");
+			error("Wrong arguments for GetNameOwner reply");
 		}
 		goto done;
 	}
 
-	if (has_owner && data->conn_func)
-		data->conn_func(data->conn, data->user_data);
+	update_service(data);
 
+	return;
 done:
 	dbus_message_unref(reply);
+	service_data_free(data);
 }
 
 static void check_service(DBusConnection *connection, const char *name,
@@ -505,12 +587,19 @@ static void check_service(DBusConnection *connection, const char *name,
 		return;
 	}
 
-	data->conn = connection;
+	data->conn = dbus_connection_ref(connection);
+	data->name = g_strdup(name);
 	data->conn_func = connect;
 	data->user_data = user_data;
 
+	data->owner = check_name_cache(name);
+	if (data->owner != NULL) {
+		g_idle_add(update_service, data);
+		return;
+	}
+
 	message = dbus_message_new_method_call(DBUS_SERVICE_DBUS,
-			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "NameHasOwner");
+			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "GetNameOwner");
 	if (message == NULL) {
 		error("Can't allocate new message");
 		g_free(data);
@@ -596,6 +685,11 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
 	if (!cb)
 		return 0;
 
+	if (data->name != NULL && data->name_watch == 0)
+		data->name_watch = g_dbus_add_service_watch(connection,
+							data->name, NULL,
+							NULL, NULL, NULL);
+
 	return cb->id;
 }
 
@@ -625,7 +719,8 @@ void g_dbus_remove_all_watches(DBusConnection *connection)
 {
 	struct filter_data *data;
 
-	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL))) {
+	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL,
+					NULL, NULL))) {
 		listeners = g_slist_remove(listeners, data);
 		filter_data_call_and_free(data);
 	}
-- 
1.6.3.3

Re: [PATCH] Fix signal watch when a service name is given

[Luiz Augusto von Dentz]

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

Hi,

On Tue, Feb 16, 2010 at 4:59 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> This should fix g_dbus_add_signal_watch when a service like org.bluez is given.


Updating since the last one was broken.


-- 
Luiz Augusto von Dentz
Computer Engineer

[-- Attachment #2: 0002-Fix-signal-watch-when-a-service-name-is-given.patch --]
[-- Type: text/x-patch, Size: 9334 bytes --]

From 00d9fe7a749130d51f7d70cb0469005ef7445441 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Tue, 16 Feb 2010 17:11:27 +0200
Subject: [PATCH 2/2] Fix signal watch when a service name is given

The bus name should be resolved when adding a watch by service name since
messages do always come with sender set to owner's bus name, also it
should listen to owner updates since it can change without invalidating
the watch.
---
 gdbus/watch.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 117 insertions(+), 22 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 1d479fa..864b4a3 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -55,19 +55,22 @@ struct filter_callback {
 struct filter_data {
 	DBusConnection *connection;
 	DBusHandleMessageFunction handle_func;
-	char *sender;
+	char *name;
+	char *owner;
 	char *path;
 	char *interface;
 	char *member;
 	char *argument;
 	GSList *callbacks;
 	GSList *processed;
+	guint name_watch;
 	gboolean lock;
 	gboolean registered;
 };
 
 static struct filter_data *filter_data_find(DBusConnection *connection,
-							const char *sender,
+							const char *name,
+							const char *owner,
 							const char *path,
 							const char *interface,
 							const char *member,
@@ -82,8 +85,12 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 		if (connection != data->connection)
 			continue;
 
-		if (sender && data->sender &&
-				g_str_equal(sender, data->sender) == FALSE)
+		if (name && data->name &&
+				g_str_equal(name, data->name) == FALSE)
+			continue;
+
+		if (owner && data->owner &&
+				g_str_equal(owner, data->owner) == FALSE)
 			continue;
 
 		if (path && data->path &&
@@ -110,13 +117,15 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 
 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 (data->sender)
+	if (sender)
 		offset += snprintf(rule + offset, size - offset,
-				",sender='%s'", data->sender);
+				",sender='%s'", sender);
 	if (data->path)
 		offset += snprintf(rule + offset, size - offset,
 				",path='%s'", data->path);
@@ -183,8 +192,10 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 					const char *argument)
 {
 	struct filter_data *data;
+	const char *name = NULL, *owner = NULL;
 
-	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL)) {
+	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+				NULL)) {
 		if (!dbus_connection_add_filter(connection,
 					message_filter, NULL, NULL)) {
 			error("dbus_connection_add_filter() failed");
@@ -192,15 +203,25 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 		}
 	}
 
-	data = filter_data_find(connection, sender, path, interface, member,
-					argument);
+	if (sender == NULL)
+		goto proceed;
+
+	if (sender[0] == ':')
+		owner = sender;
+	else
+		name = sender;
+
+proceed:
+	data = filter_data_find(connection, name, owner, path, interface,
+					member, argument);
 	if (data)
 		return data;
 
 	data = g_new0(struct filter_data, 1);
 
 	data->connection = dbus_connection_ref(connection);
-	data->sender = g_strdup(sender);
+	data->name = name ? g_strdup(name) : NULL;
+	data->owner = owner ? g_strdup(owner) : NULL;
 	data->path = g_strdup(path);
 	data->interface = g_strdup(interface);
 	data->member = g_strdup(member);
@@ -244,7 +265,9 @@ static void filter_data_free(struct filter_data *data)
 		g_free(l->data);
 
 	g_slist_free(data->callbacks);
-	g_free(data->sender);
+	g_dbus_remove_watch(data->connection, data->name_watch);
+	g_free(data->name);
+	g_free(data->owner);
 	g_free(data->path);
 	g_free(data->interface);
 	g_free(data->member);
@@ -322,7 +345,8 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	filter_data_free(data);
 
 	/* Remove filter if there are no listeners left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -359,6 +383,37 @@ static DBusHandlerResult signal_filter(DBusConnection *connection,
 	return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
+static void update_name_cache(const char *name, const char *owner)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, name) != 0)
+			continue;
+
+		g_free(data->owner);
+		data->owner = g_strdup(owner);
+	}
+}
+
+static const char *check_name_cache(const char *name)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, name) != 0)
+			continue;
+
+		return data->owner;
+	}
+
+	return NULL;
+}
+
 static DBusHandlerResult service_filter(DBusConnection *connection,
 					DBusMessage *message, void *user_data)
 {
@@ -375,6 +430,8 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 	}
 
+	update_name_cache(name, new);
+
 	while (data->callbacks) {
 		cb = data->callbacks->data;
 
@@ -421,7 +478,9 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	member = dbus_message_get_member(message);
 	dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &arg, DBUS_TYPE_INVALID);
 
-	data = filter_data_find(connection, sender, path, iface, member, arg);
+	/* Sender is always bus name */
+	data = filter_data_find(connection, NULL, sender, path, iface, member,
+					arg);
 	if (!data) {
 		error("Got %s.%s signal which has no listeners", iface, member);
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
@@ -446,7 +505,8 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	filter_data_free(data);
 
 	/* Remove filter if there no listener left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -456,16 +516,37 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 
 struct service_data {
 	DBusConnection *conn;
+	char *name;
+	const char *owner;
 	GDBusWatchFunction conn_func;
 	void *user_data;
 };
 
+static void service_data_free(struct service_data *data)
+{
+	dbus_connection_unref(data->conn);
+	g_free(data->name);
+	g_free(data);
+}
+
+static gboolean update_service(void *user_data)
+{
+	struct service_data *data = user_data;
+
+	update_name_cache(data->name, data->owner);
+	if (data->conn_func)
+		data->conn_func(data->conn, data->user_data);
+
+	service_data_free(data);
+
+	return FALSE;
+}
+
 static void service_reply(DBusPendingCall *call, void *user_data)
 {
 	struct service_data *data = user_data;
 	DBusMessage *reply;
 	DBusError error;
-	dbus_bool_t has_owner;
 
 	reply = dbus_pending_call_steal_reply(call);
 	if (reply == NULL)
@@ -474,22 +555,23 @@ static void service_reply(DBusPendingCall *call, void *user_data)
 	dbus_error_init(&error);
 
 	if (dbus_message_get_args(reply, &error,
-					DBUS_TYPE_BOOLEAN, &has_owner,
+					DBUS_TYPE_STRING, &data->owner,
 						DBUS_TYPE_INVALID) == FALSE) {
 		if (dbus_error_is_set(&error) == TRUE) {
 			error("%s", error.message);
 			dbus_error_free(&error);
 		} else {
-			error("Wrong arguments for NameHasOwner reply");
+			error("Wrong arguments for GetNameOwner reply");
 		}
 		goto done;
 	}
 
-	if (has_owner && data->conn_func)
-		data->conn_func(data->conn, data->user_data);
+	update_service(data);
 
+	return;
 done:
 	dbus_message_unref(reply);
+	service_data_free(data);
 }
 
 static void check_service(DBusConnection *connection, const char *name,
@@ -505,12 +587,19 @@ static void check_service(DBusConnection *connection, const char *name,
 		return;
 	}
 
-	data->conn = connection;
+	data->conn = dbus_connection_ref(connection);
+	data->name = g_strdup(name);
 	data->conn_func = connect;
 	data->user_data = user_data;
 
+	data->owner = check_name_cache(name);
+	if (data->owner != NULL) {
+		g_idle_add(update_service, data);
+		return;
+	}
+
 	message = dbus_message_new_method_call(DBUS_SERVICE_DBUS,
-			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "NameHasOwner");
+			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "GetNameOwner");
 	if (message == NULL) {
 		error("Can't allocate new message");
 		g_free(data);
@@ -596,6 +685,11 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
 	if (!cb)
 		return 0;
 
+	if (data->name != NULL && data->name_watch == 0)
+		data->name_watch = g_dbus_add_service_watch(connection,
+							data->name, NULL,
+							NULL, NULL, NULL);
+
 	return cb->id;
 }
 
@@ -625,7 +719,8 @@ void g_dbus_remove_all_watches(DBusConnection *connection)
 {
 	struct filter_data *data;
 
-	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL))) {
+	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL,
+					NULL, NULL))) {
 		listeners = g_slist_remove(listeners, data);
 		filter_data_call_and_free(data);
 	}
-- 
1.6.3.3

Re: [PATCH] Fix signal watch when a service name is given

[Marcel Holtmann]

Hi Luiz,

> > This should fix g_dbus_add_signal_watch when a service like org.bluez is given.
> 
> 
> Updating since the last one was broken.

I also think that in check_service() we actually have a missing call to
dbus_pending_unref(). That would cause a memory leak.

Regards

Marcel

Re: [PATCH] Fix signal watch when a service name is given

[Luiz Augusto von Dentz]

Hi Marcel,

On Tue, Feb 16, 2010 at 5:37 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> > This should fix g_dbus_add_signal_watch when a service like org.bluez is given.
>>
>>
>> Updating since the last one was broken.
>
> I also think that in check_service() we actually have a missing call to
> dbus_pending_unref(). That would cause a memory leak.

There is a call to  dbus_pending_call_unref on check_service after
dbus_pending_call_set_notify, which I took a look and seems correct
but we have another problem there since we never cancel the pending
call if the filter is unregister. Anyway there is a leak on
service_reply where I don't call dbus_message_unref, so I will come up
with another update soon.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix signal watch when a service name is given

[Luiz Augusto von Dentz]

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

Hi,

On Tue, Feb 16, 2010 at 5:58 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Marcel,
>
> On Tue, Feb 16, 2010 at 5:37 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Luiz,
>>
>>> > This should fix g_dbus_add_signal_watch when a service like org.bluez is given.
>>>
>>>
>>> Updating since the last one was broken.
>>
>> I also think that in check_service() we actually have a missing call to
>> dbus_pending_unref(). That would cause a memory leak.
>
> There is a call to  dbus_pending_call_unref on check_service after
> dbus_pending_call_set_notify, which I took a look and seems correct
> but we have another problem there since we never cancel the pending
> call if the filter is unregister. Anyway there is a leak on
> service_reply where I don't call dbus_message_unref, so I will come up
> with another update soon.

Now with the memory leaks fixed.

-- 
Luiz Augusto von Dentz
Computer Engineer

[-- Attachment #2: 0001-Fix-signal-watch-when-a-service-name-is-given.patch --]
[-- Type: text/x-patch, Size: 9243 bytes --]

From ae57b44fe1aa0203797d0838c27a91fef24fa83f Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Tue, 16 Feb 2010 18:20:22 +0200
Subject: [PATCH] Fix signal watch when a service name is given

The bus name should be resolved when adding a watch by service name since
messages do always come with sender set to owner's bus name, also it
should listen to owner updates since it can change without invalidating
the watch.
---
 gdbus/watch.c |  138 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 116 insertions(+), 22 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 1d479fa..fae53e6 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -55,19 +55,22 @@ struct filter_callback {
 struct filter_data {
 	DBusConnection *connection;
 	DBusHandleMessageFunction handle_func;
-	char *sender;
+	char *name;
+	char *owner;
 	char *path;
 	char *interface;
 	char *member;
 	char *argument;
 	GSList *callbacks;
 	GSList *processed;
+	guint name_watch;
 	gboolean lock;
 	gboolean registered;
 };
 
 static struct filter_data *filter_data_find(DBusConnection *connection,
-							const char *sender,
+							const char *name,
+							const char *owner,
 							const char *path,
 							const char *interface,
 							const char *member,
@@ -82,8 +85,12 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 		if (connection != data->connection)
 			continue;
 
-		if (sender && data->sender &&
-				g_str_equal(sender, data->sender) == FALSE)
+		if (name && data->name &&
+				g_str_equal(name, data->name) == FALSE)
+			continue;
+
+		if (owner && data->owner &&
+				g_str_equal(owner, data->owner) == FALSE)
 			continue;
 
 		if (path && data->path &&
@@ -110,13 +117,15 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 
 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 (data->sender)
+	if (sender)
 		offset += snprintf(rule + offset, size - offset,
-				",sender='%s'", data->sender);
+				",sender='%s'", sender);
 	if (data->path)
 		offset += snprintf(rule + offset, size - offset,
 				",path='%s'", data->path);
@@ -183,8 +192,10 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 					const char *argument)
 {
 	struct filter_data *data;
+	const char *name = NULL, *owner = NULL;
 
-	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL)) {
+	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+				NULL)) {
 		if (!dbus_connection_add_filter(connection,
 					message_filter, NULL, NULL)) {
 			error("dbus_connection_add_filter() failed");
@@ -192,15 +203,25 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 		}
 	}
 
-	data = filter_data_find(connection, sender, path, interface, member,
-					argument);
+	if (sender == NULL)
+		goto proceed;
+
+	if (sender[0] == ':')
+		owner = sender;
+	else
+		name = sender;
+
+proceed:
+	data = filter_data_find(connection, name, owner, path, interface,
+					member, argument);
 	if (data)
 		return data;
 
 	data = g_new0(struct filter_data, 1);
 
 	data->connection = dbus_connection_ref(connection);
-	data->sender = g_strdup(sender);
+	data->name = name ? g_strdup(name) : NULL;
+	data->owner = owner ? g_strdup(owner) : NULL;
 	data->path = g_strdup(path);
 	data->interface = g_strdup(interface);
 	data->member = g_strdup(member);
@@ -244,7 +265,9 @@ static void filter_data_free(struct filter_data *data)
 		g_free(l->data);
 
 	g_slist_free(data->callbacks);
-	g_free(data->sender);
+	g_dbus_remove_watch(data->connection, data->name_watch);
+	g_free(data->name);
+	g_free(data->owner);
 	g_free(data->path);
 	g_free(data->interface);
 	g_free(data->member);
@@ -322,7 +345,8 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	filter_data_free(data);
 
 	/* Remove filter if there are no listeners left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -359,6 +383,37 @@ static DBusHandlerResult signal_filter(DBusConnection *connection,
 	return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
+static void update_name_cache(const char *name, const char *owner)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, name) != 0)
+			continue;
+
+		g_free(data->owner);
+		data->owner = g_strdup(owner);
+	}
+}
+
+static const char *check_name_cache(const char *name)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, name) != 0)
+			continue;
+
+		return data->owner;
+	}
+
+	return NULL;
+}
+
 static DBusHandlerResult service_filter(DBusConnection *connection,
 					DBusMessage *message, void *user_data)
 {
@@ -375,6 +430,8 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 	}
 
+	update_name_cache(name, new);
+
 	while (data->callbacks) {
 		cb = data->callbacks->data;
 
@@ -421,7 +478,9 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	member = dbus_message_get_member(message);
 	dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &arg, DBUS_TYPE_INVALID);
 
-	data = filter_data_find(connection, sender, path, iface, member, arg);
+	/* Sender is always bus name */
+	data = filter_data_find(connection, NULL, sender, path, iface, member,
+					arg);
 	if (!data) {
 		error("Got %s.%s signal which has no listeners", iface, member);
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
@@ -446,7 +505,8 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	filter_data_free(data);
 
 	/* Remove filter if there no listener left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -456,16 +516,37 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 
 struct service_data {
 	DBusConnection *conn;
+	char *name;
+	const char *owner;
 	GDBusWatchFunction conn_func;
 	void *user_data;
 };
 
+static void service_data_free(struct service_data *data)
+{
+	dbus_connection_unref(data->conn);
+	g_free(data->name);
+	g_free(data);
+}
+
+static gboolean update_service(void *user_data)
+{
+	struct service_data *data = user_data;
+
+	update_name_cache(data->name, data->owner);
+	if (data->conn_func)
+		data->conn_func(data->conn, data->user_data);
+
+	service_data_free(data);
+
+	return FALSE;
+}
+
 static void service_reply(DBusPendingCall *call, void *user_data)
 {
 	struct service_data *data = user_data;
 	DBusMessage *reply;
 	DBusError error;
-	dbus_bool_t has_owner;
 
 	reply = dbus_pending_call_steal_reply(call);
 	if (reply == NULL)
@@ -474,19 +555,19 @@ static void service_reply(DBusPendingCall *call, void *user_data)
 	dbus_error_init(&error);
 
 	if (dbus_message_get_args(reply, &error,
-					DBUS_TYPE_BOOLEAN, &has_owner,
+					DBUS_TYPE_STRING, &data->owner,
 						DBUS_TYPE_INVALID) == FALSE) {
 		if (dbus_error_is_set(&error) == TRUE) {
 			error("%s", error.message);
 			dbus_error_free(&error);
 		} else {
-			error("Wrong arguments for NameHasOwner reply");
+			error("Wrong arguments for GetNameOwner reply");
 		}
+		service_data_free(data);
 		goto done;
 	}
 
-	if (has_owner && data->conn_func)
-		data->conn_func(data->conn, data->user_data);
+	update_service(data);
 
 done:
 	dbus_message_unref(reply);
@@ -505,12 +586,19 @@ static void check_service(DBusConnection *connection, const char *name,
 		return;
 	}
 
-	data->conn = connection;
+	data->conn = dbus_connection_ref(connection);
+	data->name = g_strdup(name);
 	data->conn_func = connect;
 	data->user_data = user_data;
 
+	data->owner = check_name_cache(name);
+	if (data->owner != NULL) {
+		g_idle_add(update_service, data);
+		return;
+	}
+
 	message = dbus_message_new_method_call(DBUS_SERVICE_DBUS,
-			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "NameHasOwner");
+			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "GetNameOwner");
 	if (message == NULL) {
 		error("Can't allocate new message");
 		g_free(data);
@@ -596,6 +684,11 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
 	if (!cb)
 		return 0;
 
+	if (data->name != NULL && data->name_watch == 0)
+		data->name_watch = g_dbus_add_service_watch(connection,
+							data->name, NULL,
+							NULL, NULL, NULL);
+
 	return cb->id;
 }
 
@@ -625,7 +718,8 @@ void g_dbus_remove_all_watches(DBusConnection *connection)
 {
 	struct filter_data *data;
 
-	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL))) {
+	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL,
+					NULL, NULL))) {
 		listeners = g_slist_remove(listeners, data);
 		filter_data_call_and_free(data);
 	}
-- 
1.6.3.3

Re: [PATCH] Fix signal watch when a service name is given

[Vinicius Gomes]

Hi Luiz,

On Tue, Feb 16, 2010 at 12:58 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Marcel,
>
> On Tue, Feb 16, 2010 at 5:37 PM, Marcel Holtmann <marcel@holtmann.org> wr=
ote:
>> Hi Luiz,
>>
>>> > This should fix g_dbus_add_signal_watch when a service like org.bluez=
 is given.
>>>
>>>
>>> Updating since the last one was broken.
>>
>> I also think that in check_service() we actually have a missing call to
>> dbus_pending_unref(). That would cause a memory leak.
>
> There is a call to =C2=A0dbus_pending_call_unref on check_service after
> dbus_pending_call_set_notify, which I took a look and seems correct
> but we have another problem there since we never cancel the pending
> call if the filter is unregister. Anyway there is a leak on
> service_reply where I don't call dbus_message_unref, so I will come up
> with another update soon.
>

This is fixed on obexd, but as far as I could see it is not (yet?)
applied on the other users
of gdbus.

And, from what I tested the greater problem is that the timeout of the
pending call would still
trigger. This would cause libdbus to terminate the application, if
libdbus was built with debug enabled,
because one assert would fail (the message is "trying to remove an
nonexistent timeout" or
 something like it). The timeout still triggering is the reason we
don't see the pending call leaking.

> --
> Luiz Augusto von Dentz
> Computer Engineer
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>


Cheers,
--=20
Vinicius Gomes
INdT - Instituto Nokia de Tecnologia

Re: [PATCH] Fix signal watch when a service name is given

[Luiz Augusto von Dentz]

Hi Vinicius,

On Wed, Feb 17, 2010 at 2:34 AM, Vinicius Gomes
<vinicius.gomes@openbossa.org> wrote:
> Hi Luiz,
>
> On Tue, Feb 16, 2010 at 12:58 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Marcel,
>>
>> On Tue, Feb 16, 2010 at 5:37 PM, Marcel Holtmann <marcel@holtmann.org> w=
rote:
>>> Hi Luiz,
>>>
>>>> > This should fix g_dbus_add_signal_watch when a service like org.blue=
z is given.
>>>>
>>>>
>>>> Updating since the last one was broken.
>>>
>>> I also think that in check_service() we actually have a missing call to
>>> dbus_pending_unref(). That would cause a memory leak.
>>
>> There is a call to =A0dbus_pending_call_unref on check_service after
>> dbus_pending_call_set_notify, which I took a look and seems correct
>> but we have another problem there since we never cancel the pending
>> call if the filter is unregister. Anyway there is a leak on
>> service_reply where I don't call dbus_message_unref, so I will come up
>> with another update soon.
>>
>
> This is fixed on obexd, but as far as I could see it is not (yet?)
> applied on the other users
> of gdbus.

Hmm, that why I see it, I used obexd to test this not bluetoothd as
(probably) Marcel did, anyway we still need to store and cancel the
pending call if we really want the watches to be truly cancelable.

> And, from what I tested the greater problem is that the timeout of the
> pending call would still
> trigger. This would cause libdbus to terminate the application, if
> libdbus was built with debug enabled,
> because one assert would fail (the message is "trying to remove an
> nonexistent timeout" or
> =A0something like it). The timeout still triggering is the reason we
> don't see the pending call leaking.

@Marcel: So before you apply this you should really apply Vinicius
patch to the rest of projects: bluez, ofono, connman...

--=20
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix signal watch when a service name is given

[Marcel Holtmann]

Hi Luiz,

> @Marcel: So before you apply this you should really apply Vinicius
> patch to the rest of projects: bluez, ofono, connman...

has been applied to all projects now. Please resend your other patch
based on the latest tree.

Regards

Marcel

Re: [PATCH] Fix signal watch when a service name is given

[Luiz Augusto von Dentz]

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

Hi Marcel,

On Wed, Feb 17, 2010 at 12:00 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> @Marcel: So before you apply this you should really apply Vinicius
>> patch to the rest of projects: bluez, ofono, connman...
>
> has been applied to all projects now. Please resend your other patch
> based on the latest tree.

Resending the patch and also add another one to make sure we cancel
any pending operation during watch removal.

-- 
Luiz Augusto von Dentz
Computer Engineer

[-- Attachment #2: 0001-Fix-signal-watch-when-a-service-name-is-given.patch --]
[-- Type: text/x-patch, Size: 9393 bytes --]

From b496f47dd7cdd3e3930d13579021908a8b3f857b Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Wed, 17 Feb 2010 17:11:02 +0200
Subject: [PATCH 1/2] Fix signal watch when a service name is given

The bus name should be resolved when adding a watch by service name since
messages do always come with sender set to owner's bus name, also it
should listen to owner updates since it can change without invalidating
the watch.
---
 gdbus/watch.c |  161 +++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 128 insertions(+), 33 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 1d479fa..ef02201 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -55,19 +55,22 @@ struct filter_callback {
 struct filter_data {
 	DBusConnection *connection;
 	DBusHandleMessageFunction handle_func;
-	char *sender;
+	char *name;
+	char *owner;
 	char *path;
 	char *interface;
 	char *member;
 	char *argument;
 	GSList *callbacks;
 	GSList *processed;
+	guint name_watch;
 	gboolean lock;
 	gboolean registered;
 };
 
 static struct filter_data *filter_data_find(DBusConnection *connection,
-							const char *sender,
+							const char *name,
+							const char *owner,
 							const char *path,
 							const char *interface,
 							const char *member,
@@ -82,8 +85,12 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 		if (connection != data->connection)
 			continue;
 
-		if (sender && data->sender &&
-				g_str_equal(sender, data->sender) == FALSE)
+		if (name && data->name &&
+				g_str_equal(name, data->name) == FALSE)
+			continue;
+
+		if (owner && data->owner &&
+				g_str_equal(owner, data->owner) == FALSE)
 			continue;
 
 		if (path && data->path &&
@@ -110,13 +117,15 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 
 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 (data->sender)
+	if (sender)
 		offset += snprintf(rule + offset, size - offset,
-				",sender='%s'", data->sender);
+				",sender='%s'", sender);
 	if (data->path)
 		offset += snprintf(rule + offset, size - offset,
 				",path='%s'", data->path);
@@ -183,8 +192,10 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 					const char *argument)
 {
 	struct filter_data *data;
+	const char *name = NULL, *owner = NULL;
 
-	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL)) {
+	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+				NULL)) {
 		if (!dbus_connection_add_filter(connection,
 					message_filter, NULL, NULL)) {
 			error("dbus_connection_add_filter() failed");
@@ -192,15 +203,25 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 		}
 	}
 
-	data = filter_data_find(connection, sender, path, interface, member,
-					argument);
+	if (sender == NULL)
+		goto proceed;
+
+	if (sender[0] == ':')
+		owner = sender;
+	else
+		name = sender;
+
+proceed:
+	data = filter_data_find(connection, name, owner, path, interface,
+					member, argument);
 	if (data)
 		return data;
 
 	data = g_new0(struct filter_data, 1);
 
 	data->connection = dbus_connection_ref(connection);
-	data->sender = g_strdup(sender);
+	data->name = name ? g_strdup(name) : NULL;
+	data->owner = owner ? g_strdup(owner) : NULL;
 	data->path = g_strdup(path);
 	data->interface = g_strdup(interface);
 	data->member = g_strdup(member);
@@ -244,7 +265,9 @@ static void filter_data_free(struct filter_data *data)
 		g_free(l->data);
 
 	g_slist_free(data->callbacks);
-	g_free(data->sender);
+	g_dbus_remove_watch(data->connection, data->name_watch);
+	g_free(data->name);
+	g_free(data->owner);
 	g_free(data->path);
 	g_free(data->interface);
 	g_free(data->member);
@@ -322,7 +345,8 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	filter_data_free(data);
 
 	/* Remove filter if there are no listeners left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -359,6 +383,37 @@ static DBusHandlerResult signal_filter(DBusConnection *connection,
 	return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
+static void update_name_cache(const char *name, const char *owner)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, name) != 0)
+			continue;
+
+		g_free(data->owner);
+		data->owner = g_strdup(owner);
+	}
+}
+
+static const char *check_name_cache(const char *name)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, name) != 0)
+			continue;
+
+		return data->owner;
+	}
+
+	return NULL;
+}
+
 static DBusHandlerResult service_filter(DBusConnection *connection,
 					DBusMessage *message, void *user_data)
 {
@@ -375,6 +430,8 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 	}
 
+	update_name_cache(name, new);
+
 	while (data->callbacks) {
 		cb = data->callbacks->data;
 
@@ -421,7 +478,9 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	member = dbus_message_get_member(message);
 	dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &arg, DBUS_TYPE_INVALID);
 
-	data = filter_data_find(connection, sender, path, iface, member, arg);
+	/* Sender is always bus name */
+	data = filter_data_find(connection, NULL, sender, path, iface, member,
+					arg);
 	if (!data) {
 		error("Got %s.%s signal which has no listeners", iface, member);
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
@@ -446,7 +505,8 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	filter_data_free(data);
 
 	/* Remove filter if there no listener left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -456,38 +516,60 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 
 struct service_data {
 	DBusConnection *conn;
+	char *name;
+	const char *owner;
 	GDBusWatchFunction conn_func;
 	void *user_data;
 };
 
+static void service_data_free(struct service_data *data)
+{
+	dbus_connection_unref(data->conn);
+	g_free(data->name);
+	g_free(data);
+}
+
+static gboolean update_service(void *user_data)
+{
+	struct service_data *data = user_data;
+
+	update_name_cache(data->name, data->owner);
+	if (data->conn_func)
+		data->conn_func(data->conn, data->user_data);
+
+	service_data_free(data);
+
+	return FALSE;
+}
+
 static void service_reply(DBusPendingCall *call, void *user_data)
 {
 	struct service_data *data = user_data;
 	DBusMessage *reply;
-	DBusError error;
-	dbus_bool_t has_owner;
+	DBusError err;
 
 	reply = dbus_pending_call_steal_reply(call);
 	if (reply == NULL)
 		return;
 
-	dbus_error_init(&error);
+	dbus_error_init(&err);
 
-	if (dbus_message_get_args(reply, &error,
-					DBUS_TYPE_BOOLEAN, &has_owner,
-						DBUS_TYPE_INVALID) == FALSE) {
-		if (dbus_error_is_set(&error) == TRUE) {
-			error("%s", error.message);
-			dbus_error_free(&error);
-		} else {
-			error("Wrong arguments for NameHasOwner reply");
-		}
-		goto done;
-	}
+	if (dbus_set_error_from_message(&err, reply))
+		goto fail;
 
-	if (has_owner && data->conn_func)
-		data->conn_func(data->conn, data->user_data);
+	if (dbus_message_get_args(reply, &err,
+					DBUS_TYPE_STRING, &data->owner,
+						DBUS_TYPE_INVALID) == FALSE)
+		goto fail;
 
+	update_service(data);
+
+	goto done;
+
+fail:
+	error("%s", err.message);
+	dbus_error_free(&err);
+	service_data_free(data);
 done:
 	dbus_message_unref(reply);
 }
@@ -505,12 +587,19 @@ static void check_service(DBusConnection *connection, const char *name,
 		return;
 	}
 
-	data->conn = connection;
+	data->conn = dbus_connection_ref(connection);
+	data->name = g_strdup(name);
 	data->conn_func = connect;
 	data->user_data = user_data;
 
+	data->owner = check_name_cache(name);
+	if (data->owner != NULL) {
+		g_idle_add(update_service, data);
+		return;
+	}
+
 	message = dbus_message_new_method_call(DBUS_SERVICE_DBUS,
-			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "NameHasOwner");
+			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "GetNameOwner");
 	if (message == NULL) {
 		error("Can't allocate new message");
 		g_free(data);
@@ -596,6 +685,11 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
 	if (!cb)
 		return 0;
 
+	if (data->name != NULL && data->name_watch == 0)
+		data->name_watch = g_dbus_add_service_watch(connection,
+							data->name, NULL,
+							NULL, NULL, NULL);
+
 	return cb->id;
 }
 
@@ -625,7 +719,8 @@ void g_dbus_remove_all_watches(DBusConnection *connection)
 {
 	struct filter_data *data;
 
-	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL))) {
+	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL,
+					NULL, NULL))) {
 		listeners = g_slist_remove(listeners, data);
 		filter_data_call_and_free(data);
 	}
-- 
1.6.3.3


[-- Attachment #3: 0002-Fix-calling-watch-callbacks-after-it-has-been-remove.patch --]
[-- Type: text/x-patch, Size: 5020 bytes --]

From c282840ddba7fb9ace5050bb93ae9ca44b57db9f Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Wed, 17 Feb 2010 17:12:19 +0200
Subject: [PATCH 2/2] Fix calling watch callbacks after it has been removed

Pending call should be removed if the watch is removed since the
application no longer expect that to be reached and may already freed the
data associated with it.
---
 gdbus/watch.c |   79 +++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index ef02201..2bec21a 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -43,11 +43,21 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 static guint listener_id = 0;
 static GSList *listeners = NULL;
 
+struct service_data {
+	DBusConnection *conn;
+	DBusPendingCall *call;
+	char *name;
+	const char *owner;
+	guint id;
+	struct filter_callback *callback;
+};
+
 struct filter_callback {
 	GDBusWatchFunction conn_func;
 	GDBusWatchFunction disc_func;
 	GDBusSignalFunction signal_func;
 	GDBusDestroyFunction destroy_func;
+	struct service_data *data;
 	void *user_data;
 	guint id;
 };
@@ -302,7 +312,7 @@ static struct filter_callback *filter_data_add_callback(
 {
 	struct filter_callback *cb = NULL;
 
-	cb = g_new(struct filter_callback, 1);
+	cb = g_new0(struct filter_callback, 1);
 
 	cb->conn_func = connect;
 	cb->disc_func = disconnect;
@@ -319,6 +329,24 @@ static struct filter_callback *filter_data_add_callback(
 	return cb;
 }
 
+static void service_data_free(struct service_data *data)
+{
+	struct filter_callback *callback = data->callback;
+
+	dbus_connection_unref(data->conn);
+
+	if (data->call)
+		dbus_pending_call_unref(data->call);
+
+	if (data->id)
+		g_source_remove(data->id);
+
+	g_free(data->name);
+	g_free(data);
+
+	callback->data = NULL;
+}
+
 static gboolean filter_data_remove_callback(struct filter_data *data,
 						struct filter_callback *cb)
 {
@@ -327,6 +355,13 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	data->callbacks = g_slist_remove(data->callbacks, cb);
 	data->processed = g_slist_remove(data->processed, cb);
 
+	/* Cancel pending operations */
+	if (cb->data) {
+		if (cb->data->call)
+			dbus_pending_call_cancel(cb->data->call);
+		service_data_free(cb->data);
+	}
+
 	if (cb->destroy_func)
 		cb->destroy_func(cb->user_data);
 
@@ -514,28 +549,14 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
-struct service_data {
-	DBusConnection *conn;
-	char *name;
-	const char *owner;
-	GDBusWatchFunction conn_func;
-	void *user_data;
-};
-
-static void service_data_free(struct service_data *data)
-{
-	dbus_connection_unref(data->conn);
-	g_free(data->name);
-	g_free(data);
-}
-
 static gboolean update_service(void *user_data)
 {
 	struct service_data *data = user_data;
+	struct filter_callback *cb = data->callback;
 
 	update_name_cache(data->name, data->owner);
-	if (data->conn_func)
-		data->conn_func(data->conn, data->user_data);
+	if (cb->conn_func)
+		cb->conn_func(data->conn, cb->user_data);
 
 	service_data_free(data);
 
@@ -574,11 +595,11 @@ done:
 	dbus_message_unref(reply);
 }
 
-static void check_service(DBusConnection *connection, const char *name,
-				GDBusWatchFunction connect, void *user_data)
+static void check_service(DBusConnection *connection,
+					const char *name,
+					struct filter_callback *callback)
 {
 	DBusMessage *message;
-	DBusPendingCall *call;
 	struct service_data *data;
 
 	data = g_try_malloc0(sizeof(*data));
@@ -589,12 +610,12 @@ static void check_service(DBusConnection *connection, const char *name,
 
 	data->conn = dbus_connection_ref(connection);
 	data->name = g_strdup(name);
-	data->conn_func = connect;
-	data->user_data = user_data;
+	data->callback = callback;
+	callback->data = data;
 
 	data->owner = check_name_cache(name);
 	if (data->owner != NULL) {
-		g_idle_add(update_service, data);
+		data->id = g_idle_add(update_service, data);
 		return;
 	}
 
@@ -610,21 +631,19 @@ static void check_service(DBusConnection *connection, const char *name,
 							DBUS_TYPE_INVALID);
 
 	if (dbus_connection_send_with_reply(connection, message,
-							&call, -1) == FALSE) {
+							&data->call, -1) == FALSE) {
 		error("Failed to execute method call");
 		g_free(data);
 		goto done;
 	}
 
-	if (call == NULL) {
+	if (data->call == NULL) {
 		error("D-Bus connection not available");
 		g_free(data);
 		goto done;
 	}
 
-	dbus_pending_call_set_notify(call, service_reply, data, NULL);
-
-	dbus_pending_call_unref(call);
+	dbus_pending_call_set_notify(data->call, service_reply, data, NULL);
 
 done:
 	dbus_message_unref(message);
@@ -653,7 +672,7 @@ guint g_dbus_add_service_watch(DBusConnection *connection, const char *name,
 		return 0;
 
 	if (connect)
-		check_service(connection, name, connect, user_data);
+		check_service(connection, name, cb);
 
 	return cb->id;
 }
-- 
1.6.3.3

Re: [PATCH] Fix signal watch when a service name is given

[Luiz Augusto von Dentz]

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

Hi,

On Wed, Feb 17, 2010 at 6:08 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Marcel,
>
> On Wed, Feb 17, 2010 at 12:00 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Luiz,
>>
>>> @Marcel: So before you apply this you should really apply Vinicius
>>> patch to the rest of projects: bluez, ofono, connman...
>>
>> has been applied to all projects now. Please resend your other patch
>> based on the latest tree.
>
> Resending the patch and also add another one to make sure we cancel
> any pending operation during watch removal.

Resending with the latests fixes to make hfp ofono plugin to work properly.


-- 
Luiz Augusto von Dentz
Computer Engineer

[-- Attachment #2: 0001-Do-not-automatically-remove-watches-for-service-name.patch --]
[-- Type: text/x-patch, Size: 1207 bytes --]

From 84a594eee3b8c3b793526e65936022e0655b99a1 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Sat, 20 Feb 2010 12:16:32 +0200
Subject: [PATCH 1/3] Do not automatically remove watches for service names

Services can be owned again so it is perfectly fine to keep the watch.
---
 gdbus/watch.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 1d479fa..e85f288 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -386,18 +386,19 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
 				cb->conn_func(connection, cb->user_data);
 		}
 
+		/* Only auto remove if it is a bus name watch */
+		if (data->argument[0] == ':' &&
+				(!cb->conn_func || !cb->disc_func)) {
+			filter_data_remove_callback(data, cb);
+			continue;
+		}
+
 		/* Check if the watch was removed/freed by the callback
 		 * function */
 		if (!g_slist_find(data->callbacks, cb))
 			continue;
 
 		data->callbacks = g_slist_remove(data->callbacks, cb);
-
-		if (!cb->conn_func || !cb->disc_func) {
-			g_free(cb);
-			continue;
-		}
-
 		data->processed = g_slist_append(data->processed, cb);
 	}
 
-- 
1.6.3.3


[-- Attachment #3: 0002-Fix-signal-watch-when-a-service-name-is-given.patch --]
[-- Type: text/x-patch, Size: 9393 bytes --]

From f3c194ec729d4fa754cb8a0aa87d0e098d3ff420 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Wed, 17 Feb 2010 17:11:02 +0200
Subject: [PATCH 2/3] Fix signal watch when a service name is given

The bus name should be resolved when adding a watch by service name since
messages do always come with sender set to owner's bus name, also it
should listen to owner updates since it can change without invalidating
the watch.
---
 gdbus/watch.c |  161 +++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 128 insertions(+), 33 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index e85f288..a1acb72 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -55,19 +55,22 @@ struct filter_callback {
 struct filter_data {
 	DBusConnection *connection;
 	DBusHandleMessageFunction handle_func;
-	char *sender;
+	char *name;
+	char *owner;
 	char *path;
 	char *interface;
 	char *member;
 	char *argument;
 	GSList *callbacks;
 	GSList *processed;
+	guint name_watch;
 	gboolean lock;
 	gboolean registered;
 };
 
 static struct filter_data *filter_data_find(DBusConnection *connection,
-							const char *sender,
+							const char *name,
+							const char *owner,
 							const char *path,
 							const char *interface,
 							const char *member,
@@ -82,8 +85,12 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 		if (connection != data->connection)
 			continue;
 
-		if (sender && data->sender &&
-				g_str_equal(sender, data->sender) == FALSE)
+		if (name && data->name &&
+				g_str_equal(name, data->name) == FALSE)
+			continue;
+
+		if (owner && data->owner &&
+				g_str_equal(owner, data->owner) == FALSE)
 			continue;
 
 		if (path && data->path &&
@@ -110,13 +117,15 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 
 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 (data->sender)
+	if (sender)
 		offset += snprintf(rule + offset, size - offset,
-				",sender='%s'", data->sender);
+				",sender='%s'", sender);
 	if (data->path)
 		offset += snprintf(rule + offset, size - offset,
 				",path='%s'", data->path);
@@ -183,8 +192,10 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 					const char *argument)
 {
 	struct filter_data *data;
+	const char *name = NULL, *owner = NULL;
 
-	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL)) {
+	if (!filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+				NULL)) {
 		if (!dbus_connection_add_filter(connection,
 					message_filter, NULL, NULL)) {
 			error("dbus_connection_add_filter() failed");
@@ -192,15 +203,25 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 		}
 	}
 
-	data = filter_data_find(connection, sender, path, interface, member,
-					argument);
+	if (sender == NULL)
+		goto proceed;
+
+	if (sender[0] == ':')
+		owner = sender;
+	else
+		name = sender;
+
+proceed:
+	data = filter_data_find(connection, name, owner, path, interface,
+					member, argument);
 	if (data)
 		return data;
 
 	data = g_new0(struct filter_data, 1);
 
 	data->connection = dbus_connection_ref(connection);
-	data->sender = g_strdup(sender);
+	data->name = name ? g_strdup(name) : NULL;
+	data->owner = owner ? g_strdup(owner) : NULL;
 	data->path = g_strdup(path);
 	data->interface = g_strdup(interface);
 	data->member = g_strdup(member);
@@ -244,7 +265,9 @@ static void filter_data_free(struct filter_data *data)
 		g_free(l->data);
 
 	g_slist_free(data->callbacks);
-	g_free(data->sender);
+	g_dbus_remove_watch(data->connection, data->name_watch);
+	g_free(data->name);
+	g_free(data->owner);
 	g_free(data->path);
 	g_free(data->interface);
 	g_free(data->member);
@@ -322,7 +345,8 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	filter_data_free(data);
 
 	/* Remove filter if there are no listeners left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -359,6 +383,37 @@ static DBusHandlerResult signal_filter(DBusConnection *connection,
 	return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
+static void update_name_cache(const char *name, const char *owner)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, name) != 0)
+			continue;
+
+		g_free(data->owner);
+		data->owner = g_strdup(owner);
+	}
+}
+
+static const char *check_name_cache(const char *name)
+{
+	GSList *l;
+
+	for (l = listeners; l != NULL; l = l->next) {
+		struct filter_data *data = l->data;
+
+		if (g_strcmp0(data->name, name) != 0)
+			continue;
+
+		return data->owner;
+	}
+
+	return NULL;
+}
+
 static DBusHandlerResult service_filter(DBusConnection *connection,
 					DBusMessage *message, void *user_data)
 {
@@ -375,6 +430,8 @@ static DBusHandlerResult service_filter(DBusConnection *connection,
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 	}
 
+	update_name_cache(name, new);
+
 	while (data->callbacks) {
 		cb = data->callbacks->data;
 
@@ -422,7 +479,9 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	member = dbus_message_get_member(message);
 	dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &arg, DBUS_TYPE_INVALID);
 
-	data = filter_data_find(connection, sender, path, iface, member, arg);
+	/* Sender is always bus name */
+	data = filter_data_find(connection, NULL, sender, path, iface, member,
+					arg);
 	if (!data) {
 		error("Got %s.%s signal which has no listeners", iface, member);
 		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
@@ -447,7 +506,8 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	filter_data_free(data);
 
 	/* Remove filter if there no listener left for the connection */
-	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL);
+	data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
+					NULL);
 	if (!data)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
@@ -457,38 +517,60 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 
 struct service_data {
 	DBusConnection *conn;
+	char *name;
+	const char *owner;
 	GDBusWatchFunction conn_func;
 	void *user_data;
 };
 
+static void service_data_free(struct service_data *data)
+{
+	dbus_connection_unref(data->conn);
+	g_free(data->name);
+	g_free(data);
+}
+
+static gboolean update_service(void *user_data)
+{
+	struct service_data *data = user_data;
+
+	update_name_cache(data->name, data->owner);
+	if (data->conn_func)
+		data->conn_func(data->conn, data->user_data);
+
+	service_data_free(data);
+
+	return FALSE;
+}
+
 static void service_reply(DBusPendingCall *call, void *user_data)
 {
 	struct service_data *data = user_data;
 	DBusMessage *reply;
-	DBusError error;
-	dbus_bool_t has_owner;
+	DBusError err;
 
 	reply = dbus_pending_call_steal_reply(call);
 	if (reply == NULL)
 		return;
 
-	dbus_error_init(&error);
+	dbus_error_init(&err);
 
-	if (dbus_message_get_args(reply, &error,
-					DBUS_TYPE_BOOLEAN, &has_owner,
-						DBUS_TYPE_INVALID) == FALSE) {
-		if (dbus_error_is_set(&error) == TRUE) {
-			error("%s", error.message);
-			dbus_error_free(&error);
-		} else {
-			error("Wrong arguments for NameHasOwner reply");
-		}
-		goto done;
-	}
+	if (dbus_set_error_from_message(&err, reply))
+		goto fail;
 
-	if (has_owner && data->conn_func)
-		data->conn_func(data->conn, data->user_data);
+	if (dbus_message_get_args(reply, &err,
+					DBUS_TYPE_STRING, &data->owner,
+						DBUS_TYPE_INVALID) == FALSE)
+		goto fail;
 
+	update_service(data);
+
+	goto done;
+
+fail:
+	error("%s", err.message);
+	dbus_error_free(&err);
+	service_data_free(data);
 done:
 	dbus_message_unref(reply);
 }
@@ -506,12 +588,19 @@ static void check_service(DBusConnection *connection, const char *name,
 		return;
 	}
 
-	data->conn = connection;
+	data->conn = dbus_connection_ref(connection);
+	data->name = g_strdup(name);
 	data->conn_func = connect;
 	data->user_data = user_data;
 
+	data->owner = check_name_cache(name);
+	if (data->owner != NULL) {
+		g_idle_add(update_service, data);
+		return;
+	}
+
 	message = dbus_message_new_method_call(DBUS_SERVICE_DBUS,
-			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "NameHasOwner");
+			DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "GetNameOwner");
 	if (message == NULL) {
 		error("Can't allocate new message");
 		g_free(data);
@@ -597,6 +686,11 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
 	if (!cb)
 		return 0;
 
+	if (data->name != NULL && data->name_watch == 0)
+		data->name_watch = g_dbus_add_service_watch(connection,
+							data->name, NULL,
+							NULL, NULL, NULL);
+
 	return cb->id;
 }
 
@@ -626,7 +720,8 @@ void g_dbus_remove_all_watches(DBusConnection *connection)
 {
 	struct filter_data *data;
 
-	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL, NULL))) {
+	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL,
+					NULL, NULL))) {
 		listeners = g_slist_remove(listeners, data);
 		filter_data_call_and_free(data);
 	}
-- 
1.6.3.3


[-- Attachment #4: 0003-Fix-calling-watch-callbacks-after-it-has-been-remove.patch --]
[-- Type: text/x-patch, Size: 5020 bytes --]

From 35d3c2168ac95967cd4e6b23c7e66e770e540395 Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Wed, 17 Feb 2010 17:12:19 +0200
Subject: [PATCH 3/3] Fix calling watch callbacks after it has been removed

Pending call should be removed if the watch is removed since the
application no longer expect that to be reached and may already freed the
data associated with it.
---
 gdbus/watch.c |   79 +++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index a1acb72..c0dcc93 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -43,11 +43,21 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 static guint listener_id = 0;
 static GSList *listeners = NULL;
 
+struct service_data {
+	DBusConnection *conn;
+	DBusPendingCall *call;
+	char *name;
+	const char *owner;
+	guint id;
+	struct filter_callback *callback;
+};
+
 struct filter_callback {
 	GDBusWatchFunction conn_func;
 	GDBusWatchFunction disc_func;
 	GDBusSignalFunction signal_func;
 	GDBusDestroyFunction destroy_func;
+	struct service_data *data;
 	void *user_data;
 	guint id;
 };
@@ -302,7 +312,7 @@ static struct filter_callback *filter_data_add_callback(
 {
 	struct filter_callback *cb = NULL;
 
-	cb = g_new(struct filter_callback, 1);
+	cb = g_new0(struct filter_callback, 1);
 
 	cb->conn_func = connect;
 	cb->disc_func = disconnect;
@@ -319,6 +329,24 @@ static struct filter_callback *filter_data_add_callback(
 	return cb;
 }
 
+static void service_data_free(struct service_data *data)
+{
+	struct filter_callback *callback = data->callback;
+
+	dbus_connection_unref(data->conn);
+
+	if (data->call)
+		dbus_pending_call_unref(data->call);
+
+	if (data->id)
+		g_source_remove(data->id);
+
+	g_free(data->name);
+	g_free(data);
+
+	callback->data = NULL;
+}
+
 static gboolean filter_data_remove_callback(struct filter_data *data,
 						struct filter_callback *cb)
 {
@@ -327,6 +355,13 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	data->callbacks = g_slist_remove(data->callbacks, cb);
 	data->processed = g_slist_remove(data->processed, cb);
 
+	/* Cancel pending operations */
+	if (cb->data) {
+		if (cb->data->call)
+			dbus_pending_call_cancel(cb->data->call);
+		service_data_free(cb->data);
+	}
+
 	if (cb->destroy_func)
 		cb->destroy_func(cb->user_data);
 
@@ -515,28 +550,14 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
-struct service_data {
-	DBusConnection *conn;
-	char *name;
-	const char *owner;
-	GDBusWatchFunction conn_func;
-	void *user_data;
-};
-
-static void service_data_free(struct service_data *data)
-{
-	dbus_connection_unref(data->conn);
-	g_free(data->name);
-	g_free(data);
-}
-
 static gboolean update_service(void *user_data)
 {
 	struct service_data *data = user_data;
+	struct filter_callback *cb = data->callback;
 
 	update_name_cache(data->name, data->owner);
-	if (data->conn_func)
-		data->conn_func(data->conn, data->user_data);
+	if (cb->conn_func)
+		cb->conn_func(data->conn, cb->user_data);
 
 	service_data_free(data);
 
@@ -575,11 +596,11 @@ done:
 	dbus_message_unref(reply);
 }
 
-static void check_service(DBusConnection *connection, const char *name,
-				GDBusWatchFunction connect, void *user_data)
+static void check_service(DBusConnection *connection,
+					const char *name,
+					struct filter_callback *callback)
 {
 	DBusMessage *message;
-	DBusPendingCall *call;
 	struct service_data *data;
 
 	data = g_try_malloc0(sizeof(*data));
@@ -590,12 +611,12 @@ static void check_service(DBusConnection *connection, const char *name,
 
 	data->conn = dbus_connection_ref(connection);
 	data->name = g_strdup(name);
-	data->conn_func = connect;
-	data->user_data = user_data;
+	data->callback = callback;
+	callback->data = data;
 
 	data->owner = check_name_cache(name);
 	if (data->owner != NULL) {
-		g_idle_add(update_service, data);
+		data->id = g_idle_add(update_service, data);
 		return;
 	}
 
@@ -611,21 +632,19 @@ static void check_service(DBusConnection *connection, const char *name,
 							DBUS_TYPE_INVALID);
 
 	if (dbus_connection_send_with_reply(connection, message,
-							&call, -1) == FALSE) {
+							&data->call, -1) == FALSE) {
 		error("Failed to execute method call");
 		g_free(data);
 		goto done;
 	}
 
-	if (call == NULL) {
+	if (data->call == NULL) {
 		error("D-Bus connection not available");
 		g_free(data);
 		goto done;
 	}
 
-	dbus_pending_call_set_notify(call, service_reply, data, NULL);
-
-	dbus_pending_call_unref(call);
+	dbus_pending_call_set_notify(data->call, service_reply, data, NULL);
 
 done:
 	dbus_message_unref(message);
@@ -654,7 +673,7 @@ guint g_dbus_add_service_watch(DBusConnection *connection, const char *name,
 		return 0;
 
 	if (connect)
-		check_service(connection, name, connect, user_data);
+		check_service(connection, name, cb);
 
 	return cb->id;
 }
-- 
1.6.3.3