[PATCH BlueZ v0 0/7] Add removing GATT services

[PATCH BlueZ v0 0/7] Add removing GATT services

[Claudio Takahasi]

This patchset implements GattManager1.UnregisterService(), and low-level
functions to remove service declarations (and its attributes) from the
attribute database.

Andre Guedes (1):
  gatt: Add helper for removing GATT services

Claudio Takahasi (6):
  gdbus: Avoid reporting GDBusClient disconnect twice
  gatt: Rename external_app to external_service
  gatt: Add GattManager1.UnregisterService()
  gatt: Add removing service from the database
  gatt: Fix possibly lost block when bluetoothd exits
  doc: Add InvalidArguments to UnregisterService() errors

 doc/gatt-api.txt |   3 +-
 gdbus/client.c   |  14 ++++-
 src/gatt-dbus.c  | 162 ++++++++++++++++++++++++++++++++++---------------------
 src/gatt.c       |  42 +++++++++++++++
 src/gatt.h       |   7 +++
 5 files changed, 165 insertions(+), 63 deletions(-)

-- 
1.8.3.1

[PATCH BlueZ v0 1/7] gatt: Add helper for removing GATT services

[Claudio Takahasi]

From: Andre Guedes <andre.guedes@openbossa.org>

This patch adds the btd_gatt_remove_service() helper which removes a GATT
primary (or secondary) Service declaration (including its characteristics)
from the local attribute database.
---
 src/gatt.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 src/gatt.h |  7 +++++++
 2 files changed, 49 insertions(+)

diff --git a/src/gatt.c b/src/gatt.c
index f07effa..3060462 100644
--- a/src/gatt.c
+++ b/src/gatt.c
@@ -26,6 +26,7 @@
 #endif
 
 #include <glib.h>
+#include <stdbool.h>
 
 #include "log.h"
 #include "lib/uuid.h"
@@ -104,6 +105,18 @@ static struct btd_attribute *new_attribute(const bt_uuid_t *type,
 	return attr;
 }
 
+static bool is_service(const struct btd_attribute *attr)
+{
+	if (attr->type.type != BT_UUID16)
+		return false;
+
+	if (attr->type.value.u16 == GATT_PRIM_SVC_UUID ||
+			attr->type.value.u16 == GATT_SND_SVC_UUID)
+		return true;
+
+	return false;
+}
+
 static int local_database_add(uint16_t handle, struct btd_attribute *attr)
 {
 	attr->handle = handle;
@@ -149,6 +162,35 @@ struct btd_attribute *btd_gatt_add_service(const bt_uuid_t *uuid)
 	return attr;
 }
 
+void btd_gatt_remove_service(struct btd_attribute *service)
+{
+	GList *list = g_list_find(local_attribute_db, service);
+	bool first_node;
+
+	if (!list)
+		return;
+
+	first_node = local_attribute_db == list;
+
+	/* Remove service declaration attribute */
+	free(list->data);
+	list = g_list_delete_link(list, list);
+
+	/* Remove all characteristics until next service declaration */
+	while (list && !is_service(list->data)) {
+		free(list->data);
+		list = g_list_delete_link(list, list);
+	}
+
+	/*
+	 * When removing the first node, local attribute database head
+	 * needs to be updated. Node removed from middle doesn't change
+	 * the list head address.
+	 */
+	if (first_node)
+		local_attribute_db = list;
+}
+
 struct btd_attribute *btd_gatt_add_char(const bt_uuid_t *uuid,
 						uint8_t properties,
 						btd_attr_read_t read_cb,
diff --git a/src/gatt.h b/src/gatt.h
index da7af92..f16541e 100644
--- a/src/gatt.h
+++ b/src/gatt.h
@@ -81,6 +81,13 @@ typedef void (*btd_attr_write_t) (struct btd_attribute *attr,
 struct btd_attribute *btd_gatt_add_service(const bt_uuid_t *uuid);
 
 /*
+ * btd_gatt_remove_service - Remove a service (along with all its
+ * characteristics) from the local attribute database.
+ * @service:	Service declaration attribute.
+ */
+void btd_gatt_remove_service(struct btd_attribute *service);
+
+/*
  * btd_gatt_add_char - Add a characteristic (declaration and value attributes)
  * to local attribute database.
  * @uuid:	Characteristic UUID (16-bits or 128-bits).
-- 
1.8.3.1

[PATCH BlueZ v0 2/7] gdbus: Avoid reporting GDBusClient disconnect twice

[Claudio Takahasi]

No matter if disconnection was reported previously, g_dbus_client_unref()
was always calling service disconnect callback. This patch fix the
following scenario:
1) service disconnects from the bus
2) disconnect callback gets called
3) client calls g_dbus_client_unref(), disconnect callback is called
   again.
---
 gdbus/client.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/gdbus/client.c b/gdbus/client.c
index 3bf883a..eb68a0f 100644
--- a/gdbus/client.c
+++ b/gdbus/client.c
@@ -51,6 +51,7 @@ struct GDBusClient {
 	GDBusWatchFunction connect_func;
 	void *connect_data;
 	GDBusWatchFunction disconn_func;
+	gboolean connected;
 	void *disconn_data;
 	GDBusMessageFunction signal_func;
 	void *signal_data;
@@ -1146,6 +1147,8 @@ static void service_connect(DBusConnection *conn, void *user_data)
 
 	get_managed_objects(client);
 
+	client->connected = TRUE;
+
 	g_dbus_client_unref(client);
 }
 
@@ -1156,8 +1159,10 @@ static void service_disconnect(DBusConnection *conn, void *user_data)
 	g_list_free_full(client->proxy_list, proxy_free);
 	client->proxy_list = NULL;
 
-	if (client->disconn_func)
+	if (client->disconn_func) {
 		client->disconn_func(conn, client->disconn_data);
+		client->connected = FALSE;
+	}
 }
 
 static DBusHandlerResult message_filter(DBusConnection *connection,
@@ -1210,6 +1215,7 @@ GDBusClient *g_dbus_client_new(DBusConnection *connection,
 	client->dbus_conn = dbus_connection_ref(connection);
 	client->service_name = g_strdup(service);
 	client->base_path = g_strdup(path);
+	client->connected = FALSE;
 
 	client->match_rules = g_ptr_array_sized_new(1);
 	g_ptr_array_set_free_func(client->match_rules, g_free);
@@ -1284,7 +1290,11 @@ void g_dbus_client_unref(GDBusClient *client)
 
 	g_list_free_full(client->proxy_list, proxy_free);
 
-	if (client->disconn_func)
+	/*
+	 * Don't call disconn_func twice if disconnection
+	 * was previously reported.
+	 */
+	if (client->disconn_func && client->connected)
 		client->disconn_func(client->dbus_conn, client->disconn_data);
 
 	g_dbus_remove_watch(client->dbus_conn, client->watch);
-- 
1.8.3.1

[PATCH BlueZ v0 3/7] gatt: Rename external_app to external_service

[Claudio Takahasi]

Even for external applications providing multiple services, each
external service should have its own instance of GDBusClient. This
approach allows a more dynamic management of the registered services.
Based on this assumption, rename the external_app to external_service
is more logical.
---
 src/gatt-dbus.c | 107 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 54 insertions(+), 53 deletions(-)

diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index 1bfa21f..b29371d 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -50,7 +50,7 @@
 #define GATT_CHR_IFACE			"org.bluez.GattCharacteristic1"
 #define GATT_DESCRIPTOR_IFACE		"org.bluez.GattDescriptor1"
 
-struct external_app {
+struct external_service {
 	char *owner;
 	char *path;
 	DBusMessage *reg;
@@ -70,31 +70,31 @@ struct proxy_write_data {
  */
 static GHashTable *proxy_hash;
 
-static GSList *external_apps;
+static GSList *external_services;
 
-static int external_app_path_cmp(gconstpointer a, gconstpointer b)
+static int external_service_path_cmp(gconstpointer a, gconstpointer b)
 {
-	const struct external_app *eapp = a;
+	const struct external_service *esvc = a;
 	const char *path = b;
 
-	return g_strcmp0(eapp->path, path);
+	return g_strcmp0(esvc->path, path);
 }
 
-static void external_app_watch_destroy(gpointer user_data)
+static void external_service_watch_destroy(gpointer user_data)
 {
-	struct external_app *eapp = user_data;
+	struct external_service *esvc = user_data;
 
 	/* TODO: Remove from the database */
 
-	external_apps = g_slist_remove(external_apps, eapp);
+	external_services = g_slist_remove(external_services, esvc);
 
-	g_dbus_client_unref(eapp->client);
-	if (eapp->reg)
-		dbus_message_unref(eapp->reg);
+	g_dbus_client_unref(esvc->client);
+	if (esvc->reg)
+		dbus_message_unref(esvc->reg);
 
-	g_free(eapp->owner);
-	g_free(eapp->path);
-	g_free(eapp);
+	g_free(esvc->owner);
+	g_free(esvc->path);
+	g_free(esvc);
 }
 
 static int proxy_path_cmp(gconstpointer a, gconstpointer b)
@@ -167,13 +167,13 @@ fail:
 
 static void proxy_added(GDBusProxy *proxy, void *user_data)
 {
-	struct external_app *eapp = user_data;
+	struct external_service *esvc = user_data;
 	const char *interface, *path;
 
 	interface = g_dbus_proxy_get_interface(proxy);
 	path = g_dbus_proxy_get_path(proxy);
 
-	if (!g_str_has_prefix(path, eapp->path))
+	if (!g_str_has_prefix(path, esvc->path))
 		return;
 
 	if (g_strcmp0(interface, GATT_CHR_IFACE) != 0 &&
@@ -188,13 +188,13 @@ static void proxy_added(GDBusProxy *proxy, void *user_data)
 	 * proxies sorted by path helps the logic to register the
 	 * object path later.
 	 */
-	eapp->proxies = g_slist_insert_sorted(eapp->proxies, proxy,
+	esvc->proxies = g_slist_insert_sorted(esvc->proxies, proxy,
 							proxy_path_cmp);
 }
 
 static void proxy_removed(GDBusProxy *proxy, void *user_data)
 {
-	struct external_app *eapp = user_data;
+	struct external_service *esvc = user_data;
 	const char *interface, *path;
 
 	interface = g_dbus_proxy_get_interface(proxy);
@@ -202,7 +202,7 @@ static void proxy_removed(GDBusProxy *proxy, void *user_data)
 
 	DBG("path %s iface %s", path, interface);
 
-	eapp->proxies = g_slist_remove(eapp->proxies, proxy);
+	esvc->proxies = g_slist_remove(esvc->proxies, proxy);
 }
 
 static void proxy_read_cb(struct btd_attribute *attr,
@@ -323,7 +323,7 @@ static void proxy_write_cb(struct btd_attribute *attr,
 
 }
 
-static int register_external_service(const struct external_app *eapp,
+static int register_external_service(const struct external_service *esvc,
 							GDBusProxy *proxy)
 {
 	DBusMessageIter iter;
@@ -332,7 +332,7 @@ static int register_external_service(const struct external_app *eapp,
 
 	path = g_dbus_proxy_get_path(proxy);
 	iface = g_dbus_proxy_get_interface(proxy);
-	if (g_strcmp0(eapp->path, path) != 0 ||
+	if (g_strcmp0(esvc->path, path) != 0 ||
 			g_strcmp0(iface, GATT_SERVICE_IFACE) != 0)
 		return -EINVAL;
 
@@ -450,43 +450,43 @@ static int register_external_characteristics(GSList *proxies)
 
 static void client_ready(GDBusClient *client, void *user_data)
 {
-	struct external_app *eapp = user_data;
+	struct external_service *esvc = user_data;
 	GDBusProxy *proxy;
 	DBusConnection *conn = btd_get_dbus_connection();
 	DBusMessage *reply;
 
-	if (!eapp->proxies)
+	if (!esvc->proxies)
 		goto fail;
 
-	proxy = eapp->proxies->data;
-	if (register_external_service(eapp, proxy) < 0)
+	proxy = esvc->proxies->data;
+	if (register_external_service(esvc, proxy) < 0)
 		goto fail;
 
-	if (register_external_characteristics(g_slist_next(eapp->proxies)) < 0)
+	if (register_external_characteristics(g_slist_next(esvc->proxies)) < 0)
 		goto fail;
 
-	DBG("Added GATT service %s", eapp->path);
+	DBG("Added GATT service %s", esvc->path);
 
-	reply = dbus_message_new_method_return(eapp->reg);
+	reply = dbus_message_new_method_return(esvc->reg);
 	goto reply;
 
 fail:
-	error("Could not register external service: %s", eapp->path);
+	error("Could not register external service: %s", esvc->path);
 
-	reply = btd_error_invalid_args(eapp->reg);
-	/* TODO: missing eapp/database cleanup */
+	reply = btd_error_invalid_args(esvc->reg);
+	/* TODO: missing esvc/database cleanup */
 
 reply:
-	dbus_message_unref(eapp->reg);
-	eapp->reg = NULL;
+	dbus_message_unref(esvc->reg);
+	esvc->reg = NULL;
 
 	g_dbus_send_message(conn, reply);
 }
 
-static struct external_app *new_external_app(DBusConnection *conn,
+static struct external_service *new_external_service(DBusConnection *conn,
 					DBusMessage *msg, const char *path)
 {
-	struct external_app *eapp;
+	struct external_service *esvc;
 	GDBusClient *client;
 	const char *sender = dbus_message_get_sender(msg);
 
@@ -494,33 +494,33 @@ static struct external_app *new_external_app(DBusConnection *conn,
 	if (!client)
 		return NULL;
 
-	eapp = g_new0(struct external_app, 1);
+	esvc = g_new0(struct external_service, 1);
 
-	eapp->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
-			sender, NULL, eapp, external_app_watch_destroy);
-	if (eapp->watch == 0) {
+	esvc->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
+			sender, NULL, esvc, external_service_watch_destroy);
+	if (esvc->watch == 0) {
 		g_dbus_client_unref(client);
-		g_free(eapp);
+		g_free(esvc);
 		return NULL;
 	}
 
-	eapp->owner = g_strdup(sender);
-	eapp->reg = dbus_message_ref(msg);
-	eapp->client = client;
-	eapp->path = g_strdup(path);
+	esvc->owner = g_strdup(sender);
+	esvc->reg = dbus_message_ref(msg);
+	esvc->client = client;
+	esvc->path = g_strdup(path);
 
 	g_dbus_client_set_proxy_handlers(client, proxy_added, proxy_removed,
-								NULL, eapp);
+								NULL, esvc);
 
-	g_dbus_client_set_ready_watch(client, client_ready, eapp);
+	g_dbus_client_set_ready_watch(client, client_ready, esvc);
 
-	return eapp;
+	return esvc;
 }
 
 static DBusMessage *register_service(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
-	struct external_app *eapp;
+	struct external_service *esvc;
 	DBusMessageIter iter;
 	const char *path;
 
@@ -532,16 +532,17 @@ static DBusMessage *register_service(DBusConnection *conn,
 
 	dbus_message_iter_get_basic(&iter, &path);
 
-	if (g_slist_find_custom(external_apps, path, external_app_path_cmp))
+	if (g_slist_find_custom(external_services, path,
+						external_service_path_cmp))
 		return btd_error_already_exists(msg);
 
-	eapp = new_external_app(conn, msg, path);
-	if (!eapp)
+	esvc = new_external_service(conn, msg, path);
+	if (!esvc)
 		return btd_error_failed(msg, "Not enough resources");
 
-	external_apps = g_slist_prepend(external_apps, eapp);
+	external_services = g_slist_prepend(external_services, esvc);
 
-	DBG("New app %p: %s", eapp, path);
+	DBG("New service %p: %s", esvc, path);
 
 	return NULL;
 }
-- 
1.8.3.1

[PATCH BlueZ v0 4/7] gatt: Add GattManager1.UnregisterService()

[Claudio Takahasi]

This patch adds GattManager1.UnregisterService() method implementation
according to doc/gatt-api.txt.
---
 src/gatt-dbus.c | 64 ++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 17 deletions(-)

diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index b29371d..3ded9cd 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -56,7 +56,6 @@ struct external_service {
 	DBusMessage *reg;
 	GDBusClient *client;
 	GSList *proxies;
-	unsigned int watch;
 };
 
 struct proxy_write_data {
@@ -80,21 +79,35 @@ static int external_service_path_cmp(gconstpointer a, gconstpointer b)
 	return g_strcmp0(esvc->path, path);
 }
 
-static void external_service_watch_destroy(gpointer user_data)
+static gboolean external_service_destroy(void *user_data)
 {
 	struct external_service *esvc = user_data;
 
-	/* TODO: Remove from the database */
-
-	external_services = g_slist_remove(external_services, esvc);
-
 	g_dbus_client_unref(esvc->client);
+
 	if (esvc->reg)
 		dbus_message_unref(esvc->reg);
 
 	g_free(esvc->owner);
 	g_free(esvc->path);
 	g_free(esvc);
+
+	return FALSE;
+}
+
+static void remove_service(DBusConnection *conn, void *user_data)
+{
+	struct external_service *esvc = user_data;
+
+	/* TODO: Remove from the database */
+
+	external_services = g_slist_remove(external_services, esvc);
+
+	/*
+	 * Do not run in the same loop, this may be a disconnect
+	 * watch call and GDBusClient should not be destroyed.
+	 */
+	g_idle_add(external_service_destroy, esvc);
 }
 
 static int proxy_path_cmp(gconstpointer a, gconstpointer b)
@@ -483,7 +496,7 @@ reply:
 	g_dbus_send_message(conn, reply);
 }
 
-static struct external_service *new_external_service(DBusConnection *conn,
+static struct external_service *external_service_new(DBusConnection *conn,
 					DBusMessage *msg, const char *path)
 {
 	struct external_service *esvc;
@@ -495,20 +508,13 @@ static struct external_service *new_external_service(DBusConnection *conn,
 		return NULL;
 
 	esvc = g_new0(struct external_service, 1);
-
-	esvc->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
-			sender, NULL, esvc, external_service_watch_destroy);
-	if (esvc->watch == 0) {
-		g_dbus_client_unref(client);
-		g_free(esvc);
-		return NULL;
-	}
-
 	esvc->owner = g_strdup(sender);
 	esvc->reg = dbus_message_ref(msg);
 	esvc->client = client;
 	esvc->path = g_strdup(path);
 
+	g_dbus_client_set_disconnect_watch(client, remove_service, esvc);
+
 	g_dbus_client_set_proxy_handlers(client, proxy_added, proxy_removed,
 								NULL, esvc);
 
@@ -536,7 +542,7 @@ static DBusMessage *register_service(DBusConnection *conn,
 						external_service_path_cmp))
 		return btd_error_already_exists(msg);
 
-	esvc = new_external_service(conn, msg, path);
+	esvc = external_service_new(conn, msg, path);
 	if (!esvc)
 		return btd_error_failed(msg, "Not enough resources");
 
@@ -550,6 +556,30 @@ static DBusMessage *register_service(DBusConnection *conn,
 static DBusMessage *unregister_service(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
+	struct external_service *esvc;
+	DBusMessageIter iter;
+	const char *path;
+	GSList *list;
+
+	if (!dbus_message_iter_init(msg, &iter))
+		return btd_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_OBJECT_PATH)
+		return btd_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&iter, &path);
+
+	list = g_slist_find_custom(external_services, path,
+						external_service_path_cmp);
+	if (!list)
+		return btd_error_does_not_exist(msg);
+
+	esvc = list->data;
+	if (!strcmp(dbus_message_get_sender(msg), esvc->owner))
+		return btd_error_does_not_exist(msg);
+
+	remove_service(conn, esvc);
+
 	return dbus_message_new_method_return(msg);
 }
 
-- 
1.8.3.1

[PATCH BlueZ v0 5/7] gatt: Add removing service from the database

[Claudio Takahasi]

This patch removes the service declaration and its attributes when
the external service implementation leaves the system bus, calls
UnregisterService(), or RegisterService() fails.
---
 src/gatt-dbus.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index 3ded9cd..17c3359 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -56,6 +56,7 @@ struct external_service {
 	DBusMessage *reg;
 	GDBusClient *client;
 	GSList *proxies;
+	struct btd_attribute *service;
 };
 
 struct proxy_write_data {
@@ -99,7 +100,8 @@ static void remove_service(DBusConnection *conn, void *user_data)
 {
 	struct external_service *esvc = user_data;
 
-	/* TODO: Remove from the database */
+	if (esvc->service)
+		btd_gatt_remove_service(esvc->service);
 
 	external_services = g_slist_remove(external_services, esvc);
 
@@ -336,7 +338,7 @@ static void proxy_write_cb(struct btd_attribute *attr,
 
 }
 
-static int register_external_service(const struct external_service *esvc,
+static int register_external_service(struct external_service *esvc,
 							GDBusProxy *proxy)
 {
 	DBusMessageIter iter;
@@ -360,7 +362,8 @@ static int register_external_service(const struct external_service *esvc,
 	if (bt_string_to_uuid(&uuid, str) < 0)
 		return -EINVAL;
 
-	if (!btd_gatt_add_service(&uuid))
+	esvc->service = btd_gatt_add_service(&uuid);
+	if (!esvc->service)
 		return -EINVAL;
 
 	return 0;
@@ -487,7 +490,8 @@ fail:
 	error("Could not register external service: %s", esvc->path);
 
 	reply = btd_error_invalid_args(esvc->reg);
-	/* TODO: missing esvc/database cleanup */
+	if (esvc->service)
+		btd_gatt_remove_service(esvc->service);
 
 reply:
 	dbus_message_unref(esvc->reg);
-- 
1.8.3.1

Re: [PATCH BlueZ v0 5/7] gatt: Add removing service from the database

[Johan Hedberg]

Hi Claudio,

On Wed, Apr 02, 2014, Claudio Takahasi wrote:
> This patch removes the service declaration and its attributes when
> the external service implementation leaves the system bus, calls
> UnregisterService(), or RegisterService() fails.
> ---
>  src/gatt-dbus.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

I've applied patches 1-4.

> @@ -99,7 +100,8 @@ static void remove_service(DBusConnection *conn, void *user_data)
>  {
>  	struct external_service *esvc = user_data;
>  
> -	/* TODO: Remove from the database */
> +	if (esvc->service)
> +		btd_gatt_remove_service(esvc->service);

Would it not be safer to set esvc->service to NULL after this?

> @@ -360,7 +362,8 @@ static int register_external_service(const struct external_service *esvc,
>  	if (bt_string_to_uuid(&uuid, str) < 0)
>  		return -EINVAL;
>  
> -	if (!btd_gatt_add_service(&uuid))
> +	esvc->service = btd_gatt_add_service(&uuid);
> +	if (!esvc->service)
>  		return -EINVAL;
>  
>  	return 0;
> @@ -487,7 +490,8 @@ fail:
>  	error("Could not register external service: %s", esvc->path);
>  
>  	reply = btd_error_invalid_args(esvc->reg);
> -	/* TODO: missing esvc/database cleanup */
> +	if (esvc->service)
> +		btd_gatt_remove_service(esvc->service);

Same here.

Johan

Re: [PATCH BlueZ v0 5/7] gatt: Add removing service from the database

[Claudio Takahasi]

Hi Johan:

On Thu, Apr 3, 2014 at 4:49 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Claudio,
>
> On Wed, Apr 02, 2014, Claudio Takahasi wrote:
>> This patch removes the service declaration and its attributes when
>> the external service implementation leaves the system bus, calls
>> UnregisterService(), or RegisterService() fails.
>> ---
>>  src/gatt-dbus.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> I've applied patches 1-4.
>
>> @@ -99,7 +100,8 @@ static void remove_service(DBusConnection *conn, void *user_data)
>>  {
>>       struct external_service *esvc = user_data;
>>
>> -     /* TODO: Remove from the database */
>> +     if (esvc->service)
>> +             btd_gatt_remove_service(esvc->service);
>
> Would it not be safer to set esvc->service to NULL after this?
>
>> @@ -360,7 +362,8 @@ static int register_external_service(const struct external_service *esvc,
>>       if (bt_string_to_uuid(&uuid, str) < 0)
>>               return -EINVAL;
>>
>> -     if (!btd_gatt_add_service(&uuid))
>> +     esvc->service = btd_gatt_add_service(&uuid);
>> +     if (!esvc->service)
>>               return -EINVAL;
>>
>>       return 0;
>> @@ -487,7 +490,8 @@ fail:
>>       error("Could not register external service: %s", esvc->path);
>>
>>       reply = btd_error_invalid_args(esvc->reg);
>> -     /* TODO: missing esvc/database cleanup */
>> +     if (esvc->service)
>> +             btd_gatt_remove_service(esvc->service);
>
> Same here.
>
> Johan

Actually, there is a potential invalid memory access related to esvc
(external_service), set esvc->service to NULL is not necessary.

Race condition & invalid memory access can happen when calling
remove_service callback & GDBusClient unref.

I will send another patchset fixing this problem.

Regards,
Claudio

[PATCH BlueZ v0 6/7] gatt: Fix possibly lost block when bluetoothd exits

[Claudio Takahasi]

This patch fixes a possibly lost memory related to GDBusClient
and GDBusProxy objects when bluetoothd daemon exits and there
is registered external service.
==1503==    by 0x47ECFB: g_dbus_client_new (client.c:1232)
==1503==    by 0x461EC7: register_service (gatt-dbus.c:510)
---
 src/gatt-dbus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index 17c3359..1a0e55e 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -96,6 +96,11 @@ static gboolean external_service_destroy(void *user_data)
 	return FALSE;
 }
 
+static void external_service_free(void *user_data)
+{
+	external_service_destroy(user_data);
+}
+
 static void remove_service(DBusConnection *conn, void *user_data)
 {
 	struct external_service *esvc = user_data;
@@ -622,6 +627,8 @@ void gatt_dbus_manager_unregister(void)
 	g_hash_table_destroy(proxy_hash);
 	proxy_hash = NULL;
 
+	g_slist_free_full(external_services, external_service_free);
+
 	g_dbus_unregister_interface(btd_get_dbus_connection(), "/org/bluez",
 							GATT_MGR_IFACE);
 }
-- 
1.8.3.1

[PATCH BlueZ v0 7/7] doc: Add InvalidArguments to UnregisterService() errors

[Claudio Takahasi]

This patch adds "org.bluez.Error.InvalidArguments" as possible error
returned by GattManager1.UnregisterService().
---
 doc/gatt-api.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index deb519b..8c7975c 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -145,4 +145,5 @@ Methods		RegisterService(object service, dict options)
 			must match the same value that has been used
 			on registration.
 
-			Possible errors: org.bluez.Error.DoesNotExist
+			Possible errors: org.bluez.Error.InvalidArguments
+					 org.bluez.Error.DoesNotExist
-- 
1.8.3.1

[PATCH BlueZ v1 0/3] Add removing GATT services

[Claudio Takahasi]

This patchset implements GattManager1.UnregisterService(), and low-level
functions to remove service declarations (and its attributes) from the
attribute database.

Changes v1-v0:
 * Fix potential invalid memory access
 * Cleanup of external service when registration fails

Claudio Takahasi (3):
  gatt: Add removing service from the database
  gatt: Fix possibly lost block when bluetoothd exits
  doc: Add InvalidArguments to UnregisterService() errors

 doc/gatt-api.txt |  3 ++-
 src/gatt-dbus.c  | 51 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[PATCH BlueZ v1 1/3] gatt: Add removing service from the database

[Claudio Takahasi]

This patch removes the service declaration and its attributes when
the external service implementation leaves the system bus, calls
UnregisterService(), or RegisterService() fails.
---
 src/gatt-dbus.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index 3ded9cd..d3fd717 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -56,6 +56,7 @@ struct external_service {
 	DBusMessage *reg;
 	GDBusClient *client;
 	GSList *proxies;
+	struct btd_attribute *service;
 };
 
 struct proxy_write_data {
@@ -99,10 +100,11 @@ static void remove_service(DBusConnection *conn, void *user_data)
 {
 	struct external_service *esvc = user_data;
 
-	/* TODO: Remove from the database */
-
 	external_services = g_slist_remove(external_services, esvc);
 
+	if (esvc->service)
+		btd_gatt_remove_service(esvc->service);
+
 	/*
 	 * Do not run in the same loop, this may be a disconnect
 	 * watch call and GDBusClient should not be destroyed.
@@ -336,7 +338,7 @@ static void proxy_write_cb(struct btd_attribute *attr,
 
 }
 
-static int register_external_service(const struct external_service *esvc,
+static int register_external_service(struct external_service *esvc,
 							GDBusProxy *proxy)
 {
 	DBusMessageIter iter;
@@ -360,7 +362,8 @@ static int register_external_service(const struct external_service *esvc,
 	if (bt_string_to_uuid(&uuid, str) < 0)
 		return -EINVAL;
 
-	if (!btd_gatt_add_service(&uuid))
+	esvc->service = btd_gatt_add_service(&uuid);
+	if (!esvc->service)
 		return -EINVAL;
 
 	return 0;
@@ -481,18 +484,25 @@ static void client_ready(GDBusClient *client, void *user_data)
 	DBG("Added GATT service %s", esvc->path);
 
 	reply = dbus_message_new_method_return(esvc->reg);
-	goto reply;
+	g_dbus_send_message(conn, reply);
+
+	dbus_message_unref(esvc->reg);
+	esvc->reg = NULL;
+
+	return;
 
 fail:
 	error("Could not register external service: %s", esvc->path);
 
-	reply = btd_error_invalid_args(esvc->reg);
-	/* TODO: missing esvc/database cleanup */
+	/*
+	 * Set callback to NULL to avoid potential race condition
+	 * when calling remove_service and GDBusClient unref.
+	 */
+	g_dbus_client_set_disconnect_watch(esvc->client, NULL, NULL);
 
-reply:
-	dbus_message_unref(esvc->reg);
-	esvc->reg = NULL;
+	remove_service(conn, esvc);
 
+	reply = btd_error_invalid_args(esvc->reg);
 	g_dbus_send_message(conn, reply);
 }
 
@@ -578,6 +588,12 @@ static DBusMessage *unregister_service(DBusConnection *conn,
 	if (!strcmp(dbus_message_get_sender(msg), esvc->owner))
 		return btd_error_does_not_exist(msg);
 
+	/*
+	 * Set callback to NULL to avoid potential race condition
+	 * when calling remove_service and GDBusClient unref.
+	 */
+	g_dbus_client_set_disconnect_watch(esvc->client, NULL, NULL);
+
 	remove_service(conn, esvc);
 
 	return dbus_message_new_method_return(msg);
-- 
1.8.3.1

[PATCH BlueZ v1 2/3] gatt: Fix possibly lost block when bluetoothd exits

[Claudio Takahasi]

This patch fixes a possibly lost memory related to GDBusClient
and GDBusProxy objects when bluetoothd daemon exits and there
is registered external service.
==1503==    by 0x47ECFB: g_dbus_client_new (client.c:1232)
==1503==    by 0x461EC7: register_service (gatt-dbus.c:510)
---
 src/gatt-dbus.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index d3fd717..26437e7 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -96,6 +96,19 @@ static gboolean external_service_destroy(void *user_data)
 	return FALSE;
 }
 
+static void external_service_free(void *user_data)
+{
+	struct external_service *esvc = user_data;
+
+	/*
+	 * Set callback to NULL to avoid potential race condition
+	 * when calling remove_service and GDBusClient unref.
+	 */
+	g_dbus_client_set_disconnect_watch(esvc->client, NULL, NULL);
+
+	external_service_destroy(user_data);
+}
+
 static void remove_service(DBusConnection *conn, void *user_data)
 {
 	struct external_service *esvc = user_data;
@@ -634,6 +647,8 @@ void gatt_dbus_manager_unregister(void)
 	g_hash_table_destroy(proxy_hash);
 	proxy_hash = NULL;
 
+	g_slist_free_full(external_services, external_service_free);
+
 	g_dbus_unregister_interface(btd_get_dbus_connection(), "/org/bluez",
 							GATT_MGR_IFACE);
 }
-- 
1.8.3.1

[PATCH BlueZ v1 3/3] doc: Add InvalidArguments to UnregisterService() errors

[Claudio Takahasi]

This patch adds "org.bluez.Error.InvalidArguments" as possible error
returned by GattManager1.UnregisterService().
---
 doc/gatt-api.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index deb519b..8c7975c 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -145,4 +145,5 @@ Methods		RegisterService(object service, dict options)
 			must match the same value that has been used
 			on registration.
 
-			Possible errors: org.bluez.Error.DoesNotExist
+			Possible errors: org.bluez.Error.InvalidArguments
+					 org.bluez.Error.DoesNotExist
-- 
1.8.3.1

Re: [PATCH BlueZ v1 0/3] Add removing GATT services

[Johan Hedberg]

Hi Claudio,

On Thu, Apr 03, 2014, Claudio Takahasi wrote:
> This patchset implements GattManager1.UnregisterService(), and low-level
> functions to remove service declarations (and its attributes) from the
> attribute database.
> 
> Changes v1-v0:
>  * Fix potential invalid memory access
>  * Cleanup of external service when registration fails
> 
> Claudio Takahasi (3):
>   gatt: Add removing service from the database
>   gatt: Fix possibly lost block when bluetoothd exits
>   doc: Add InvalidArguments to UnregisterService() errors
> 
>  doc/gatt-api.txt |  3 ++-
>  src/gatt-dbus.c  | 51 +++++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 43 insertions(+), 11 deletions(-)

All three patches have been applied. Thanks.

Johan