[PATCH BlueZ v3 0/8] Implement doc/gatt-api.txt for client

[PATCH BlueZ v3 0/8] Implement doc/gatt-api.txt for client

[Arman Uguray]

*v3:
  - Fixed broken GATT service UUID management in its own patch.
  - Other small fixes.

Arman Uguray (8):
  shared/gatt-db: Add service getter by UUID
  core: device: Fix GATT profile probing
  core: device: Fix broken GATT UUID management
  profiles/gap: Fix probe/accept behavior.
  core: service: Remove GATT handle logic
  shared/gatt-db: Add "claimed" field to services
  core: gatt: Use "claimed" instead of "active"
  doc/gatt-api.txt: Update error names

 doc/gatt-api.txt     |  12 +-
 plugins/sixaxis.c    |   2 +-
 profiles/gap/gas.c   | 127 +++++++------
 src/adapter.c        |   8 +-
 src/device.c         | 498 +++++++++++++++++++++++++++++----------------------
 src/device.h         |   6 +-
 src/gatt-client.c    |   9 +-
 src/service.c        |  38 ----
 src/service.h        |   7 -
 src/shared/gatt-db.c |  46 +++++
 src/shared/gatt-db.h |   7 +
 11 files changed, 428 insertions(+), 332 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ v3 1/8] shared/gatt-db: Add service getter by UUID

[Arman Uguray]

Added gatt_db_get_service_with_uuid which checks if the database
contains a service with the given UUID and returns the first result.
---
 src/shared/gatt-db.c | 26 ++++++++++++++++++++++++++
 src/shared/gatt-db.h |  3 +++
 2 files changed, 29 insertions(+)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index e4a6f73..e8fb52c 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -1179,6 +1179,32 @@ struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
 	return service->attributes[handle - service_handle];
 }
 
+static bool find_service_with_uuid(const void *data, const void *user_data)
+{
+	const struct gatt_db_service *service = data;
+	const bt_uuid_t *uuid = user_data;
+	bt_uuid_t svc_uuid;
+
+	gatt_db_attribute_get_service_uuid(service->attributes[0], &svc_uuid);
+
+	return bt_uuid_cmp(uuid, &svc_uuid) == 0;
+}
+
+struct gatt_db_attribute *gatt_db_get_service_with_uuid(struct gatt_db *db,
+							const bt_uuid_t *uuid)
+{
+	struct gatt_db_service *service;
+
+	if (!db || !uuid)
+		return NULL;
+
+	service = queue_find(db->services, find_service_with_uuid, uuid);
+	if (!service)
+		return NULL;
+
+	return service->attributes[0];
+}
+
 const bt_uuid_t *gatt_db_attribute_get_type(
 					const struct gatt_db_attribute *attrib)
 {
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index 1f4005e..dc1b819 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -151,6 +151,9 @@ bool gatt_db_unregister(struct gatt_db *db, unsigned int id);
 struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
 							uint16_t handle);
 
+struct gatt_db_attribute *gatt_db_get_service_with_uuid(struct gatt_db *db,
+							const bt_uuid_t *uuid);
+
 const bt_uuid_t *gatt_db_attribute_get_type(
 					const struct gatt_db_attribute *attrib);
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ v3 2/8] core: device: Fix GATT profile probing

[Arman Uguray]

This patch fixes a recently introduced issue which changed the behavior
of profile probing for GATT-based profiles to create a btd_service
instance for all discovered GATT services to be on a per-UUID basis
rather than per-service.
---
 src/device.c | 182 +++++++++++++++++++++++------------------------------------
 1 file changed, 70 insertions(+), 112 deletions(-)

diff --git a/src/device.c b/src/device.c
index 75a5984..4f702ba 100644
--- a/src/device.c
+++ b/src/device.c
@@ -295,21 +295,15 @@ static GSList *find_service_with_state(GSList *list,
 	return NULL;
 }
 
-static GSList *find_service_with_gatt_handles(GSList *list,
-							uint16_t start_handle,
-							uint16_t end_handle)
+static GSList *find_service_with_uuid(GSList *list, char *uuid)
 {
 	GSList *l;
-	uint16_t svc_start, svc_end;
 
 	for (l = list; l != NULL; l = g_slist_next(l)) {
 		struct btd_service *service = l->data;
+		struct btd_profile *profile = btd_service_get_profile(service);
 
-		if (!btd_service_get_gatt_handles(service, &svc_start,
-								&svc_end))
-			continue;
-
-		if (svc_start == start_handle && svc_end == end_handle)
+		if (bt_uuid_strcmp(profile->remote_uuid, uuid) == 0)
 			return l;
 	}
 
@@ -2420,6 +2414,7 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data)
 static void device_add_uuids(struct btd_device *device, GSList *uuids)
 {
 	GSList *l;
+	bool changed = false;
 
 	for (l = uuids; l != NULL; l = g_slist_next(l)) {
 		GSList *match = g_slist_find_custom(device->uuids, l->data,
@@ -2427,12 +2422,14 @@ static void device_add_uuids(struct btd_device *device, GSList *uuids)
 		if (match)
 			continue;
 
+		changed = true;
 		device->uuids = g_slist_insert_sorted(device->uuids,
 						g_strdup(l->data),
 						bt_uuid_strcmp);
 	}
 
-	g_dbus_emit_property_changed(dbus_conn, device->path,
+	if (changed)
+		g_dbus_emit_property_changed(dbus_conn, device->path,
 						DEVICE_INTERFACE, "UUIDs");
 }
 
@@ -2444,8 +2441,6 @@ struct gatt_probe_data {
 	GSList *uuids;
 	struct gatt_db_attribute *cur_attr;
 	char cur_uuid[MAX_LEN_UUID_STR];
-	uint16_t start_handle, end_handle;
-	GSList *valid_services;
 };
 
 static bool device_match_profile(struct btd_device *device,
@@ -2473,8 +2468,7 @@ static void dev_probe_gatt(struct btd_profile *p, void *user_data)
 	if (!p->remote_uuid || bt_uuid_strcmp(p->remote_uuid, data->cur_uuid))
 		return;
 
-	service = service_create_gatt(data->dev, p, data->start_handle,
-							data->end_handle);
+	service = service_create(data->dev, p);
 	if (!service)
 		return;
 
@@ -2487,72 +2481,6 @@ static void dev_probe_gatt(struct btd_profile *p, void *user_data)
 	gatt_db_service_set_active(data->cur_attr, false);
 
 	data->dev->services = g_slist_append(data->dev->services, service);
-
-	if (data->all_services)
-		data->valid_services = g_slist_append(data->valid_services,
-								service);
-}
-
-static bool match_existing_service(struct gatt_probe_data *data)
-{
-	struct btd_device *dev = data->dev;
-	struct btd_service *service;
-	struct btd_profile *profile;
-	uint16_t start, end;
-	GSList *l, *tmp;
-
-	/*
-	 * Check if the profiles should be probed for the service in the
-	 * database.
-	 */
-	for (l = dev->services; l != NULL;) {
-		service = l->data;
-		profile = btd_service_get_profile(service);
-
-		/* If this is local or non-GATT based service, then skip. */
-		if (!profile->remote_uuid ||
-			!btd_service_get_gatt_handles(service, &start, &end)) {
-			l = g_slist_next(l);
-			continue;
-		}
-
-		/* Skip this service if the handle ranges don't overlap. */
-		if (start > data->end_handle || end < data->start_handle) {
-			l = g_slist_next(l);
-			continue;
-		}
-
-		/*
-		 * If there is an exact match, then there's no need to probe the
-		 * profiles. An exact match is when the service handles AND the
-		 * service UUID match.
-		 */
-		if (start == data->start_handle && end == data->end_handle &&
-			!bt_uuid_strcmp(profile->remote_uuid, data->cur_uuid)) {
-			if (data->all_services)
-				data->valid_services = g_slist_append(
-						data->valid_services, service);
-			return true;
-		}
-
-		/*
-		 * The handles overlap but there is no exact match. This means
-		 * that the service is no longer valid. Remove it.
-		 *
-		 * TODO: The following code is fairly inefficient, especially
-		 * when we consider all the extra searches that we're already
-		 * doing. Consider changing the services list to a GList.
-		 */
-		tmp = l->next;
-		dev->services = g_slist_delete_link(dev->services, l);
-		dev->pending = g_slist_remove(dev->pending, service);
-		service_remove(service);
-
-		l = tmp;
-	}
-
-	/* No match found */
-	return false;
 }
 
 static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
@@ -2562,22 +2490,27 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
 	bt_uuid_t uuid;
 	GSList *l = NULL;
 
-	gatt_db_attribute_get_service_data(attr, &data->start_handle,
-							&data->end_handle, NULL,
-							&uuid);
+	gatt_db_attribute_get_service_uuid(attr, &uuid);
 	bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid));
 
 	data->cur_attr = attr;
 
-	/* Don't probe the profiles if a matching service already exists. */
-	if (!match_existing_service(data))
-		btd_profile_foreach(dev_probe_gatt, data);
-
-	if (data->all_services) {
+	/*
+	 * If we're probing for all services, store the UUID since device->uuids
+	 * was cleared.
+	 */
+	if (data->all_services)
 		data->uuids = g_slist_append(data->uuids,
 						g_strdup(data->cur_uuid));
+
+	/* Don't probe the profiles if a matching service already exists. */
+	if (find_service_with_uuid(data->dev->services, data->cur_uuid))
+		return;
+
+	btd_profile_foreach(dev_probe_gatt, data);
+
+	if (data->all_services)
 		return;
-	}
 
 	l = g_slist_append(l, g_strdup(data->cur_uuid));
 	device_add_uuids(data->dev, l);
@@ -2600,12 +2533,15 @@ static void remove_invalid_services(struct gatt_probe_data *data)
 {
 	struct btd_device *dev = data->dev;
 	struct btd_service *service;
+	struct btd_profile *profile;
 	GSList *l, *tmp;
 
 	for (l = dev->services; l != NULL;) {
 		service = l->data;
+		profile = btd_service_get_profile(service);
 
-		if (g_slist_find(data->valid_services, service)) {
+		if (g_slist_find_custom(dev->uuids, profile->remote_uuid,
+							bt_uuid_strcmp)) {
 			l = g_slist_next(l);
 			continue;
 		}
@@ -2639,11 +2575,21 @@ static void device_probe_gatt_profiles(struct btd_device *device)
 
 	gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile,
 									&data);
+
+	/*
+	 * Clear the UUIDs list
+	 *
+	 * FIXME: The management of UUIDs here iss currently broken, as clearing
+	 * the entire list here will likely remove UUIDs that shouldn't be
+	 * removed (e.g. those obtained via SDP for a dual-mode device).
+	 */
+	g_slist_free_full(device->uuids, g_free);
+	device->uuids = NULL;
+
 	device_add_uuids(device, data.uuids);
 	g_slist_free_full(data.uuids, g_free);
 
 	remove_invalid_services(&data);
-	g_slist_free(data.valid_services);
 }
 
 static void device_accept_gatt_profiles(struct btd_device *device)
@@ -2657,13 +2603,15 @@ static void device_accept_gatt_profiles(struct btd_device *device)
 static void device_remove_gatt_profile(struct btd_device *device,
 						struct gatt_db_attribute *attr)
 {
-	uint16_t start, end;
 	struct btd_service *service;
+	bt_uuid_t uuid;
+	char uuid_str[MAX_LEN_UUID_STR];
 	GSList *l;
 
-	gatt_db_attribute_get_service_handles(attr, &start, &end);
+	gatt_db_attribute_get_service_uuid(attr, &uuid);
+	bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
 
-	l = find_service_with_gatt_handles(device->services, start, end);
+	l = find_service_with_uuid(device->services, uuid_str);
 	if (!l)
 		return;
 
@@ -2677,13 +2625,16 @@ static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data)
 {
 	struct btd_device *device = user_data;
 	GSList *new_service = NULL;
+	bt_uuid_t uuid;
+	char uuid_str[MAX_LEN_UUID_STR];
 	uint16_t start, end;
 	GSList *l;
 
 	if (!bt_gatt_client_is_ready(device->client))
 		return;
 
-	gatt_db_attribute_get_service_handles(attr, &start, &end);
+	gatt_db_attribute_get_service_data(attr, &start, &end, NULL, &uuid);
+	bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
 
 	DBG("start: 0x%04x, end: 0x%04x", start, end);
 
@@ -2695,14 +2646,19 @@ static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data)
 	if (!new_service)
 		return;
 
-	device_register_primaries(device, new_service, -1);
+	l = find_service_with_uuid(device->services, uuid_str);
 
-	device_probe_gatt_profile(device, attr);
+	device_register_primaries(device, new_service, -1);
 
-	l = find_service_with_gatt_handles(device->services, start, end);
-	if (l)
+	/*
+	 * If the profile was probed for the first time then call accept on
+	 * the service.
+	 */
+	if (!l && (l = find_service_with_uuid(device->services, uuid_str)))
 		service_accept(l->data);
 
+	device_probe_gatt_profile(device, attr);
+
 	store_device_info(device);
 
 	btd_gatt_client_service_added(device->client_dbus, attr);
@@ -2750,24 +2706,26 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
 	prim = l->data;
 	device->primaries = g_slist_delete_link(device->primaries, l);
 
-	/*
-	 * If this happend since the db was cleared for a non-bonded device,
-	 * then don't remove the btd_service just yet. We do this so that we can
-	 * avoid re-probing the profile if the same GATT service is found on the
-	 * device on re-connection. However, if the device is marked as
-	 * temporary, then we remove it anyway.
-	 */
-	if (device->client || device->temporary == TRUE)
-		device_remove_gatt_profile(device, attr);
+	/* Get the UUID entry to be removed below */
+	l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
 
 	/*
-	 * Remove the corresponding UUIDs entry, only if this is the last
-	 * service with this UUID.
+	 * Remove the corresponding UUIDs entry and the profile, only if this
+	 * is the last service with this UUID.
 	 */
-	l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
-
 	if (!g_slist_find_custom(device->primaries, prim->uuid,
 							prim_uuid_cmp)) {
+		/*
+		 * If this happened since the db was cleared for a non-bonded
+		 * device, then don't remove the btd_service just yet. We do
+		 * this so that we can avoid re-probing the profile if the same
+		 * GATT service is found on the device on re-connection.
+		 * However, if the device is marked as temporary, then we remove
+		 * it anyway.
+		 */
+		if (device->client || device->temporary == TRUE)
+			device_remove_gatt_profile(device, attr);
+
 		g_free(l->data);
 		device->uuids = g_slist_delete_link(device->uuids, l);
 		g_dbus_emit_property_changed(dbus_conn, device->path,
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ v3 3/8] core: device: Fix broken GATT UUID management

[Arman Uguray]

This patch fixes the broken GATT UUID management, which incorrectly
remove valid UUIDs during the GATT profile probe process. This is
achieved by splitting UUIDs obtained via GATT and SDP into their own
lists that are internal to btd_device.
---
 plugins/sixaxis.c |   2 +-
 src/adapter.c     |   8 +-
 src/device.c      | 367 +++++++++++++++++++++++++++++++++++-------------------
 src/device.h      |   6 +-
 4 files changed, 248 insertions(+), 135 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index ac53ba9..0a4e3d8 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -289,7 +289,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 
 	device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
 
-	if (g_slist_find_custom(btd_device_get_uuids(device), HID_UUID,
+	if (g_slist_find_custom(btd_device_get_sdp_uuids(device), HID_UUID,
 						(GCompareFunc)strcasecmp)) {
 		DBG("device %s already known, skipping", device_addr);
 		return true;
diff --git a/src/adapter.c b/src/adapter.c
index d7e2550..379a80b 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2912,9 +2912,13 @@ static void load_devices(struct btd_adapter *adapter)
 
 		/* TODO: register services from pre-loaded list of primaries */
 
-		list = btd_device_get_uuids(device);
+		list = btd_device_get_sdp_uuids(device);
 		if (list)
-			device_probe_profiles(device, list);
+			device_probe_profiles(device, list, false);
+
+		list = btd_device_get_gatt_uuids(device);
+		if (list)
+			device_probe_profiles(device, list, true);
 
 device_exist:
 		if (key_info) {
diff --git a/src/device.c b/src/device.c
index 4f702ba..9d1a480 100644
--- a/src/device.c
+++ b/src/device.c
@@ -193,7 +193,8 @@ struct btd_device {
 	uint16_t	appearance;
 	char		*modalias;
 	struct btd_adapter	*adapter;
-	GSList		*uuids;
+	GSList		*sdp_uuids;
+	GSList		*gatt_uuids;
 	GSList		*primaries;		/* List of primary services */
 	GSList		*services;		/* List of btd_service */
 	GSList		*pending;		/* Pending services */
@@ -335,6 +336,28 @@ static void update_technologies(GKeyFile *file, struct btd_device *dev)
 								list, len);
 }
 
+static char **store_service_uuids(GKeyFile *key_file, gchar *key, GSList *uuids)
+{
+	GSList *l;
+	char **array;
+	int i;
+
+	if (!uuids) {
+		g_key_file_remove_key(key_file, "General", key, NULL);
+		return NULL;
+	}
+
+	array = g_new0(char *, g_slist_length(uuids) + 1);
+
+	for (i = 0, l = uuids; l; l = g_slist_next(l), i++)
+		array[i] = l->data;
+
+	g_key_file_set_string_list(key_file, "General", key,
+						(const char **)array, i);
+
+	return array;
+}
+
 static gboolean store_device_info_cb(gpointer user_data)
 {
 	struct btd_device *device = user_data;
@@ -344,7 +367,7 @@ static gboolean store_device_info_cb(gpointer user_data)
 	char device_addr[18];
 	char *str;
 	char class[9];
-	char **uuids = NULL;
+	char **sdp_uuids, **gatt_uuids;
 	gsize length = 0;
 
 	device->store_id = 0;
@@ -387,18 +410,10 @@ static gboolean store_device_info_cb(gpointer user_data)
 	g_key_file_set_boolean(key_file, "General", "Blocked",
 							device->blocked);
 
-	if (device->uuids) {
-		GSList *l;
-		int i;
-
-		uuids = g_new0(char *, g_slist_length(device->uuids) + 1);
-		for (i = 0, l = device->uuids; l; l = g_slist_next(l), i++)
-			uuids[i] = l->data;
-		g_key_file_set_string_list(key_file, "General", "Services",
-						(const char **)uuids, i);
-	} else {
-		g_key_file_remove_key(key_file, "General", "Services", NULL);
-	}
+	sdp_uuids = store_service_uuids(key_file, "SDPServices",
+							device->sdp_uuids);
+	gatt_uuids = store_service_uuids(key_file, "GATTServices",
+							device->gatt_uuids);
 
 	if (device->vendor_src) {
 		g_key_file_set_integer(key_file, "DeviceID", "Source",
@@ -420,7 +435,8 @@ static gboolean store_device_info_cb(gpointer user_data)
 	g_free(str);
 
 	g_key_file_free(key_file);
-	g_free(uuids);
+	g_free(sdp_uuids);
+	g_free(gatt_uuids);
 
 	return FALSE;
 }
@@ -574,7 +590,8 @@ static void device_free(gpointer user_data)
 	btd_gatt_client_destroy(device->client_dbus);
 	device->client_dbus = NULL;
 
-	g_slist_free_full(device->uuids, g_free);
+	g_slist_free_full(device->sdp_uuids, g_free);
+	g_slist_free_full(device->gatt_uuids, g_free);
 	g_slist_free_full(device->primaries, g_free);
 	g_slist_free_full(device->attios, g_free);
 	g_slist_free_full(device->attios_offline, g_free);
@@ -988,22 +1005,43 @@ static gboolean dev_property_get_uuids(const GDBusPropertyTable *property,
 {
 	struct btd_device *dev = data;
 	DBusMessageIter entry;
-	GSList *l;
+	GHashTable *uuids = NULL;
+	GSList *sl;
+	GList *keys, *l;
 
 	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
 				DBUS_TYPE_STRING_AS_STRING, &entry);
 
-	if (dev->bredr_state.svc_resolved || dev->le_state.svc_resolved)
-		l = dev->uuids;
-	else if (dev->eir_uuids)
-		l = dev->eir_uuids;
-	else
-		l = dev->uuids;
+	if (!dev->bredr_state.svc_resolved && dev->le_state.svc_resolved &&
+							dev->eir_uuids) {
+		for (sl = dev->eir_uuids; sl; sl = g_slist_next(sl))
+			dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
+								&sl->data);
+
+		goto done;
+	}
+
+	/* Insert GATT and SDP UUIDs into a set to prevent any duplicates */
+	uuids = g_hash_table_new(g_str_hash, bt_uuid_strcmp);
 
-	for (; l != NULL; l = l->next)
+	for (sl = dev->sdp_uuids; sl; sl = g_slist_next(sl))
+		g_hash_table_add(uuids, sl->data);
+
+	for (sl = dev->gatt_uuids; sl; sl = g_slist_next(sl))
+		g_hash_table_add(uuids, sl->data);
+
+	/* Obtain a list of UUIDs and sort it */
+	keys = g_hash_table_get_keys(uuids);
+	keys = g_list_sort(keys, bt_uuid_strcmp);
+
+	for (l = keys; l; l = g_list_next(l))
 		dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
-							&l->data);
+								&l->data);
+
+	g_list_free(keys);
+	g_hash_table_destroy(uuids);
 
+done:
 	dbus_message_iter_close_container(iter, &entry);
 
 	return TRUE;
@@ -1130,7 +1168,8 @@ int device_unblock(struct btd_device *device, gboolean silent,
 	if (!silent) {
 		g_dbus_emit_property_changed(dbus_conn, device->path,
 						DEVICE_INTERFACE, "Blocked");
-		device_probe_profiles(device, device->uuids);
+		device_probe_profiles(device, device->sdp_uuids, false);
+		device_probe_profiles(device, device->gatt_uuids, true);
 	}
 
 	return 0;
@@ -2165,6 +2204,66 @@ failed:
 	return str;
 }
 
+static bool device_add_uuid(GSList **uuids, const char *uuid)
+{
+	if (g_slist_find_custom(*uuids, uuid, bt_uuid_strcmp))
+		return false;
+
+	*uuids = g_slist_insert_sorted(*uuids, g_strdup(uuid), bt_uuid_strcmp);
+
+	return true;
+}
+
+static bool device_add_uuids(struct btd_device *device, GSList **uuids,
+							GSList *new_uuids)
+{
+	GSList *l;
+	bool changed = false;
+
+	for (l = new_uuids; l; l = g_slist_next(l)) {
+		if (device_add_uuid(uuids, l->data))
+			changed = true;
+	}
+
+	if (changed)
+		g_dbus_emit_property_changed(dbus_conn, device->path,
+						DEVICE_INTERFACE, "UUIDs");
+
+	return changed;
+}
+
+static bool device_add_sdp_uuid(struct btd_device *device, const char *uuid)
+{
+	return device_add_uuid(&device->sdp_uuids, uuid);
+}
+
+static bool device_add_sdp_uuids(struct btd_device *device, GSList *uuids)
+{
+	return device_add_uuids(device, &device->sdp_uuids, uuids);
+}
+
+static bool device_add_gatt_uuid(struct btd_device *device, const char *uuid)
+{
+	return device_add_uuid(&device->gatt_uuids, uuid);
+}
+
+static bool device_add_gatt_uuids(struct btd_device *device, GSList *uuids)
+{
+	return device_add_uuids(device, &device->gatt_uuids, uuids);
+}
+
+static void device_remove_gatt_uuid(struct btd_device *device, const char *uuid)
+{
+	GSList *l;
+
+	l = g_slist_find_custom(device->gatt_uuids, uuid, bt_uuid_strcmp);
+	if (!l)
+		return;
+
+	g_free(l->data);
+	device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
+}
+
 static void load_info(struct btd_device *device, const char *local,
 			const char *peer, GKeyFile *key_file)
 {
@@ -2254,30 +2353,40 @@ next:
 	if (blocked)
 		device_block(device, FALSE);
 
-	/* Load device profile list */
-	uuids = g_key_file_get_string_list(key_file, "General", "Services",
-						NULL, NULL);
+	/* Load classic device profile list */
+	uuids = g_key_file_get_string_list(key_file, "General", "SDPServices",
+								NULL, NULL);
 	if (uuids) {
 		char **uuid;
 
-		for (uuid = uuids; *uuid; uuid++) {
-			GSList *match;
+		for (uuid = uuids; *uuid; uuid++)
+			device_add_sdp_uuid(device, *uuid);
 
-			match = g_slist_find_custom(device->uuids, *uuid,
-							bt_uuid_strcmp);
-			if (match)
-				continue;
-
-			device->uuids = g_slist_insert_sorted(device->uuids,
-								g_strdup(*uuid),
-								bt_uuid_strcmp);
-		}
 		g_strfreev(uuids);
 
 		/* Discovered services restored from storage */
 		device->bredr_state.svc_resolved = true;
 	}
 
+	/* Load GATT-based device profile list */
+	uuids = g_key_file_get_string_list(key_file, "General", "GATTServices",
+								NULL, NULL);
+	if (uuids) {
+		char **uuid;
+
+		for (uuid = uuids; *uuid; uuid++)
+			device_add_gatt_uuid(device, *uuid);
+
+		g_strfreev(uuids);
+
+		/*
+		 * TODO: The GATT-based service UUIDs have been restored from
+		 * storage but we don't have a populated gatt-db yet. Here we
+		 * should mark le_state.svc_resolved as true, if we're bonded
+		 * and we have a populated gatt_db.
+		 */
+	}
+
 	/* Load device id */
 	source = g_key_file_get_integer(key_file, "DeviceID", "Source", NULL);
 	if (source) {
@@ -2411,34 +2520,13 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data)
 	*new_services = g_slist_append(*new_services, prim);
 }
 
-static void device_add_uuids(struct btd_device *device, GSList *uuids)
-{
-	GSList *l;
-	bool changed = false;
-
-	for (l = uuids; l != NULL; l = g_slist_next(l)) {
-		GSList *match = g_slist_find_custom(device->uuids, l->data,
-							bt_uuid_strcmp);
-		if (match)
-			continue;
-
-		changed = true;
-		device->uuids = g_slist_insert_sorted(device->uuids,
-						g_strdup(l->data),
-						bt_uuid_strcmp);
-	}
-
-	if (changed)
-		g_dbus_emit_property_changed(dbus_conn, device->path,
-						DEVICE_INTERFACE, "UUIDs");
-}
-
 static void store_services(struct btd_device *device);
 
 struct gatt_probe_data {
 	struct btd_device *dev;
 	bool all_services;
-	GSList *uuids;
+	GSList *new_uuids;
+	GSList *old_uuids;
 	struct gatt_db_attribute *cur_attr;
 	char cur_uuid[MAX_LEN_UUID_STR];
 };
@@ -2488,20 +2576,31 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
 {
 	struct gatt_probe_data *data = user_data;
 	bt_uuid_t uuid;
-	GSList *l = NULL;
+	gpointer dup_uuid;
 
 	gatt_db_attribute_get_service_uuid(attr, &uuid);
 	bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid));
 
 	data->cur_attr = attr;
 
-	/*
-	 * If we're probing for all services, store the UUID since device->uuids
-	 * was cleared.
-	 */
-	if (data->all_services)
-		data->uuids = g_slist_append(data->uuids,
+	if (data->all_services) {
+		GSList *l;
+
+		/*
+		 * Check if the service is already in the old UUIDs list. If so,
+		 * remove it.
+		 */
+		l = g_slist_find_custom(data->old_uuids, data->cur_uuid,
+								bt_uuid_strcmp);
+		if (l) {
+			g_free(l->data);
+			data->old_uuids = g_slist_delete_link(data->old_uuids,
+									l);
+		} else {
+			data->new_uuids = g_slist_append(data->new_uuids,
 						g_strdup(data->cur_uuid));
+		}
+	}
 
 	/* Don't probe the profiles if a matching service already exists. */
 	if (find_service_with_uuid(data->dev->services, data->cur_uuid))
@@ -2512,8 +2611,14 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
 	if (data->all_services)
 		return;
 
-	l = g_slist_append(l, g_strdup(data->cur_uuid));
-	device_add_uuids(data->dev, l);
+	dup_uuid = g_strdup(data->cur_uuid);
+	if (!device_add_gatt_uuid(data->dev, dup_uuid)) {
+		g_free(dup_uuid);
+		return;
+	}
+
+	g_dbus_emit_property_changed(dbus_conn, data->dev->path,
+						DEVICE_INTERFACE, "UUIDs");
 }
 
 static void device_probe_gatt_profile(struct btd_device *device,
@@ -2526,33 +2631,25 @@ static void device_probe_gatt_profile(struct btd_device *device,
 	data.dev = device;
 
 	dev_probe_gatt_profile(attr, &data);
-	g_slist_free_full(data.uuids, g_free);
 }
 
 static void remove_invalid_services(struct gatt_probe_data *data)
 {
 	struct btd_device *dev = data->dev;
 	struct btd_service *service;
-	struct btd_profile *profile;
-	GSList *l, *tmp;
+	GSList *l, *svc;
 
-	for (l = dev->services; l != NULL;) {
-		service = l->data;
-		profile = btd_service_get_profile(service);
+	for (l = data->old_uuids; l; l = g_slist_next(l)) {
+		device_remove_gatt_uuid(dev, l->data);
 
-		if (g_slist_find_custom(dev->uuids, profile->remote_uuid,
-							bt_uuid_strcmp)) {
-			l = g_slist_next(l);
+		svc = find_service_with_uuid(dev->services, l->data);
+		if (!svc)
 			continue;
-		}
 
-		/* Service no longer valid, so remove it */
-		tmp = l->next;
-		dev->services = g_slist_delete_link(dev->services, l);
+		service = svc->data;
+		dev->services = g_slist_delete_link(dev->services, svc);
 		dev->pending = g_slist_remove(dev->pending, service);
 		service_remove(service);
-
-		l = tmp;
 	}
 }
 
@@ -2560,6 +2657,7 @@ static void device_probe_gatt_profiles(struct btd_device *device)
 {
 	struct gatt_probe_data data;
 	char addr[18];
+	GSList *l;
 
 	ba2str(&device->bdaddr, addr);
 
@@ -2573,23 +2671,24 @@ static void device_probe_gatt_profiles(struct btd_device *device)
 	data.dev = device;
 	data.all_services = true;
 
+	/* Copy current list of GATT UUIDs */
+	for (l = device->gatt_uuids; l; l = g_slist_next(l))
+		data.old_uuids = g_slist_append(data.old_uuids,
+							g_strdup(l->data));
+
 	gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile,
 									&data);
 
-	/*
-	 * Clear the UUIDs list
-	 *
-	 * FIXME: The management of UUIDs here iss currently broken, as clearing
-	 * the entire list here will likely remove UUIDs that shouldn't be
-	 * removed (e.g. those obtained via SDP for a dual-mode device).
-	 */
-	g_slist_free_full(device->uuids, g_free);
-	device->uuids = NULL;
+	/* Whatever remains in data.old_uuids is stale and should be removed */
+	remove_invalid_services(&data);
 
-	device_add_uuids(device, data.uuids);
-	g_slist_free_full(data.uuids, g_free);
+	/* Update the list with new UUIDs */
+	if (!device_add_gatt_uuids(device, data.new_uuids) && data.old_uuids)
+		g_dbus_emit_property_changed(dbus_conn, device->path,
+						DEVICE_INTERFACE, "UUIDs");
 
-	remove_invalid_services(&data);
+	g_slist_free_full(data.new_uuids, g_free);
+	g_slist_free_full(data.old_uuids, g_free);
 }
 
 static void device_accept_gatt_profiles(struct btd_device *device)
@@ -2675,46 +2774,39 @@ static gint prim_attr_cmp(gconstpointer a, gconstpointer b)
 	return !(prim->range.start == start && prim->range.end == end);
 }
 
-static gint prim_uuid_cmp(gconstpointer a, gconstpointer b)
-{
-	const struct gatt_primary *prim = a;
-	const char *uuid = b;
-
-	return bt_uuid_strcmp(prim->uuid, uuid);
-}
-
 static void gatt_service_removed(struct gatt_db_attribute *attr,
 								void *user_data)
 {
 	struct btd_device *device = user_data;
 	GSList *l;
-	struct gatt_primary *prim;
 	uint16_t start, end;
+	bt_uuid_t uuid;
+	char uuid_str[MAX_LEN_UUID_STR];
 
 	if (!bt_gatt_client_is_ready(device->client))
 		return;
 
-	gatt_db_attribute_get_service_handles(attr, &start, &end);
+	gatt_db_attribute_get_service_data(attr, &start, &end, NULL, &uuid);
 
 	DBG("start: 0x%04x, end: 0x%04x", start, end);
 
 	/* Remove the corresponding gatt_primary */
 	l = g_slist_find_custom(device->primaries, attr, prim_attr_cmp);
-	if (!l)
-		return;
+	if (l) {
+		g_free(l->data);
+		device->primaries = g_slist_delete_link(device->primaries, l);
+	}
 
-	prim = l->data;
-	device->primaries = g_slist_delete_link(device->primaries, l);
+	bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
 
 	/* Get the UUID entry to be removed below */
-	l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
+	l = g_slist_find_custom(device->gatt_uuids, uuid_str, bt_uuid_strcmp);
 
 	/*
 	 * Remove the corresponding UUIDs entry and the profile, only if this
 	 * is the last service with this UUID.
 	 */
-	if (!g_slist_find_custom(device->primaries, prim->uuid,
-							prim_uuid_cmp)) {
+	if (!gatt_db_get_service_with_uuid(device->db, &uuid)) {
 		/*
 		 * If this happened since the db was cleared for a non-bonded
 		 * device, then don't remove the btd_service just yet. We do
@@ -2727,13 +2819,11 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
 			device_remove_gatt_profile(device, attr);
 
 		g_free(l->data);
-		device->uuids = g_slist_delete_link(device->uuids, l);
+		device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
 		g_dbus_emit_property_changed(dbus_conn, device->path,
 						DEVICE_INTERFACE, "UUIDs");
 	}
 
-	g_free(prim);
-
 	store_device_info(device);
 
 	btd_gatt_client_service_removed(device->client_dbus, attr);
@@ -2988,8 +3078,13 @@ void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup)
 	dev->trusted = dup->trusted;
 	dev->blocked = dup->blocked;
 
-	for (l = dup->uuids; l; l = g_slist_next(l))
-		dev->uuids = g_slist_append(dev->uuids, g_strdup(l->data));
+	for (l = dup->sdp_uuids; l; l = g_slist_next(l))
+		dev->sdp_uuids = g_slist_append(dev->sdp_uuids,
+							g_strdup(l->data));
+
+	for (l = dup->gatt_uuids; l; l = g_slist_next(l))
+		dev->gatt_uuids = g_slist_append(dev->gatt_uuids,
+							g_strdup(l->data));
 
 	if (dev->name[0] == '\0')
 		strcpy(dev->name, dup->name);
@@ -3240,9 +3335,14 @@ static gboolean record_has_uuid(const sdp_record_t *rec,
 	return FALSE;
 }
 
-GSList *btd_device_get_uuids(struct btd_device *device)
+GSList *btd_device_get_sdp_uuids(struct btd_device *device)
 {
-	return device->uuids;
+	return device->sdp_uuids;
+}
+
+GSList *btd_device_get_gatt_uuids(struct btd_device *device)
+{
+	return device->gatt_uuids;
 }
 
 struct probe_data {
@@ -3280,7 +3380,8 @@ void device_probe_profile(gpointer a, gpointer b)
 	if (profile->device_probe == NULL)
 		return;
 
-	if (!device_match_profile(device, profile, device->uuids))
+	if (!device_match_profile(device, profile, device->sdp_uuids) &&
+		!device_match_profile(device, profile, device->gatt_uuids))
 		return;
 
 	service = service_create(device, profile);
@@ -3318,7 +3419,8 @@ void device_remove_profile(gpointer a, gpointer b)
 	service_remove(service);
 }
 
-void device_probe_profiles(struct btd_device *device, GSList *uuids)
+void device_probe_profiles(struct btd_device *device, GSList *uuids,
+								bool gatt)
 {
 	struct probe_data d = { device, uuids };
 	char addr[18];
@@ -3335,7 +3437,10 @@ void device_probe_profiles(struct btd_device *device, GSList *uuids)
 	btd_profile_foreach(dev_probe, &d);
 
 add_uuids:
-	device_add_uuids(device, uuids);
+	if (gatt)
+		device_add_gatt_uuids(device, uuids);
+	else
+		device_add_sdp_uuids(device, uuids);
 }
 
 static void store_sdp_record(GKeyFile *key_file, sdp_record_t *rec)
@@ -3431,7 +3536,7 @@ static int update_record(struct browse_req *req, const char *uuid,
 	req->records = sdp_list_append(req->records, sdp_copy_record(rec));
 
 	/* Check if UUID is duplicated */
-	l = g_slist_find_custom(req->device->uuids, uuid, bt_uuid_strcmp);
+	l = g_slist_find_custom(req->device->sdp_uuids, uuid, bt_uuid_strcmp);
 	if (l == NULL) {
 		l = g_slist_find_custom(req->profiles_added, uuid,
 							bt_uuid_strcmp);
@@ -3662,7 +3767,7 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
 	 * the full list of services and populate a client-role gatt_db over
 	 * BR/EDR.
 	 */
-	device_probe_profiles(device, req->profiles_added);
+	device_probe_profiles(device, req->profiles_added, false);
 
 	/* Propagate services changes */
 	g_dbus_emit_property_changed(dbus_conn, req->device->path,
@@ -3927,6 +4032,8 @@ static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
 
 	if (device->browse)
 		register_gatt_services(device->browse);
+	else
+		device_probe_gatt_profiles(device);
 
 	device_accept_gatt_profiles(device);
 
@@ -5127,13 +5234,13 @@ void btd_device_add_uuid(struct btd_device *device, const char *uuid)
 	GSList *uuid_list;
 	char *new_uuid;
 
-	if (g_slist_find_custom(device->uuids, uuid, bt_uuid_strcmp))
+	if (g_slist_find_custom(device->sdp_uuids, uuid, bt_uuid_strcmp))
 		return;
 
 	new_uuid = g_strdup(uuid);
 	uuid_list = g_slist_append(NULL, new_uuid);
 
-	device_probe_profiles(device, uuid_list);
+	device_probe_profiles(device, uuid_list, false);
 
 	g_free(new_uuid);
 	g_slist_free(uuid_list);
diff --git a/src/device.h b/src/device.h
index a7fefee..f250bfa 100644
--- a/src/device.h
+++ b/src/device.h
@@ -60,8 +60,10 @@ struct device_addr_type {
 };
 
 int device_addr_type_cmp(gconstpointer a, gconstpointer b);
-GSList *btd_device_get_uuids(struct btd_device *device);
-void device_probe_profiles(struct btd_device *device, GSList *profiles);
+GSList *btd_device_get_sdp_uuids(struct btd_device *device);
+GSList *btd_device_get_gatt_uuids(struct btd_device *device);
+void device_probe_profiles(struct btd_device *device, GSList *profiles,
+								bool gatt);
 const sdp_record_t *btd_device_get_record(struct btd_device *device,
 						const char *uuid);
 struct gatt_primary *btd_device_get_primary(struct btd_device *device,
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH BlueZ v3 3/8] core: device: Fix broken GATT UUID management

[Luiz Augusto von Dentz]

Hi Arman,

On Wed, Jan 14, 2015 at 1:31 AM, Arman Uguray <armansito@chromium.org> wrote:
> This patch fixes the broken GATT UUID management, which incorrectly
> remove valid UUIDs during the GATT profile probe process. This is
> achieved by splitting UUIDs obtained via GATT and SDP into their own
> lists that are internal to btd_device.
> ---
>  plugins/sixaxis.c |   2 +-
>  src/adapter.c     |   8 +-
>  src/device.c      | 367 +++++++++++++++++++++++++++++++++++-------------------
>  src/device.h      |   6 +-
>  4 files changed, 248 insertions(+), 135 deletions(-)
>
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index ac53ba9..0a4e3d8 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -289,7 +289,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>
>         device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
>
> -       if (g_slist_find_custom(btd_device_get_uuids(device), HID_UUID,
> +       if (g_slist_find_custom(btd_device_get_sdp_uuids(device), HID_UUID,
>                                                 (GCompareFunc)strcasecmp)) {
>                 DBG("device %s already known, skipping", device_addr);
>                 return true;
> diff --git a/src/adapter.c b/src/adapter.c
> index d7e2550..379a80b 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -2912,9 +2912,13 @@ static void load_devices(struct btd_adapter *adapter)
>
>                 /* TODO: register services from pre-loaded list of primaries */
>
> -               list = btd_device_get_uuids(device);
> +               list = btd_device_get_sdp_uuids(device);
>                 if (list)
> -                       device_probe_profiles(device, list);
> +                       device_probe_profiles(device, list, false);
> +
> +               list = btd_device_get_gatt_uuids(device);
> +               if (list)
> +                       device_probe_profiles(device, list, true);
>
>  device_exist:
>                 if (key_info) {
> diff --git a/src/device.c b/src/device.c
> index 4f702ba..9d1a480 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -193,7 +193,8 @@ struct btd_device {
>         uint16_t        appearance;
>         char            *modalias;
>         struct btd_adapter      *adapter;
> -       GSList          *uuids;
> +       GSList          *sdp_uuids;
> +       GSList          *gatt_uuids;
>         GSList          *primaries;             /* List of primary services */
>         GSList          *services;              /* List of btd_service */
>         GSList          *pending;               /* Pending services */
> @@ -335,6 +336,28 @@ static void update_technologies(GKeyFile *file, struct btd_device *dev)
>                                                                 list, len);
>  }
>
> +static char **store_service_uuids(GKeyFile *key_file, gchar *key, GSList *uuids)
> +{
> +       GSList *l;
> +       char **array;
> +       int i;
> +
> +       if (!uuids) {
> +               g_key_file_remove_key(key_file, "General", key, NULL);
> +               return NULL;
> +       }
> +
> +       array = g_new0(char *, g_slist_length(uuids) + 1);
> +
> +       for (i = 0, l = uuids; l; l = g_slist_next(l), i++)
> +               array[i] = l->data;
> +
> +       g_key_file_set_string_list(key_file, "General", key,
> +                                               (const char **)array, i);
> +
> +       return array;
> +}
> +
>  static gboolean store_device_info_cb(gpointer user_data)
>  {
>         struct btd_device *device = user_data;
> @@ -344,7 +367,7 @@ static gboolean store_device_info_cb(gpointer user_data)
>         char device_addr[18];
>         char *str;
>         char class[9];
> -       char **uuids = NULL;
> +       char **sdp_uuids, **gatt_uuids;
>         gsize length = 0;
>
>         device->store_id = 0;
> @@ -387,18 +410,10 @@ static gboolean store_device_info_cb(gpointer user_data)
>         g_key_file_set_boolean(key_file, "General", "Blocked",
>                                                         device->blocked);
>
> -       if (device->uuids) {
> -               GSList *l;
> -               int i;
> -
> -               uuids = g_new0(char *, g_slist_length(device->uuids) + 1);
> -               for (i = 0, l = device->uuids; l; l = g_slist_next(l), i++)
> -                       uuids[i] = l->data;
> -               g_key_file_set_string_list(key_file, "General", "Services",
> -                                               (const char **)uuids, i);
> -       } else {
> -               g_key_file_remove_key(key_file, "General", "Services", NULL);
> -       }
> +       sdp_uuids = store_service_uuids(key_file, "SDPServices",
> +                                                       device->sdp_uuids);
> +       gatt_uuids = store_service_uuids(key_file, "GATTServices",
> +                                                       device->gatt_uuids);

This change is not backward compatible which mean existing storage
will not load properly if we change the storage fields, furthermore I
think it is better to store the UUIDs per bearer_state since service
discovered flag is stored there it seem logical to couple UUIDs founds
there as well.

>         if (device->vendor_src) {
>                 g_key_file_set_integer(key_file, "DeviceID", "Source",
> @@ -420,7 +435,8 @@ static gboolean store_device_info_cb(gpointer user_data)
>         g_free(str);
>
>         g_key_file_free(key_file);
> -       g_free(uuids);
> +       g_free(sdp_uuids);
> +       g_free(gatt_uuids);
>
>         return FALSE;
>  }
> @@ -574,7 +590,8 @@ static void device_free(gpointer user_data)
>         btd_gatt_client_destroy(device->client_dbus);
>         device->client_dbus = NULL;
>
> -       g_slist_free_full(device->uuids, g_free);
> +       g_slist_free_full(device->sdp_uuids, g_free);
> +       g_slist_free_full(device->gatt_uuids, g_free);
>         g_slist_free_full(device->primaries, g_free);
>         g_slist_free_full(device->attios, g_free);
>         g_slist_free_full(device->attios_offline, g_free);
> @@ -988,22 +1005,43 @@ static gboolean dev_property_get_uuids(const GDBusPropertyTable *property,
>  {
>         struct btd_device *dev = data;
>         DBusMessageIter entry;
> -       GSList *l;
> +       GHashTable *uuids = NULL;
> +       GSList *sl;
> +       GList *keys, *l;
>
>         dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>                                 DBUS_TYPE_STRING_AS_STRING, &entry);
>
> -       if (dev->bredr_state.svc_resolved || dev->le_state.svc_resolved)
> -               l = dev->uuids;
> -       else if (dev->eir_uuids)
> -               l = dev->eir_uuids;
> -       else
> -               l = dev->uuids;
> +       if (!dev->bredr_state.svc_resolved && dev->le_state.svc_resolved &&
> +                                                       dev->eir_uuids) {
> +               for (sl = dev->eir_uuids; sl; sl = g_slist_next(sl))
> +                       dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
> +                                                               &sl->data);
> +
> +               goto done;
> +       }
> +
> +       /* Insert GATT and SDP UUIDs into a set to prevent any duplicates */
> +       uuids = g_hash_table_new(g_str_hash, bt_uuid_strcmp);
>
> -       for (; l != NULL; l = l->next)
> +       for (sl = dev->sdp_uuids; sl; sl = g_slist_next(sl))
> +               g_hash_table_add(uuids, sl->data);
> +
> +       for (sl = dev->gatt_uuids; sl; sl = g_slist_next(sl))
> +               g_hash_table_add(uuids, sl->data);
> +
> +       /* Obtain a list of UUIDs and sort it */
> +       keys = g_hash_table_get_keys(uuids);
> +       keys = g_list_sort(keys, bt_uuid_strcmp);
> +
> +       for (l = keys; l; l = g_list_next(l))
>                 dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
> -                                                       &l->data);
> +                                                               &l->data);
> +
> +       g_list_free(keys);
> +       g_hash_table_destroy(uuids);
>
> +done:
>         dbus_message_iter_close_container(iter, &entry);
>
>         return TRUE;
> @@ -1130,7 +1168,8 @@ int device_unblock(struct btd_device *device, gboolean silent,
>         if (!silent) {
>                 g_dbus_emit_property_changed(dbus_conn, device->path,
>                                                 DEVICE_INTERFACE, "Blocked");
> -               device_probe_profiles(device, device->uuids);
> +               device_probe_profiles(device, device->sdp_uuids, false);
> +               device_probe_profiles(device, device->gatt_uuids, true);
>         }
>
>         return 0;
> @@ -2165,6 +2204,66 @@ failed:
>         return str;
>  }
>
> +static bool device_add_uuid(GSList **uuids, const char *uuid)
> +{
> +       if (g_slist_find_custom(*uuids, uuid, bt_uuid_strcmp))
> +               return false;
> +
> +       *uuids = g_slist_insert_sorted(*uuids, g_strdup(uuid), bt_uuid_strcmp);
> +
> +       return true;
> +}
> +
> +static bool device_add_uuids(struct btd_device *device, GSList **uuids,
> +                                                       GSList *new_uuids)
> +{
> +       GSList *l;
> +       bool changed = false;
> +
> +       for (l = new_uuids; l; l = g_slist_next(l)) {
> +               if (device_add_uuid(uuids, l->data))
> +                       changed = true;
> +       }
> +
> +       if (changed)
> +               g_dbus_emit_property_changed(dbus_conn, device->path,
> +                                               DEVICE_INTERFACE, "UUIDs");
> +
> +       return changed;
> +}
> +
> +static bool device_add_sdp_uuid(struct btd_device *device, const char *uuid)
> +{
> +       return device_add_uuid(&device->sdp_uuids, uuid);
> +}
> +
> +static bool device_add_sdp_uuids(struct btd_device *device, GSList *uuids)
> +{
> +       return device_add_uuids(device, &device->sdp_uuids, uuids);
> +}
> +
> +static bool device_add_gatt_uuid(struct btd_device *device, const char *uuid)
> +{
> +       return device_add_uuid(&device->gatt_uuids, uuid);
> +}
> +
> +static bool device_add_gatt_uuids(struct btd_device *device, GSList *uuids)
> +{
> +       return device_add_uuids(device, &device->gatt_uuids, uuids);
> +}
> +
> +static void device_remove_gatt_uuid(struct btd_device *device, const char *uuid)
> +{
> +       GSList *l;
> +
> +       l = g_slist_find_custom(device->gatt_uuids, uuid, bt_uuid_strcmp);
> +       if (!l)
> +               return;
> +
> +       g_free(l->data);
> +       device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
> +}
> +
>  static void load_info(struct btd_device *device, const char *local,
>                         const char *peer, GKeyFile *key_file)
>  {
> @@ -2254,30 +2353,40 @@ next:
>         if (blocked)
>                 device_block(device, FALSE);
>
> -       /* Load device profile list */
> -       uuids = g_key_file_get_string_list(key_file, "General", "Services",
> -                                               NULL, NULL);
> +       /* Load classic device profile list */
> +       uuids = g_key_file_get_string_list(key_file, "General", "SDPServices",
> +                                                               NULL, NULL);
>         if (uuids) {
>                 char **uuid;
>
> -               for (uuid = uuids; *uuid; uuid++) {
> -                       GSList *match;
> +               for (uuid = uuids; *uuid; uuid++)
> +                       device_add_sdp_uuid(device, *uuid);
>
> -                       match = g_slist_find_custom(device->uuids, *uuid,
> -                                                       bt_uuid_strcmp);
> -                       if (match)
> -                               continue;
> -
> -                       device->uuids = g_slist_insert_sorted(device->uuids,
> -                                                               g_strdup(*uuid),
> -                                                               bt_uuid_strcmp);
> -               }
>                 g_strfreev(uuids);
>
>                 /* Discovered services restored from storage */
>                 device->bredr_state.svc_resolved = true;
>         }
>
> +       /* Load GATT-based device profile list */
> +       uuids = g_key_file_get_string_list(key_file, "General", "GATTServices",
> +                                                               NULL, NULL);
> +       if (uuids) {
> +               char **uuid;
> +
> +               for (uuid = uuids; *uuid; uuid++)
> +                       device_add_gatt_uuid(device, *uuid);
> +
> +               g_strfreev(uuids);
> +
> +               /*
> +                * TODO: The GATT-based service UUIDs have been restored from
> +                * storage but we don't have a populated gatt-db yet. Here we
> +                * should mark le_state.svc_resolved as true, if we're bonded
> +                * and we have a populated gatt_db.
> +                */
> +       }
> +
>         /* Load device id */
>         source = g_key_file_get_integer(key_file, "DeviceID", "Source", NULL);
>         if (source) {
> @@ -2411,34 +2520,13 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data)
>         *new_services = g_slist_append(*new_services, prim);
>  }
>
> -static void device_add_uuids(struct btd_device *device, GSList *uuids)
> -{
> -       GSList *l;
> -       bool changed = false;
> -
> -       for (l = uuids; l != NULL; l = g_slist_next(l)) {
> -               GSList *match = g_slist_find_custom(device->uuids, l->data,
> -                                                       bt_uuid_strcmp);
> -               if (match)
> -                       continue;
> -
> -               changed = true;
> -               device->uuids = g_slist_insert_sorted(device->uuids,
> -                                               g_strdup(l->data),
> -                                               bt_uuid_strcmp);
> -       }
> -
> -       if (changed)
> -               g_dbus_emit_property_changed(dbus_conn, device->path,
> -                                               DEVICE_INTERFACE, "UUIDs");
> -}
> -
>  static void store_services(struct btd_device *device);
>
>  struct gatt_probe_data {
>         struct btd_device *dev;
>         bool all_services;
> -       GSList *uuids;
> +       GSList *new_uuids;
> +       GSList *old_uuids;
>         struct gatt_db_attribute *cur_attr;
>         char cur_uuid[MAX_LEN_UUID_STR];
>  };
> @@ -2488,20 +2576,31 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>  {
>         struct gatt_probe_data *data = user_data;
>         bt_uuid_t uuid;
> -       GSList *l = NULL;
> +       gpointer dup_uuid;
>
>         gatt_db_attribute_get_service_uuid(attr, &uuid);
>         bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid));
>
>         data->cur_attr = attr;
>
> -       /*
> -        * If we're probing for all services, store the UUID since device->uuids
> -        * was cleared.
> -        */
> -       if (data->all_services)
> -               data->uuids = g_slist_append(data->uuids,
> +       if (data->all_services) {
> +               GSList *l;
> +
> +               /*
> +                * Check if the service is already in the old UUIDs list. If so,
> +                * remove it.
> +                */
> +               l = g_slist_find_custom(data->old_uuids, data->cur_uuid,
> +                                                               bt_uuid_strcmp);
> +               if (l) {
> +                       g_free(l->data);
> +                       data->old_uuids = g_slist_delete_link(data->old_uuids,
> +                                                                       l);
> +               } else {
> +                       data->new_uuids = g_slist_append(data->new_uuids,
>                                                 g_strdup(data->cur_uuid));
> +               }
> +       }
>
>         /* Don't probe the profiles if a matching service already exists. */
>         if (find_service_with_uuid(data->dev->services, data->cur_uuid))
> @@ -2512,8 +2611,14 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>         if (data->all_services)
>                 return;
>
> -       l = g_slist_append(l, g_strdup(data->cur_uuid));
> -       device_add_uuids(data->dev, l);
> +       dup_uuid = g_strdup(data->cur_uuid);
> +       if (!device_add_gatt_uuid(data->dev, dup_uuid)) {
> +               g_free(dup_uuid);
> +               return;
> +       }
> +
> +       g_dbus_emit_property_changed(dbus_conn, data->dev->path,
> +                                               DEVICE_INTERFACE, "UUIDs");
>  }
>
>  static void device_probe_gatt_profile(struct btd_device *device,
> @@ -2526,33 +2631,25 @@ static void device_probe_gatt_profile(struct btd_device *device,
>         data.dev = device;
>
>         dev_probe_gatt_profile(attr, &data);
> -       g_slist_free_full(data.uuids, g_free);
>  }
>
>  static void remove_invalid_services(struct gatt_probe_data *data)
>  {
>         struct btd_device *dev = data->dev;
>         struct btd_service *service;
> -       struct btd_profile *profile;
> -       GSList *l, *tmp;
> +       GSList *l, *svc;
>
> -       for (l = dev->services; l != NULL;) {
> -               service = l->data;
> -               profile = btd_service_get_profile(service);
> +       for (l = data->old_uuids; l; l = g_slist_next(l)) {
> +               device_remove_gatt_uuid(dev, l->data);
>
> -               if (g_slist_find_custom(dev->uuids, profile->remote_uuid,
> -                                                       bt_uuid_strcmp)) {
> -                       l = g_slist_next(l);
> +               svc = find_service_with_uuid(dev->services, l->data);
> +               if (!svc)
>                         continue;
> -               }
>
> -               /* Service no longer valid, so remove it */
> -               tmp = l->next;
> -               dev->services = g_slist_delete_link(dev->services, l);
> +               service = svc->data;
> +               dev->services = g_slist_delete_link(dev->services, svc);
>                 dev->pending = g_slist_remove(dev->pending, service);
>                 service_remove(service);
> -
> -               l = tmp;
>         }
>  }
>
> @@ -2560,6 +2657,7 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>  {
>         struct gatt_probe_data data;
>         char addr[18];
> +       GSList *l;
>
>         ba2str(&device->bdaddr, addr);
>
> @@ -2573,23 +2671,24 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>         data.dev = device;
>         data.all_services = true;
>
> +       /* Copy current list of GATT UUIDs */
> +       for (l = device->gatt_uuids; l; l = g_slist_next(l))
> +               data.old_uuids = g_slist_append(data.old_uuids,
> +                                                       g_strdup(l->data));
>
>         gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile,
>                                                                         &data);
>
> -       /*
> -        * Clear the UUIDs list
> -        *
> -        * FIXME: The management of UUIDs here iss currently broken, as clearing
> -        * the entire list here will likely remove UUIDs that shouldn't be
> -        * removed (e.g. those obtained via SDP for a dual-mode device).
> -        */
> -       g_slist_free_full(device->uuids, g_free);
> -       device->uuids = NULL;
> +       /* Whatever remains in data.old_uuids is stale and should be removed */
> +       remove_invalid_services(&data);
>
> -       device_add_uuids(device, data.uuids);
> -       g_slist_free_full(data.uuids, g_free);
> +       /* Update the list with new UUIDs */
> +       if (!device_add_gatt_uuids(device, data.new_uuids) && data.old_uuids)
> +               g_dbus_emit_property_changed(dbus_conn, device->path,
> +                                               DEVICE_INTERFACE, "UUIDs");
>
> -       remove_invalid_services(&data);
> +       g_slist_free_full(data.new_uuids, g_free);
> +       g_slist_free_full(data.old_uuids, g_free);
>  }
>
>  static void device_accept_gatt_profiles(struct btd_device *device)
> @@ -2675,46 +2774,39 @@ static gint prim_attr_cmp(gconstpointer a, gconstpointer b)
>         return !(prim->range.start == start && prim->range.end == end);
>  }
>
> -static gint prim_uuid_cmp(gconstpointer a, gconstpointer b)
> -{
> -       const struct gatt_primary *prim = a;
> -       const char *uuid = b;
> -
> -       return bt_uuid_strcmp(prim->uuid, uuid);
> -}
> -
>  static void gatt_service_removed(struct gatt_db_attribute *attr,
>                                                                 void *user_data)
>  {
>         struct btd_device *device = user_data;
>         GSList *l;
> -       struct gatt_primary *prim;
>         uint16_t start, end;
> +       bt_uuid_t uuid;
> +       char uuid_str[MAX_LEN_UUID_STR];
>
>         if (!bt_gatt_client_is_ready(device->client))
>                 return;
>
> -       gatt_db_attribute_get_service_handles(attr, &start, &end);
> +       gatt_db_attribute_get_service_data(attr, &start, &end, NULL, &uuid);
>
>         DBG("start: 0x%04x, end: 0x%04x", start, end);
>
>         /* Remove the corresponding gatt_primary */
>         l = g_slist_find_custom(device->primaries, attr, prim_attr_cmp);
> -       if (!l)
> -               return;
> +       if (l) {
> +               g_free(l->data);
> +               device->primaries = g_slist_delete_link(device->primaries, l);
> +       }
>
> -       prim = l->data;
> -       device->primaries = g_slist_delete_link(device->primaries, l);
> +       bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
>
>         /* Get the UUID entry to be removed below */
> -       l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
> +       l = g_slist_find_custom(device->gatt_uuids, uuid_str, bt_uuid_strcmp);
>
>         /*
>          * Remove the corresponding UUIDs entry and the profile, only if this
>          * is the last service with this UUID.
>          */
> -       if (!g_slist_find_custom(device->primaries, prim->uuid,
> -                                                       prim_uuid_cmp)) {
> +       if (!gatt_db_get_service_with_uuid(device->db, &uuid)) {
>                 /*
>                  * If this happened since the db was cleared for a non-bonded
>                  * device, then don't remove the btd_service just yet. We do
> @@ -2727,13 +2819,11 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
>                         device_remove_gatt_profile(device, attr);
>
>                 g_free(l->data);
> -               device->uuids = g_slist_delete_link(device->uuids, l);
> +               device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
>                 g_dbus_emit_property_changed(dbus_conn, device->path,
>                                                 DEVICE_INTERFACE, "UUIDs");
>         }
>
> -       g_free(prim);
> -
>         store_device_info(device);
>
>         btd_gatt_client_service_removed(device->client_dbus, attr);
> @@ -2988,8 +3078,13 @@ void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup)
>         dev->trusted = dup->trusted;
>         dev->blocked = dup->blocked;
>
> -       for (l = dup->uuids; l; l = g_slist_next(l))
> -               dev->uuids = g_slist_append(dev->uuids, g_strdup(l->data));
> +       for (l = dup->sdp_uuids; l; l = g_slist_next(l))
> +               dev->sdp_uuids = g_slist_append(dev->sdp_uuids,
> +                                                       g_strdup(l->data));
> +
> +       for (l = dup->gatt_uuids; l; l = g_slist_next(l))
> +               dev->gatt_uuids = g_slist_append(dev->gatt_uuids,
> +                                                       g_strdup(l->data));
>
>         if (dev->name[0] == '\0')
>                 strcpy(dev->name, dup->name);
> @@ -3240,9 +3335,14 @@ static gboolean record_has_uuid(const sdp_record_t *rec,
>         return FALSE;
>  }
>
> -GSList *btd_device_get_uuids(struct btd_device *device)
> +GSList *btd_device_get_sdp_uuids(struct btd_device *device)
>  {
> -       return device->uuids;
> +       return device->sdp_uuids;
> +}
> +
> +GSList *btd_device_get_gatt_uuids(struct btd_device *device)
> +{
> +       return device->gatt_uuids;
>  }
>
>  struct probe_data {
> @@ -3280,7 +3380,8 @@ void device_probe_profile(gpointer a, gpointer b)
>         if (profile->device_probe == NULL)
>                 return;
>
> -       if (!device_match_profile(device, profile, device->uuids))
> +       if (!device_match_profile(device, profile, device->sdp_uuids) &&
> +               !device_match_profile(device, profile, device->gatt_uuids))
>                 return;
>
>         service = service_create(device, profile);
> @@ -3318,7 +3419,8 @@ void device_remove_profile(gpointer a, gpointer b)
>         service_remove(service);
>  }
>
> -void device_probe_profiles(struct btd_device *device, GSList *uuids)
> +void device_probe_profiles(struct btd_device *device, GSList *uuids,
> +                                                               bool gatt)
>  {
>         struct probe_data d = { device, uuids };
>         char addr[18];
> @@ -3335,7 +3437,10 @@ void device_probe_profiles(struct btd_device *device, GSList *uuids)
>         btd_profile_foreach(dev_probe, &d);
>
>  add_uuids:
> -       device_add_uuids(device, uuids);
> +       if (gatt)
> +               device_add_gatt_uuids(device, uuids);
> +       else
> +               device_add_sdp_uuids(device, uuids);
>  }
>
>  static void store_sdp_record(GKeyFile *key_file, sdp_record_t *rec)
> @@ -3431,7 +3536,7 @@ static int update_record(struct browse_req *req, const char *uuid,
>         req->records = sdp_list_append(req->records, sdp_copy_record(rec));
>
>         /* Check if UUID is duplicated */
> -       l = g_slist_find_custom(req->device->uuids, uuid, bt_uuid_strcmp);
> +       l = g_slist_find_custom(req->device->sdp_uuids, uuid, bt_uuid_strcmp);
>         if (l == NULL) {
>                 l = g_slist_find_custom(req->profiles_added, uuid,
>                                                         bt_uuid_strcmp);
> @@ -3662,7 +3767,7 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
>          * the full list of services and populate a client-role gatt_db over
>          * BR/EDR.
>          */
> -       device_probe_profiles(device, req->profiles_added);
> +       device_probe_profiles(device, req->profiles_added, false);
>
>         /* Propagate services changes */
>         g_dbus_emit_property_changed(dbus_conn, req->device->path,
> @@ -3927,6 +4032,8 @@ static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
>
>         if (device->browse)
>                 register_gatt_services(device->browse);
> +       else
> +               device_probe_gatt_profiles(device);
>
>         device_accept_gatt_profiles(device);
>
> @@ -5127,13 +5234,13 @@ void btd_device_add_uuid(struct btd_device *device, const char *uuid)
>         GSList *uuid_list;
>         char *new_uuid;
>
> -       if (g_slist_find_custom(device->uuids, uuid, bt_uuid_strcmp))
> +       if (g_slist_find_custom(device->sdp_uuids, uuid, bt_uuid_strcmp))
>                 return;
>
>         new_uuid = g_strdup(uuid);
>         uuid_list = g_slist_append(NULL, new_uuid);
>
> -       device_probe_profiles(device, uuid_list);
> +       device_probe_profiles(device, uuid_list, false);
>
>         g_free(new_uuid);
>         g_slist_free(uuid_list);
> diff --git a/src/device.h b/src/device.h
> index a7fefee..f250bfa 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -60,8 +60,10 @@ struct device_addr_type {
>  };
>
>  int device_addr_type_cmp(gconstpointer a, gconstpointer b);
> -GSList *btd_device_get_uuids(struct btd_device *device);
> -void device_probe_profiles(struct btd_device *device, GSList *profiles);
> +GSList *btd_device_get_sdp_uuids(struct btd_device *device);
> +GSList *btd_device_get_gatt_uuids(struct btd_device *device);
> +void device_probe_profiles(struct btd_device *device, GSList *profiles,
> +                                                               bool gatt);

Id leave this API as it is for now and internally classify the UUIDs,
the classification function can be used when loading from the storage
that way we don't need to break the storage format.

>  const sdp_record_t *btd_device_get_record(struct btd_device *device,
>                                                 const char *uuid);
>  struct gatt_primary *btd_device_get_primary(struct btd_device *device,
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> 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  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v3 3/8] core: device: Fix broken GATT UUID management

[Arman Uguray]

Hi Luiz,

> On Wed, Jan 14, 2015 at 5:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Arman,
>
> On Wed, Jan 14, 2015 at 1:31 AM, Arman Uguray <armansito@chromium.org> wrote:
>> This patch fixes the broken GATT UUID management, which incorrectly
>> remove valid UUIDs during the GATT profile probe process. This is
>> achieved by splitting UUIDs obtained via GATT and SDP into their own
>> lists that are internal to btd_device.
>> ---
>>  plugins/sixaxis.c |   2 +-
>>  src/adapter.c     |   8 +-
>>  src/device.c      | 367 +++++++++++++++++++++++++++++++++++-------------------
>>  src/device.h      |   6 +-
>>  4 files changed, 248 insertions(+), 135 deletions(-)
>>
>> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
>> index ac53ba9..0a4e3d8 100644
>> --- a/plugins/sixaxis.c
>> +++ b/plugins/sixaxis.c
>> @@ -289,7 +289,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>>
>>         device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
>>
>> -       if (g_slist_find_custom(btd_device_get_uuids(device), HID_UUID,
>> +       if (g_slist_find_custom(btd_device_get_sdp_uuids(device), HID_UUID,
>>                                                 (GCompareFunc)strcasecmp)) {
>>                 DBG("device %s already known, skipping", device_addr);
>>                 return true;
>> diff --git a/src/adapter.c b/src/adapter.c
>> index d7e2550..379a80b 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -2912,9 +2912,13 @@ static void load_devices(struct btd_adapter *adapter)
>>
>>                 /* TODO: register services from pre-loaded list of primaries */
>>
>> -               list = btd_device_get_uuids(device);
>> +               list = btd_device_get_sdp_uuids(device);
>>                 if (list)
>> -                       device_probe_profiles(device, list);
>> +                       device_probe_profiles(device, list, false);
>> +
>> +               list = btd_device_get_gatt_uuids(device);
>> +               if (list)
>> +                       device_probe_profiles(device, list, true);
>>
>>  device_exist:
>>                 if (key_info) {
>> diff --git a/src/device.c b/src/device.c
>> index 4f702ba..9d1a480 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -193,7 +193,8 @@ struct btd_device {
>>         uint16_t        appearance;
>>         char            *modalias;
>>         struct btd_adapter      *adapter;
>> -       GSList          *uuids;
>> +       GSList          *sdp_uuids;
>> +       GSList          *gatt_uuids;
>>         GSList          *primaries;             /* List of primary services */
>>         GSList          *services;              /* List of btd_service */
>>         GSList          *pending;               /* Pending services */
>> @@ -335,6 +336,28 @@ static void update_technologies(GKeyFile *file, struct btd_device *dev)
>>                                                                 list, len);
>>  }
>>
>> +static char **store_service_uuids(GKeyFile *key_file, gchar *key, GSList *uuids)
>> +{
>> +       GSList *l;
>> +       char **array;
>> +       int i;
>> +
>> +       if (!uuids) {
>> +               g_key_file_remove_key(key_file, "General", key, NULL);
>> +               return NULL;
>> +       }
>> +
>> +       array = g_new0(char *, g_slist_length(uuids) + 1);
>> +
>> +       for (i = 0, l = uuids; l; l = g_slist_next(l), i++)
>> +               array[i] = l->data;
>> +
>> +       g_key_file_set_string_list(key_file, "General", key,
>> +                                               (const char **)array, i);
>> +
>> +       return array;
>> +}
>> +
>>  static gboolean store_device_info_cb(gpointer user_data)
>>  {
>>         struct btd_device *device = user_data;
>> @@ -344,7 +367,7 @@ static gboolean store_device_info_cb(gpointer user_data)
>>         char device_addr[18];
>>         char *str;
>>         char class[9];
>> -       char **uuids = NULL;
>> +       char **sdp_uuids, **gatt_uuids;
>>         gsize length = 0;
>>
>>         device->store_id = 0;
>> @@ -387,18 +410,10 @@ static gboolean store_device_info_cb(gpointer user_data)
>>         g_key_file_set_boolean(key_file, "General", "Blocked",
>>                                                         device->blocked);
>>
>> -       if (device->uuids) {
>> -               GSList *l;
>> -               int i;
>> -
>> -               uuids = g_new0(char *, g_slist_length(device->uuids) + 1);
>> -               for (i = 0, l = device->uuids; l; l = g_slist_next(l), i++)
>> -                       uuids[i] = l->data;
>> -               g_key_file_set_string_list(key_file, "General", "Services",
>> -                                               (const char **)uuids, i);
>> -       } else {
>> -               g_key_file_remove_key(key_file, "General", "Services", NULL);
>> -       }
>> +       sdp_uuids = store_service_uuids(key_file, "SDPServices",
>> +                                                       device->sdp_uuids);
>> +       gatt_uuids = store_service_uuids(key_file, "GATTServices",
>> +                                                       device->gatt_uuids);
>
> This change is not backward compatible which mean existing storage
> will not load properly if we change the storage fields, furthermore I
> think it is better to store the UUIDs per bearer_state since service
> discovered flag is stored there it seem logical to couple UUIDs founds
> there as well.
>

You're right, I'm aware that this is not backwards compatible. I
didn't build any migration mechanism from the UUIDs field to the new
fields to sort of initiate this discussion with you. I'm still trying
to figure out how to resolve the current format. See my comments
below.

>>         if (device->vendor_src) {
>>                 g_key_file_set_integer(key_file, "DeviceID", "Source",
>> @@ -420,7 +435,8 @@ static gboolean store_device_info_cb(gpointer user_data)
>>         g_free(str);
>>
>>         g_key_file_free(key_file);
>> -       g_free(uuids);
>> +       g_free(sdp_uuids);
>> +       g_free(gatt_uuids);
>>
>>         return FALSE;
>>  }
>> @@ -574,7 +590,8 @@ static void device_free(gpointer user_data)
>>         btd_gatt_client_destroy(device->client_dbus);
>>         device->client_dbus = NULL;
>>
>> -       g_slist_free_full(device->uuids, g_free);
>> +       g_slist_free_full(device->sdp_uuids, g_free);
>> +       g_slist_free_full(device->gatt_uuids, g_free);
>>         g_slist_free_full(device->primaries, g_free);
>>         g_slist_free_full(device->attios, g_free);
>>         g_slist_free_full(device->attios_offline, g_free);
>> @@ -988,22 +1005,43 @@ static gboolean dev_property_get_uuids(const GDBusPropertyTable *property,
>>  {
>>         struct btd_device *dev = data;
>>         DBusMessageIter entry;
>> -       GSList *l;
>> +       GHashTable *uuids = NULL;
>> +       GSList *sl;
>> +       GList *keys, *l;
>>
>>         dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>                                 DBUS_TYPE_STRING_AS_STRING, &entry);
>>
>> -       if (dev->bredr_state.svc_resolved || dev->le_state.svc_resolved)
>> -               l = dev->uuids;
>> -       else if (dev->eir_uuids)
>> -               l = dev->eir_uuids;
>> -       else
>> -               l = dev->uuids;
>> +       if (!dev->bredr_state.svc_resolved && dev->le_state.svc_resolved &&
>> +                                                       dev->eir_uuids) {
>> +               for (sl = dev->eir_uuids; sl; sl = g_slist_next(sl))
>> +                       dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
>> +                                                               &sl->data);
>> +
>> +               goto done;
>> +       }
>> +
>> +       /* Insert GATT and SDP UUIDs into a set to prevent any duplicates */
>> +       uuids = g_hash_table_new(g_str_hash, bt_uuid_strcmp);
>>
>> -       for (; l != NULL; l = l->next)
>> +       for (sl = dev->sdp_uuids; sl; sl = g_slist_next(sl))
>> +               g_hash_table_add(uuids, sl->data);
>> +
>> +       for (sl = dev->gatt_uuids; sl; sl = g_slist_next(sl))
>> +               g_hash_table_add(uuids, sl->data);
>> +
>> +       /* Obtain a list of UUIDs and sort it */
>> +       keys = g_hash_table_get_keys(uuids);
>> +       keys = g_list_sort(keys, bt_uuid_strcmp);
>> +
>> +       for (l = keys; l; l = g_list_next(l))
>>                 dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
>> -                                                       &l->data);
>> +                                                               &l->data);
>> +
>> +       g_list_free(keys);
>> +       g_hash_table_destroy(uuids);
>>
>> +done:
>>         dbus_message_iter_close_container(iter, &entry);
>>
>>         return TRUE;
>> @@ -1130,7 +1168,8 @@ int device_unblock(struct btd_device *device, gboolean silent,
>>         if (!silent) {
>>                 g_dbus_emit_property_changed(dbus_conn, device->path,
>>                                                 DEVICE_INTERFACE, "Blocked");
>> -               device_probe_profiles(device, device->uuids);
>> +               device_probe_profiles(device, device->sdp_uuids, false);
>> +               device_probe_profiles(device, device->gatt_uuids, true);
>>         }
>>
>>         return 0;
>> @@ -2165,6 +2204,66 @@ failed:
>>         return str;
>>  }
>>
>> +static bool device_add_uuid(GSList **uuids, const char *uuid)
>> +{
>> +       if (g_slist_find_custom(*uuids, uuid, bt_uuid_strcmp))
>> +               return false;
>> +
>> +       *uuids = g_slist_insert_sorted(*uuids, g_strdup(uuid), bt_uuid_strcmp);
>> +
>> +       return true;
>> +}
>> +
>> +static bool device_add_uuids(struct btd_device *device, GSList **uuids,
>> +                                                       GSList *new_uuids)
>> +{
>> +       GSList *l;
>> +       bool changed = false;
>> +
>> +       for (l = new_uuids; l; l = g_slist_next(l)) {
>> +               if (device_add_uuid(uuids, l->data))
>> +                       changed = true;
>> +       }
>> +
>> +       if (changed)
>> +               g_dbus_emit_property_changed(dbus_conn, device->path,
>> +                                               DEVICE_INTERFACE, "UUIDs");
>> +
>> +       return changed;
>> +}
>> +
>> +static bool device_add_sdp_uuid(struct btd_device *device, const char *uuid)
>> +{
>> +       return device_add_uuid(&device->sdp_uuids, uuid);
>> +}
>> +
>> +static bool device_add_sdp_uuids(struct btd_device *device, GSList *uuids)
>> +{
>> +       return device_add_uuids(device, &device->sdp_uuids, uuids);
>> +}
>> +
>> +static bool device_add_gatt_uuid(struct btd_device *device, const char *uuid)
>> +{
>> +       return device_add_uuid(&device->gatt_uuids, uuid);
>> +}
>> +
>> +static bool device_add_gatt_uuids(struct btd_device *device, GSList *uuids)
>> +{
>> +       return device_add_uuids(device, &device->gatt_uuids, uuids);
>> +}
>> +
>> +static void device_remove_gatt_uuid(struct btd_device *device, const char *uuid)
>> +{
>> +       GSList *l;
>> +
>> +       l = g_slist_find_custom(device->gatt_uuids, uuid, bt_uuid_strcmp);
>> +       if (!l)
>> +               return;
>> +
>> +       g_free(l->data);
>> +       device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
>> +}
>> +
>>  static void load_info(struct btd_device *device, const char *local,
>>                         const char *peer, GKeyFile *key_file)
>>  {
>> @@ -2254,30 +2353,40 @@ next:
>>         if (blocked)
>>                 device_block(device, FALSE);
>>
>> -       /* Load device profile list */
>> -       uuids = g_key_file_get_string_list(key_file, "General", "Services",
>> -                                               NULL, NULL);
>> +       /* Load classic device profile list */
>> +       uuids = g_key_file_get_string_list(key_file, "General", "SDPServices",
>> +                                                               NULL, NULL);
>>         if (uuids) {
>>                 char **uuid;
>>
>> -               for (uuid = uuids; *uuid; uuid++) {
>> -                       GSList *match;
>> +               for (uuid = uuids; *uuid; uuid++)
>> +                       device_add_sdp_uuid(device, *uuid);
>>
>> -                       match = g_slist_find_custom(device->uuids, *uuid,
>> -                                                       bt_uuid_strcmp);
>> -                       if (match)
>> -                               continue;
>> -
>> -                       device->uuids = g_slist_insert_sorted(device->uuids,
>> -                                                               g_strdup(*uuid),
>> -                                                               bt_uuid_strcmp);
>> -               }
>>                 g_strfreev(uuids);
>>
>>                 /* Discovered services restored from storage */
>>                 device->bredr_state.svc_resolved = true;
>>         }
>>
>> +       /* Load GATT-based device profile list */
>> +       uuids = g_key_file_get_string_list(key_file, "General", "GATTServices",
>> +                                                               NULL, NULL);
>> +       if (uuids) {
>> +               char **uuid;
>> +
>> +               for (uuid = uuids; *uuid; uuid++)
>> +                       device_add_gatt_uuid(device, *uuid);
>> +
>> +               g_strfreev(uuids);
>> +
>> +               /*
>> +                * TODO: The GATT-based service UUIDs have been restored from
>> +                * storage but we don't have a populated gatt-db yet. Here we
>> +                * should mark le_state.svc_resolved as true, if we're bonded
>> +                * and we have a populated gatt_db.
>> +                */
>> +       }
>> +
>>         /* Load device id */
>>         source = g_key_file_get_integer(key_file, "DeviceID", "Source", NULL);
>>         if (source) {
>> @@ -2411,34 +2520,13 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data)
>>         *new_services = g_slist_append(*new_services, prim);
>>  }
>>
>> -static void device_add_uuids(struct btd_device *device, GSList *uuids)
>> -{
>> -       GSList *l;
>> -       bool changed = false;
>> -
>> -       for (l = uuids; l != NULL; l = g_slist_next(l)) {
>> -               GSList *match = g_slist_find_custom(device->uuids, l->data,
>> -                                                       bt_uuid_strcmp);
>> -               if (match)
>> -                       continue;
>> -
>> -               changed = true;
>> -               device->uuids = g_slist_insert_sorted(device->uuids,
>> -                                               g_strdup(l->data),
>> -                                               bt_uuid_strcmp);
>> -       }
>> -
>> -       if (changed)
>> -               g_dbus_emit_property_changed(dbus_conn, device->path,
>> -                                               DEVICE_INTERFACE, "UUIDs");
>> -}
>> -
>>  static void store_services(struct btd_device *device);
>>
>>  struct gatt_probe_data {
>>         struct btd_device *dev;
>>         bool all_services;
>> -       GSList *uuids;
>> +       GSList *new_uuids;
>> +       GSList *old_uuids;
>>         struct gatt_db_attribute *cur_attr;
>>         char cur_uuid[MAX_LEN_UUID_STR];
>>  };
>> @@ -2488,20 +2576,31 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>>  {
>>         struct gatt_probe_data *data = user_data;
>>         bt_uuid_t uuid;
>> -       GSList *l = NULL;
>> +       gpointer dup_uuid;
>>
>>         gatt_db_attribute_get_service_uuid(attr, &uuid);
>>         bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid));
>>
>>         data->cur_attr = attr;
>>
>> -       /*
>> -        * If we're probing for all services, store the UUID since device->uuids
>> -        * was cleared.
>> -        */
>> -       if (data->all_services)
>> -               data->uuids = g_slist_append(data->uuids,
>> +       if (data->all_services) {
>> +               GSList *l;
>> +
>> +               /*
>> +                * Check if the service is already in the old UUIDs list. If so,
>> +                * remove it.
>> +                */
>> +               l = g_slist_find_custom(data->old_uuids, data->cur_uuid,
>> +                                                               bt_uuid_strcmp);
>> +               if (l) {
>> +                       g_free(l->data);
>> +                       data->old_uuids = g_slist_delete_link(data->old_uuids,
>> +                                                                       l);
>> +               } else {
>> +                       data->new_uuids = g_slist_append(data->new_uuids,
>>                                                 g_strdup(data->cur_uuid));
>> +               }
>> +       }
>>
>>         /* Don't probe the profiles if a matching service already exists. */
>>         if (find_service_with_uuid(data->dev->services, data->cur_uuid))
>> @@ -2512,8 +2611,14 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>>         if (data->all_services)
>>                 return;
>>
>> -       l = g_slist_append(l, g_strdup(data->cur_uuid));
>> -       device_add_uuids(data->dev, l);
>> +       dup_uuid = g_strdup(data->cur_uuid);
>> +       if (!device_add_gatt_uuid(data->dev, dup_uuid)) {
>> +               g_free(dup_uuid);
>> +               return;
>> +       }
>> +
>> +       g_dbus_emit_property_changed(dbus_conn, data->dev->path,
>> +                                               DEVICE_INTERFACE, "UUIDs");
>>  }
>>
>>  static void device_probe_gatt_profile(struct btd_device *device,
>> @@ -2526,33 +2631,25 @@ static void device_probe_gatt_profile(struct btd_device *device,
>>         data.dev = device;
>>
>>         dev_probe_gatt_profile(attr, &data);
>> -       g_slist_free_full(data.uuids, g_free);
>>  }
>>
>>  static void remove_invalid_services(struct gatt_probe_data *data)
>>  {
>>         struct btd_device *dev = data->dev;
>>         struct btd_service *service;
>> -       struct btd_profile *profile;
>> -       GSList *l, *tmp;
>> +       GSList *l, *svc;
>>
>> -       for (l = dev->services; l != NULL;) {
>> -               service = l->data;
>> -               profile = btd_service_get_profile(service);
>> +       for (l = data->old_uuids; l; l = g_slist_next(l)) {
>> +               device_remove_gatt_uuid(dev, l->data);
>>
>> -               if (g_slist_find_custom(dev->uuids, profile->remote_uuid,
>> -                                                       bt_uuid_strcmp)) {
>> -                       l = g_slist_next(l);
>> +               svc = find_service_with_uuid(dev->services, l->data);
>> +               if (!svc)
>>                         continue;
>> -               }
>>
>> -               /* Service no longer valid, so remove it */
>> -               tmp = l->next;
>> -               dev->services = g_slist_delete_link(dev->services, l);
>> +               service = svc->data;
>> +               dev->services = g_slist_delete_link(dev->services, svc);
>>                 dev->pending = g_slist_remove(dev->pending, service);
>>                 service_remove(service);
>> -
>> -               l = tmp;
>>         }
>>  }
>>
>> @@ -2560,6 +2657,7 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>>  {
>>         struct gatt_probe_data data;
>>         char addr[18];
>> +       GSList *l;
>>
>>         ba2str(&device->bdaddr, addr);
>>
>> @@ -2573,23 +2671,24 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>>         data.dev = device;
>>         data.all_services = true;
>>
>> +       /* Copy current list of GATT UUIDs */
>> +       for (l = device->gatt_uuids; l; l = g_slist_next(l))
>> +               data.old_uuids = g_slist_append(data.old_uuids,
>> +                                                       g_strdup(l->data));
>>
>>         gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile,
>>                                                                         &data);
>>
>> -       /*
>> -        * Clear the UUIDs list
>> -        *
>> -        * FIXME: The management of UUIDs here iss currently broken, as clearing
>> -        * the entire list here will likely remove UUIDs that shouldn't be
>> -        * removed (e.g. those obtained via SDP for a dual-mode device).
>> -        */
>> -       g_slist_free_full(device->uuids, g_free);
>> -       device->uuids = NULL;
>> +       /* Whatever remains in data.old_uuids is stale and should be removed */
>> +       remove_invalid_services(&data);
>>
>> -       device_add_uuids(device, data.uuids);
>> -       g_slist_free_full(data.uuids, g_free);
>> +       /* Update the list with new UUIDs */
>> +       if (!device_add_gatt_uuids(device, data.new_uuids) && data.old_uuids)
>> +               g_dbus_emit_property_changed(dbus_conn, device->path,
>> +                                               DEVICE_INTERFACE, "UUIDs");
>>
>> -       remove_invalid_services(&data);
>> +       g_slist_free_full(data.new_uuids, g_free);
>> +       g_slist_free_full(data.old_uuids, g_free);
>>  }
>>
>>  static void device_accept_gatt_profiles(struct btd_device *device)
>> @@ -2675,46 +2774,39 @@ static gint prim_attr_cmp(gconstpointer a, gconstpointer b)
>>         return !(prim->range.start == start && prim->range.end == end);
>>  }
>>
>> -static gint prim_uuid_cmp(gconstpointer a, gconstpointer b)
>> -{
>> -       const struct gatt_primary *prim = a;
>> -       const char *uuid = b;
>> -
>> -       return bt_uuid_strcmp(prim->uuid, uuid);
>> -}
>> -
>>  static void gatt_service_removed(struct gatt_db_attribute *attr,
>>                                                                 void *user_data)
>>  {
>>         struct btd_device *device = user_data;
>>         GSList *l;
>> -       struct gatt_primary *prim;
>>         uint16_t start, end;
>> +       bt_uuid_t uuid;
>> +       char uuid_str[MAX_LEN_UUID_STR];
>>
>>         if (!bt_gatt_client_is_ready(device->client))
>>                 return;
>>
>> -       gatt_db_attribute_get_service_handles(attr, &start, &end);
>> +       gatt_db_attribute_get_service_data(attr, &start, &end, NULL, &uuid);
>>
>>         DBG("start: 0x%04x, end: 0x%04x", start, end);
>>
>>         /* Remove the corresponding gatt_primary */
>>         l = g_slist_find_custom(device->primaries, attr, prim_attr_cmp);
>> -       if (!l)
>> -               return;
>> +       if (l) {
>> +               g_free(l->data);
>> +               device->primaries = g_slist_delete_link(device->primaries, l);
>> +       }
>>
>> -       prim = l->data;
>> -       device->primaries = g_slist_delete_link(device->primaries, l);
>> +       bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
>>
>>         /* Get the UUID entry to be removed below */
>> -       l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
>> +       l = g_slist_find_custom(device->gatt_uuids, uuid_str, bt_uuid_strcmp);
>>
>>         /*
>>          * Remove the corresponding UUIDs entry and the profile, only if this
>>          * is the last service with this UUID.
>>          */
>> -       if (!g_slist_find_custom(device->primaries, prim->uuid,
>> -                                                       prim_uuid_cmp)) {
>> +       if (!gatt_db_get_service_with_uuid(device->db, &uuid)) {
>>                 /*
>>                  * If this happened since the db was cleared for a non-bonded
>>                  * device, then don't remove the btd_service just yet. We do
>> @@ -2727,13 +2819,11 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
>>                         device_remove_gatt_profile(device, attr);
>>
>>                 g_free(l->data);
>> -               device->uuids = g_slist_delete_link(device->uuids, l);
>> +               device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
>>                 g_dbus_emit_property_changed(dbus_conn, device->path,
>>                                                 DEVICE_INTERFACE, "UUIDs");
>>         }
>>
>> -       g_free(prim);
>> -
>>         store_device_info(device);
>>
>>         btd_gatt_client_service_removed(device->client_dbus, attr);
>> @@ -2988,8 +3078,13 @@ void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup)
>>         dev->trusted = dup->trusted;
>>         dev->blocked = dup->blocked;
>>
>> -       for (l = dup->uuids; l; l = g_slist_next(l))
>> -               dev->uuids = g_slist_append(dev->uuids, g_strdup(l->data));
>> +       for (l = dup->sdp_uuids; l; l = g_slist_next(l))
>> +               dev->sdp_uuids = g_slist_append(dev->sdp_uuids,
>> +                                                       g_strdup(l->data));
>> +
>> +       for (l = dup->gatt_uuids; l; l = g_slist_next(l))
>> +               dev->gatt_uuids = g_slist_append(dev->gatt_uuids,
>> +                                                       g_strdup(l->data));
>>
>>         if (dev->name[0] == '\0')
>>                 strcpy(dev->name, dup->name);
>> @@ -3240,9 +3335,14 @@ static gboolean record_has_uuid(const sdp_record_t *rec,
>>         return FALSE;
>>  }
>>
>> -GSList *btd_device_get_uuids(struct btd_device *device)
>> +GSList *btd_device_get_sdp_uuids(struct btd_device *device)
>>  {
>> -       return device->uuids;
>> +       return device->sdp_uuids;
>> +}
>> +
>> +GSList *btd_device_get_gatt_uuids(struct btd_device *device)
>> +{
>> +       return device->gatt_uuids;
>>  }
>>
>>  struct probe_data {
>> @@ -3280,7 +3380,8 @@ void device_probe_profile(gpointer a, gpointer b)
>>         if (profile->device_probe == NULL)
>>                 return;
>>
>> -       if (!device_match_profile(device, profile, device->uuids))
>> +       if (!device_match_profile(device, profile, device->sdp_uuids) &&
>> +               !device_match_profile(device, profile, device->gatt_uuids))
>>                 return;
>>
>>         service = service_create(device, profile);
>> @@ -3318,7 +3419,8 @@ void device_remove_profile(gpointer a, gpointer b)
>>         service_remove(service);
>>  }
>>
>> -void device_probe_profiles(struct btd_device *device, GSList *uuids)
>> +void device_probe_profiles(struct btd_device *device, GSList *uuids,
>> +                                                               bool gatt)
>>  {
>>         struct probe_data d = { device, uuids };
>>         char addr[18];
>> @@ -3335,7 +3437,10 @@ void device_probe_profiles(struct btd_device *device, GSList *uuids)
>>         btd_profile_foreach(dev_probe, &d);
>>
>>  add_uuids:
>> -       device_add_uuids(device, uuids);
>> +       if (gatt)
>> +               device_add_gatt_uuids(device, uuids);
>> +       else
>> +               device_add_sdp_uuids(device, uuids);
>>  }
>>
>>  static void store_sdp_record(GKeyFile *key_file, sdp_record_t *rec)
>> @@ -3431,7 +3536,7 @@ static int update_record(struct browse_req *req, const char *uuid,
>>         req->records = sdp_list_append(req->records, sdp_copy_record(rec));
>>
>>         /* Check if UUID is duplicated */
>> -       l = g_slist_find_custom(req->device->uuids, uuid, bt_uuid_strcmp);
>> +       l = g_slist_find_custom(req->device->sdp_uuids, uuid, bt_uuid_strcmp);
>>         if (l == NULL) {
>>                 l = g_slist_find_custom(req->profiles_added, uuid,
>>                                                         bt_uuid_strcmp);
>> @@ -3662,7 +3767,7 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
>>          * the full list of services and populate a client-role gatt_db over
>>          * BR/EDR.
>>          */
>> -       device_probe_profiles(device, req->profiles_added);
>> +       device_probe_profiles(device, req->profiles_added, false);
>>
>>         /* Propagate services changes */
>>         g_dbus_emit_property_changed(dbus_conn, req->device->path,
>> @@ -3927,6 +4032,8 @@ static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
>>
>>         if (device->browse)
>>                 register_gatt_services(device->browse);
>> +       else
>> +               device_probe_gatt_profiles(device);
>>
>>         device_accept_gatt_profiles(device);
>>
>> @@ -5127,13 +5234,13 @@ void btd_device_add_uuid(struct btd_device *device, const char *uuid)
>>         GSList *uuid_list;
>>         char *new_uuid;
>>
>> -       if (g_slist_find_custom(device->uuids, uuid, bt_uuid_strcmp))
>> +       if (g_slist_find_custom(device->sdp_uuids, uuid, bt_uuid_strcmp))
>>                 return;
>>
>>         new_uuid = g_strdup(uuid);
>>         uuid_list = g_slist_append(NULL, new_uuid);
>>
>> -       device_probe_profiles(device, uuid_list);
>> +       device_probe_profiles(device, uuid_list, false);
>>
>>         g_free(new_uuid);
>>         g_slist_free(uuid_list);
>> diff --git a/src/device.h b/src/device.h
>> index a7fefee..f250bfa 100644
>> --- a/src/device.h
>> +++ b/src/device.h
>> @@ -60,8 +60,10 @@ struct device_addr_type {
>>  };
>>
>>  int device_addr_type_cmp(gconstpointer a, gconstpointer b);
>> -GSList *btd_device_get_uuids(struct btd_device *device);
>> -void device_probe_profiles(struct btd_device *device, GSList *profiles);
>> +GSList *btd_device_get_sdp_uuids(struct btd_device *device);
>> +GSList *btd_device_get_gatt_uuids(struct btd_device *device);
>> +void device_probe_profiles(struct btd_device *device, GSList *profiles,
>> +                                                               bool gatt);
>
> Id leave this API as it is for now and internally classify the UUIDs,
> the classification function can be used when loading from the storage
> that way we don't need to break the storage format.
>

Well, I'm just not sure how to do that classification. Especially in
the case of device_probe_profiles, which gets passed in a list of
UUIDs and the case where the UUIDs get loaded from storage, I don't
know how we can figure out which ones are LE and which ones are
BR/EDR, unless we do something complicated/inefficient like going
through the gatt-db and see if there's a matching service for each
UUID but this still becomes a problem when we load the UUIDs before
having done discovery (i.e. there's no populated gatt-db).

>>  const sdp_record_t *btd_device_get_record(struct btd_device *device,
>>                                                 const char *uuid);
>>  struct gatt_primary *btd_device_get_primary(struct btd_device *device,
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>> --
>> 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  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Arman

Re: [PATCH BlueZ v3 3/8] core: device: Fix broken GATT UUID management

[Arman Uguray]

> On Wed, Jan 14, 2015 at 11:38 AM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Luiz,
>
>> On Wed, Jan 14, 2015 at 5:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> Hi Arman,
>>
>> On Wed, Jan 14, 2015 at 1:31 AM, Arman Uguray <armansito@chromium.org> wrote:
>>> This patch fixes the broken GATT UUID management, which incorrectly
>>> remove valid UUIDs during the GATT profile probe process. This is
>>> achieved by splitting UUIDs obtained via GATT and SDP into their own
>>> lists that are internal to btd_device.
>>> ---
>>>  plugins/sixaxis.c |   2 +-
>>>  src/adapter.c     |   8 +-
>>>  src/device.c      | 367 +++++++++++++++++++++++++++++++++++-------------------
>>>  src/device.h      |   6 +-
>>>  4 files changed, 248 insertions(+), 135 deletions(-)
>>>
>>> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
>>> index ac53ba9..0a4e3d8 100644
>>> --- a/plugins/sixaxis.c
>>> +++ b/plugins/sixaxis.c
>>> @@ -289,7 +289,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>>>
>>>         device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
>>>
>>> -       if (g_slist_find_custom(btd_device_get_uuids(device), HID_UUID,
>>> +       if (g_slist_find_custom(btd_device_get_sdp_uuids(device), HID_UUID,
>>>                                                 (GCompareFunc)strcasecmp)) {
>>>                 DBG("device %s already known, skipping", device_addr);
>>>                 return true;
>>> diff --git a/src/adapter.c b/src/adapter.c
>>> index d7e2550..379a80b 100644
>>> --- a/src/adapter.c
>>> +++ b/src/adapter.c
>>> @@ -2912,9 +2912,13 @@ static void load_devices(struct btd_adapter *adapter)
>>>
>>>                 /* TODO: register services from pre-loaded list of primaries */
>>>
>>> -               list = btd_device_get_uuids(device);
>>> +               list = btd_device_get_sdp_uuids(device);
>>>                 if (list)
>>> -                       device_probe_profiles(device, list);
>>> +                       device_probe_profiles(device, list, false);
>>> +
>>> +               list = btd_device_get_gatt_uuids(device);
>>> +               if (list)
>>> +                       device_probe_profiles(device, list, true);
>>>
>>>  device_exist:
>>>                 if (key_info) {
>>> diff --git a/src/device.c b/src/device.c
>>> index 4f702ba..9d1a480 100644
>>> --- a/src/device.c
>>> +++ b/src/device.c
>>> @@ -193,7 +193,8 @@ struct btd_device {
>>>         uint16_t        appearance;
>>>         char            *modalias;
>>>         struct btd_adapter      *adapter;
>>> -       GSList          *uuids;
>>> +       GSList          *sdp_uuids;
>>> +       GSList          *gatt_uuids;
>>>         GSList          *primaries;             /* List of primary services */
>>>         GSList          *services;              /* List of btd_service */
>>>         GSList          *pending;               /* Pending services */
>>> @@ -335,6 +336,28 @@ static void update_technologies(GKeyFile *file, struct btd_device *dev)
>>>                                                                 list, len);
>>>  }
>>>
>>> +static char **store_service_uuids(GKeyFile *key_file, gchar *key, GSList *uuids)
>>> +{
>>> +       GSList *l;
>>> +       char **array;
>>> +       int i;
>>> +
>>> +       if (!uuids) {
>>> +               g_key_file_remove_key(key_file, "General", key, NULL);
>>> +               return NULL;
>>> +       }
>>> +
>>> +       array = g_new0(char *, g_slist_length(uuids) + 1);
>>> +
>>> +       for (i = 0, l = uuids; l; l = g_slist_next(l), i++)
>>> +               array[i] = l->data;
>>> +
>>> +       g_key_file_set_string_list(key_file, "General", key,
>>> +                                               (const char **)array, i);
>>> +
>>> +       return array;
>>> +}
>>> +
>>>  static gboolean store_device_info_cb(gpointer user_data)
>>>  {
>>>         struct btd_device *device = user_data;
>>> @@ -344,7 +367,7 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>         char device_addr[18];
>>>         char *str;
>>>         char class[9];
>>> -       char **uuids = NULL;
>>> +       char **sdp_uuids, **gatt_uuids;
>>>         gsize length = 0;
>>>
>>>         device->store_id = 0;
>>> @@ -387,18 +410,10 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>         g_key_file_set_boolean(key_file, "General", "Blocked",
>>>                                                         device->blocked);
>>>
>>> -       if (device->uuids) {
>>> -               GSList *l;
>>> -               int i;
>>> -
>>> -               uuids = g_new0(char *, g_slist_length(device->uuids) + 1);
>>> -               for (i = 0, l = device->uuids; l; l = g_slist_next(l), i++)
>>> -                       uuids[i] = l->data;
>>> -               g_key_file_set_string_list(key_file, "General", "Services",
>>> -                                               (const char **)uuids, i);
>>> -       } else {
>>> -               g_key_file_remove_key(key_file, "General", "Services", NULL);
>>> -       }
>>> +       sdp_uuids = store_service_uuids(key_file, "SDPServices",
>>> +                                                       device->sdp_uuids);
>>> +       gatt_uuids = store_service_uuids(key_file, "GATTServices",
>>> +                                                       device->gatt_uuids);
>>
>> This change is not backward compatible which mean existing storage
>> will not load properly if we change the storage fields, furthermore I
>> think it is better to store the UUIDs per bearer_state since service
>> discovered flag is stored there it seem logical to couple UUIDs founds
>> there as well.
>>
>
> You're right, I'm aware that this is not backwards compatible. I
> didn't build any migration mechanism from the UUIDs field to the new
> fields to sort of initiate this discussion with you. I'm still trying
> to figure out how to resolve the current format. See my comments
> below.
>
>>>         if (device->vendor_src) {
>>>                 g_key_file_set_integer(key_file, "DeviceID", "Source",
>>> @@ -420,7 +435,8 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>         g_free(str);
>>>
>>>         g_key_file_free(key_file);
>>> -       g_free(uuids);
>>> +       g_free(sdp_uuids);
>>> +       g_free(gatt_uuids);
>>>
>>>         return FALSE;
>>>  }
>>> @@ -574,7 +590,8 @@ static void device_free(gpointer user_data)
>>>         btd_gatt_client_destroy(device->client_dbus);
>>>         device->client_dbus = NULL;
>>>
>>> -       g_slist_free_full(device->uuids, g_free);
>>> +       g_slist_free_full(device->sdp_uuids, g_free);
>>> +       g_slist_free_full(device->gatt_uuids, g_free);
>>>         g_slist_free_full(device->primaries, g_free);
>>>         g_slist_free_full(device->attios, g_free);
>>>         g_slist_free_full(device->attios_offline, g_free);
>>> @@ -988,22 +1005,43 @@ static gboolean dev_property_get_uuids(const GDBusPropertyTable *property,
>>>  {
>>>         struct btd_device *dev = data;
>>>         DBusMessageIter entry;
>>> -       GSList *l;
>>> +       GHashTable *uuids = NULL;
>>> +       GSList *sl;
>>> +       GList *keys, *l;
>>>
>>>         dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>>                                 DBUS_TYPE_STRING_AS_STRING, &entry);
>>>
>>> -       if (dev->bredr_state.svc_resolved || dev->le_state.svc_resolved)
>>> -               l = dev->uuids;
>>> -       else if (dev->eir_uuids)
>>> -               l = dev->eir_uuids;
>>> -       else
>>> -               l = dev->uuids;
>>> +       if (!dev->bredr_state.svc_resolved && dev->le_state.svc_resolved &&
>>> +                                                       dev->eir_uuids) {
>>> +               for (sl = dev->eir_uuids; sl; sl = g_slist_next(sl))
>>> +                       dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
>>> +                                                               &sl->data);
>>> +
>>> +               goto done;
>>> +       }
>>> +
>>> +       /* Insert GATT and SDP UUIDs into a set to prevent any duplicates */
>>> +       uuids = g_hash_table_new(g_str_hash, bt_uuid_strcmp);
>>>
>>> -       for (; l != NULL; l = l->next)
>>> +       for (sl = dev->sdp_uuids; sl; sl = g_slist_next(sl))
>>> +               g_hash_table_add(uuids, sl->data);
>>> +
>>> +       for (sl = dev->gatt_uuids; sl; sl = g_slist_next(sl))
>>> +               g_hash_table_add(uuids, sl->data);
>>> +
>>> +       /* Obtain a list of UUIDs and sort it */
>>> +       keys = g_hash_table_get_keys(uuids);
>>> +       keys = g_list_sort(keys, bt_uuid_strcmp);
>>> +
>>> +       for (l = keys; l; l = g_list_next(l))
>>>                 dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
>>> -                                                       &l->data);
>>> +                                                               &l->data);
>>> +
>>> +       g_list_free(keys);
>>> +       g_hash_table_destroy(uuids);
>>>
>>> +done:
>>>         dbus_message_iter_close_container(iter, &entry);
>>>
>>>         return TRUE;
>>> @@ -1130,7 +1168,8 @@ int device_unblock(struct btd_device *device, gboolean silent,
>>>         if (!silent) {
>>>                 g_dbus_emit_property_changed(dbus_conn, device->path,
>>>                                                 DEVICE_INTERFACE, "Blocked");
>>> -               device_probe_profiles(device, device->uuids);
>>> +               device_probe_profiles(device, device->sdp_uuids, false);
>>> +               device_probe_profiles(device, device->gatt_uuids, true);
>>>         }
>>>
>>>         return 0;
>>> @@ -2165,6 +2204,66 @@ failed:
>>>         return str;
>>>  }
>>>
>>> +static bool device_add_uuid(GSList **uuids, const char *uuid)
>>> +{
>>> +       if (g_slist_find_custom(*uuids, uuid, bt_uuid_strcmp))
>>> +               return false;
>>> +
>>> +       *uuids = g_slist_insert_sorted(*uuids, g_strdup(uuid), bt_uuid_strcmp);
>>> +
>>> +       return true;
>>> +}
>>> +
>>> +static bool device_add_uuids(struct btd_device *device, GSList **uuids,
>>> +                                                       GSList *new_uuids)
>>> +{
>>> +       GSList *l;
>>> +       bool changed = false;
>>> +
>>> +       for (l = new_uuids; l; l = g_slist_next(l)) {
>>> +               if (device_add_uuid(uuids, l->data))
>>> +                       changed = true;
>>> +       }
>>> +
>>> +       if (changed)
>>> +               g_dbus_emit_property_changed(dbus_conn, device->path,
>>> +                                               DEVICE_INTERFACE, "UUIDs");
>>> +
>>> +       return changed;
>>> +}
>>> +
>>> +static bool device_add_sdp_uuid(struct btd_device *device, const char *uuid)
>>> +{
>>> +       return device_add_uuid(&device->sdp_uuids, uuid);
>>> +}
>>> +
>>> +static bool device_add_sdp_uuids(struct btd_device *device, GSList *uuids)
>>> +{
>>> +       return device_add_uuids(device, &device->sdp_uuids, uuids);
>>> +}
>>> +
>>> +static bool device_add_gatt_uuid(struct btd_device *device, const char *uuid)
>>> +{
>>> +       return device_add_uuid(&device->gatt_uuids, uuid);
>>> +}
>>> +
>>> +static bool device_add_gatt_uuids(struct btd_device *device, GSList *uuids)
>>> +{
>>> +       return device_add_uuids(device, &device->gatt_uuids, uuids);
>>> +}
>>> +
>>> +static void device_remove_gatt_uuid(struct btd_device *device, const char *uuid)
>>> +{
>>> +       GSList *l;
>>> +
>>> +       l = g_slist_find_custom(device->gatt_uuids, uuid, bt_uuid_strcmp);
>>> +       if (!l)
>>> +               return;
>>> +
>>> +       g_free(l->data);
>>> +       device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
>>> +}
>>> +
>>>  static void load_info(struct btd_device *device, const char *local,
>>>                         const char *peer, GKeyFile *key_file)
>>>  {
>>> @@ -2254,30 +2353,40 @@ next:
>>>         if (blocked)
>>>                 device_block(device, FALSE);
>>>
>>> -       /* Load device profile list */
>>> -       uuids = g_key_file_get_string_list(key_file, "General", "Services",
>>> -                                               NULL, NULL);
>>> +       /* Load classic device profile list */
>>> +       uuids = g_key_file_get_string_list(key_file, "General", "SDPServices",
>>> +                                                               NULL, NULL);
>>>         if (uuids) {
>>>                 char **uuid;
>>>
>>> -               for (uuid = uuids; *uuid; uuid++) {
>>> -                       GSList *match;
>>> +               for (uuid = uuids; *uuid; uuid++)
>>> +                       device_add_sdp_uuid(device, *uuid);
>>>
>>> -                       match = g_slist_find_custom(device->uuids, *uuid,
>>> -                                                       bt_uuid_strcmp);
>>> -                       if (match)
>>> -                               continue;
>>> -
>>> -                       device->uuids = g_slist_insert_sorted(device->uuids,
>>> -                                                               g_strdup(*uuid),
>>> -                                                               bt_uuid_strcmp);
>>> -               }
>>>                 g_strfreev(uuids);
>>>
>>>                 /* Discovered services restored from storage */
>>>                 device->bredr_state.svc_resolved = true;
>>>         }
>>>
>>> +       /* Load GATT-based device profile list */
>>> +       uuids = g_key_file_get_string_list(key_file, "General", "GATTServices",
>>> +                                                               NULL, NULL);
>>> +       if (uuids) {
>>> +               char **uuid;
>>> +
>>> +               for (uuid = uuids; *uuid; uuid++)
>>> +                       device_add_gatt_uuid(device, *uuid);
>>> +
>>> +               g_strfreev(uuids);
>>> +
>>> +               /*
>>> +                * TODO: The GATT-based service UUIDs have been restored from
>>> +                * storage but we don't have a populated gatt-db yet. Here we
>>> +                * should mark le_state.svc_resolved as true, if we're bonded
>>> +                * and we have a populated gatt_db.
>>> +                */
>>> +       }
>>> +
>>>         /* Load device id */
>>>         source = g_key_file_get_integer(key_file, "DeviceID", "Source", NULL);
>>>         if (source) {
>>> @@ -2411,34 +2520,13 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data)
>>>         *new_services = g_slist_append(*new_services, prim);
>>>  }
>>>
>>> -static void device_add_uuids(struct btd_device *device, GSList *uuids)
>>> -{
>>> -       GSList *l;
>>> -       bool changed = false;
>>> -
>>> -       for (l = uuids; l != NULL; l = g_slist_next(l)) {
>>> -               GSList *match = g_slist_find_custom(device->uuids, l->data,
>>> -                                                       bt_uuid_strcmp);
>>> -               if (match)
>>> -                       continue;
>>> -
>>> -               changed = true;
>>> -               device->uuids = g_slist_insert_sorted(device->uuids,
>>> -                                               g_strdup(l->data),
>>> -                                               bt_uuid_strcmp);
>>> -       }
>>> -
>>> -       if (changed)
>>> -               g_dbus_emit_property_changed(dbus_conn, device->path,
>>> -                                               DEVICE_INTERFACE, "UUIDs");
>>> -}
>>> -
>>>  static void store_services(struct btd_device *device);
>>>
>>>  struct gatt_probe_data {
>>>         struct btd_device *dev;
>>>         bool all_services;
>>> -       GSList *uuids;
>>> +       GSList *new_uuids;
>>> +       GSList *old_uuids;
>>>         struct gatt_db_attribute *cur_attr;
>>>         char cur_uuid[MAX_LEN_UUID_STR];
>>>  };
>>> @@ -2488,20 +2576,31 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>>>  {
>>>         struct gatt_probe_data *data = user_data;
>>>         bt_uuid_t uuid;
>>> -       GSList *l = NULL;
>>> +       gpointer dup_uuid;
>>>
>>>         gatt_db_attribute_get_service_uuid(attr, &uuid);
>>>         bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid));
>>>
>>>         data->cur_attr = attr;
>>>
>>> -       /*
>>> -        * If we're probing for all services, store the UUID since device->uuids
>>> -        * was cleared.
>>> -        */
>>> -       if (data->all_services)
>>> -               data->uuids = g_slist_append(data->uuids,
>>> +       if (data->all_services) {
>>> +               GSList *l;
>>> +
>>> +               /*
>>> +                * Check if the service is already in the old UUIDs list. If so,
>>> +                * remove it.
>>> +                */
>>> +               l = g_slist_find_custom(data->old_uuids, data->cur_uuid,
>>> +                                                               bt_uuid_strcmp);
>>> +               if (l) {
>>> +                       g_free(l->data);
>>> +                       data->old_uuids = g_slist_delete_link(data->old_uuids,
>>> +                                                                       l);
>>> +               } else {
>>> +                       data->new_uuids = g_slist_append(data->new_uuids,
>>>                                                 g_strdup(data->cur_uuid));
>>> +               }
>>> +       }
>>>
>>>         /* Don't probe the profiles if a matching service already exists. */
>>>         if (find_service_with_uuid(data->dev->services, data->cur_uuid))
>>> @@ -2512,8 +2611,14 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>>>         if (data->all_services)
>>>                 return;
>>>
>>> -       l = g_slist_append(l, g_strdup(data->cur_uuid));
>>> -       device_add_uuids(data->dev, l);
>>> +       dup_uuid = g_strdup(data->cur_uuid);
>>> +       if (!device_add_gatt_uuid(data->dev, dup_uuid)) {
>>> +               g_free(dup_uuid);
>>> +               return;
>>> +       }
>>> +
>>> +       g_dbus_emit_property_changed(dbus_conn, data->dev->path,
>>> +                                               DEVICE_INTERFACE, "UUIDs");
>>>  }
>>>
>>>  static void device_probe_gatt_profile(struct btd_device *device,
>>> @@ -2526,33 +2631,25 @@ static void device_probe_gatt_profile(struct btd_device *device,
>>>         data.dev = device;
>>>
>>>         dev_probe_gatt_profile(attr, &data);
>>> -       g_slist_free_full(data.uuids, g_free);
>>>  }
>>>
>>>  static void remove_invalid_services(struct gatt_probe_data *data)
>>>  {
>>>         struct btd_device *dev = data->dev;
>>>         struct btd_service *service;
>>> -       struct btd_profile *profile;
>>> -       GSList *l, *tmp;
>>> +       GSList *l, *svc;
>>>
>>> -       for (l = dev->services; l != NULL;) {
>>> -               service = l->data;
>>> -               profile = btd_service_get_profile(service);
>>> +       for (l = data->old_uuids; l; l = g_slist_next(l)) {
>>> +               device_remove_gatt_uuid(dev, l->data);
>>>
>>> -               if (g_slist_find_custom(dev->uuids, profile->remote_uuid,
>>> -                                                       bt_uuid_strcmp)) {
>>> -                       l = g_slist_next(l);
>>> +               svc = find_service_with_uuid(dev->services, l->data);
>>> +               if (!svc)
>>>                         continue;
>>> -               }
>>>
>>> -               /* Service no longer valid, so remove it */
>>> -               tmp = l->next;
>>> -               dev->services = g_slist_delete_link(dev->services, l);
>>> +               service = svc->data;
>>> +               dev->services = g_slist_delete_link(dev->services, svc);
>>>                 dev->pending = g_slist_remove(dev->pending, service);
>>>                 service_remove(service);
>>> -
>>> -               l = tmp;
>>>         }
>>>  }
>>>
>>> @@ -2560,6 +2657,7 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>>>  {
>>>         struct gatt_probe_data data;
>>>         char addr[18];
>>> +       GSList *l;
>>>
>>>         ba2str(&device->bdaddr, addr);
>>>
>>> @@ -2573,23 +2671,24 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>>>         data.dev = device;
>>>         data.all_services = true;
>>>
>>> +       /* Copy current list of GATT UUIDs */
>>> +       for (l = device->gatt_uuids; l; l = g_slist_next(l))
>>> +               data.old_uuids = g_slist_append(data.old_uuids,
>>> +                                                       g_strdup(l->data));
>>>
>>>         gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile,
>>>                                                                         &data);
>>>
>>> -       /*
>>> -        * Clear the UUIDs list
>>> -        *
>>> -        * FIXME: The management of UUIDs here iss currently broken, as clearing
>>> -        * the entire list here will likely remove UUIDs that shouldn't be
>>> -        * removed (e.g. those obtained via SDP for a dual-mode device).
>>> -        */
>>> -       g_slist_free_full(device->uuids, g_free);
>>> -       device->uuids = NULL;
>>> +       /* Whatever remains in data.old_uuids is stale and should be removed */
>>> +       remove_invalid_services(&data);
>>>
>>> -       device_add_uuids(device, data.uuids);
>>> -       g_slist_free_full(data.uuids, g_free);
>>> +       /* Update the list with new UUIDs */
>>> +       if (!device_add_gatt_uuids(device, data.new_uuids) && data.old_uuids)
>>> +               g_dbus_emit_property_changed(dbus_conn, device->path,
>>> +                                               DEVICE_INTERFACE, "UUIDs");
>>>
>>> -       remove_invalid_services(&data);
>>> +       g_slist_free_full(data.new_uuids, g_free);
>>> +       g_slist_free_full(data.old_uuids, g_free);
>>>  }
>>>
>>>  static void device_accept_gatt_profiles(struct btd_device *device)
>>> @@ -2675,46 +2774,39 @@ static gint prim_attr_cmp(gconstpointer a, gconstpointer b)
>>>         return !(prim->range.start == start && prim->range.end == end);
>>>  }
>>>
>>> -static gint prim_uuid_cmp(gconstpointer a, gconstpointer b)
>>> -{
>>> -       const struct gatt_primary *prim = a;
>>> -       const char *uuid = b;
>>> -
>>> -       return bt_uuid_strcmp(prim->uuid, uuid);
>>> -}
>>> -
>>>  static void gatt_service_removed(struct gatt_db_attribute *attr,
>>>                                                                 void *user_data)
>>>  {
>>>         struct btd_device *device = user_data;
>>>         GSList *l;
>>> -       struct gatt_primary *prim;
>>>         uint16_t start, end;
>>> +       bt_uuid_t uuid;
>>> +       char uuid_str[MAX_LEN_UUID_STR];
>>>
>>>         if (!bt_gatt_client_is_ready(device->client))
>>>                 return;
>>>
>>> -       gatt_db_attribute_get_service_handles(attr, &start, &end);
>>> +       gatt_db_attribute_get_service_data(attr, &start, &end, NULL, &uuid);
>>>
>>>         DBG("start: 0x%04x, end: 0x%04x", start, end);
>>>
>>>         /* Remove the corresponding gatt_primary */
>>>         l = g_slist_find_custom(device->primaries, attr, prim_attr_cmp);
>>> -       if (!l)
>>> -               return;
>>> +       if (l) {
>>> +               g_free(l->data);
>>> +               device->primaries = g_slist_delete_link(device->primaries, l);
>>> +       }
>>>
>>> -       prim = l->data;
>>> -       device->primaries = g_slist_delete_link(device->primaries, l);
>>> +       bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
>>>
>>>         /* Get the UUID entry to be removed below */
>>> -       l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
>>> +       l = g_slist_find_custom(device->gatt_uuids, uuid_str, bt_uuid_strcmp);
>>>
>>>         /*
>>>          * Remove the corresponding UUIDs entry and the profile, only if this
>>>          * is the last service with this UUID.
>>>          */
>>> -       if (!g_slist_find_custom(device->primaries, prim->uuid,
>>> -                                                       prim_uuid_cmp)) {
>>> +       if (!gatt_db_get_service_with_uuid(device->db, &uuid)) {
>>>                 /*
>>>                  * If this happened since the db was cleared for a non-bonded
>>>                  * device, then don't remove the btd_service just yet. We do
>>> @@ -2727,13 +2819,11 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
>>>                         device_remove_gatt_profile(device, attr);
>>>
>>>                 g_free(l->data);
>>> -               device->uuids = g_slist_delete_link(device->uuids, l);
>>> +               device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
>>>                 g_dbus_emit_property_changed(dbus_conn, device->path,
>>>                                                 DEVICE_INTERFACE, "UUIDs");
>>>         }
>>>
>>> -       g_free(prim);
>>> -
>>>         store_device_info(device);
>>>
>>>         btd_gatt_client_service_removed(device->client_dbus, attr);
>>> @@ -2988,8 +3078,13 @@ void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup)
>>>         dev->trusted = dup->trusted;
>>>         dev->blocked = dup->blocked;
>>>
>>> -       for (l = dup->uuids; l; l = g_slist_next(l))
>>> -               dev->uuids = g_slist_append(dev->uuids, g_strdup(l->data));
>>> +       for (l = dup->sdp_uuids; l; l = g_slist_next(l))
>>> +               dev->sdp_uuids = g_slist_append(dev->sdp_uuids,
>>> +                                                       g_strdup(l->data));
>>> +
>>> +       for (l = dup->gatt_uuids; l; l = g_slist_next(l))
>>> +               dev->gatt_uuids = g_slist_append(dev->gatt_uuids,
>>> +                                                       g_strdup(l->data));
>>>
>>>         if (dev->name[0] == '\0')
>>>                 strcpy(dev->name, dup->name);
>>> @@ -3240,9 +3335,14 @@ static gboolean record_has_uuid(const sdp_record_t *rec,
>>>         return FALSE;
>>>  }
>>>
>>> -GSList *btd_device_get_uuids(struct btd_device *device)
>>> +GSList *btd_device_get_sdp_uuids(struct btd_device *device)
>>>  {
>>> -       return device->uuids;
>>> +       return device->sdp_uuids;
>>> +}
>>> +
>>> +GSList *btd_device_get_gatt_uuids(struct btd_device *device)
>>> +{
>>> +       return device->gatt_uuids;
>>>  }
>>>
>>>  struct probe_data {
>>> @@ -3280,7 +3380,8 @@ void device_probe_profile(gpointer a, gpointer b)
>>>         if (profile->device_probe == NULL)
>>>                 return;
>>>
>>> -       if (!device_match_profile(device, profile, device->uuids))
>>> +       if (!device_match_profile(device, profile, device->sdp_uuids) &&
>>> +               !device_match_profile(device, profile, device->gatt_uuids))
>>>                 return;
>>>
>>>         service = service_create(device, profile);
>>> @@ -3318,7 +3419,8 @@ void device_remove_profile(gpointer a, gpointer b)
>>>         service_remove(service);
>>>  }
>>>
>>> -void device_probe_profiles(struct btd_device *device, GSList *uuids)
>>> +void device_probe_profiles(struct btd_device *device, GSList *uuids,
>>> +                                                               bool gatt)
>>>  {
>>>         struct probe_data d = { device, uuids };
>>>         char addr[18];
>>> @@ -3335,7 +3437,10 @@ void device_probe_profiles(struct btd_device *device, GSList *uuids)
>>>         btd_profile_foreach(dev_probe, &d);
>>>
>>>  add_uuids:
>>> -       device_add_uuids(device, uuids);
>>> +       if (gatt)
>>> +               device_add_gatt_uuids(device, uuids);
>>> +       else
>>> +               device_add_sdp_uuids(device, uuids);
>>>  }
>>>
>>>  static void store_sdp_record(GKeyFile *key_file, sdp_record_t *rec)
>>> @@ -3431,7 +3536,7 @@ static int update_record(struct browse_req *req, const char *uuid,
>>>         req->records = sdp_list_append(req->records, sdp_copy_record(rec));
>>>
>>>         /* Check if UUID is duplicated */
>>> -       l = g_slist_find_custom(req->device->uuids, uuid, bt_uuid_strcmp);
>>> +       l = g_slist_find_custom(req->device->sdp_uuids, uuid, bt_uuid_strcmp);
>>>         if (l == NULL) {
>>>                 l = g_slist_find_custom(req->profiles_added, uuid,
>>>                                                         bt_uuid_strcmp);
>>> @@ -3662,7 +3767,7 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
>>>          * the full list of services and populate a client-role gatt_db over
>>>          * BR/EDR.
>>>          */
>>> -       device_probe_profiles(device, req->profiles_added);
>>> +       device_probe_profiles(device, req->profiles_added, false);
>>>
>>>         /* Propagate services changes */
>>>         g_dbus_emit_property_changed(dbus_conn, req->device->path,
>>> @@ -3927,6 +4032,8 @@ static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
>>>
>>>         if (device->browse)
>>>                 register_gatt_services(device->browse);
>>> +       else
>>> +               device_probe_gatt_profiles(device);
>>>
>>>         device_accept_gatt_profiles(device);
>>>
>>> @@ -5127,13 +5234,13 @@ void btd_device_add_uuid(struct btd_device *device, const char *uuid)
>>>         GSList *uuid_list;
>>>         char *new_uuid;
>>>
>>> -       if (g_slist_find_custom(device->uuids, uuid, bt_uuid_strcmp))
>>> +       if (g_slist_find_custom(device->sdp_uuids, uuid, bt_uuid_strcmp))
>>>                 return;
>>>
>>>         new_uuid = g_strdup(uuid);
>>>         uuid_list = g_slist_append(NULL, new_uuid);
>>>
>>> -       device_probe_profiles(device, uuid_list);
>>> +       device_probe_profiles(device, uuid_list, false);
>>>
>>>         g_free(new_uuid);
>>>         g_slist_free(uuid_list);
>>> diff --git a/src/device.h b/src/device.h
>>> index a7fefee..f250bfa 100644
>>> --- a/src/device.h
>>> +++ b/src/device.h
>>> @@ -60,8 +60,10 @@ struct device_addr_type {
>>>  };
>>>
>>>  int device_addr_type_cmp(gconstpointer a, gconstpointer b);
>>> -GSList *btd_device_get_uuids(struct btd_device *device);
>>> -void device_probe_profiles(struct btd_device *device, GSList *profiles);
>>> +GSList *btd_device_get_sdp_uuids(struct btd_device *device);
>>> +GSList *btd_device_get_gatt_uuids(struct btd_device *device);
>>> +void device_probe_profiles(struct btd_device *device, GSList *profiles,
>>> +                                                               bool gatt);
>>
>> Id leave this API as it is for now and internally classify the UUIDs,
>> the classification function can be used when loading from the storage
>> that way we don't need to break the storage format.
>>
>
> Well, I'm just not sure how to do that classification. Especially in
> the case of device_probe_profiles, which gets passed in a list of
> UUIDs and the case where the UUIDs get loaded from storage, I don't
> know how we can figure out which ones are LE and which ones are
> BR/EDR, unless we do something complicated/inefficient like going
> through the gatt-db and see if there's a matching service for each
> UUID but this still becomes a problem when we load the UUIDs before
> having done discovery (i.e. there's no populated gatt-db).
>
>>>  const sdp_record_t *btd_device_get_record(struct btd_device *device,
>>>                                                 const char *uuid);
>>>  struct gatt_primary *btd_device_get_primary(struct btd_device *device,
>>> --
>>> 2.2.0.rc0.207.ga3a616c
>>>
>>> --
>>> 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  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> Thanks,
> Arman

ping

Re: [PATCH BlueZ v3 3/8] core: device: Fix broken GATT UUID management

[Luiz Augusto von Dentz]

Hi Arman,

On Thu, Jan 15, 2015 at 10:58 PM, Arman Uguray <armansito@chromium.org> wrote:
>> On Wed, Jan 14, 2015 at 11:38 AM, Arman Uguray <armansito@chromium.org> wrote:
>> Hi Luiz,
>>
>>> On Wed, Jan 14, 2015 at 5:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>> Hi Arman,
>>>
>>> On Wed, Jan 14, 2015 at 1:31 AM, Arman Uguray <armansito@chromium.org> wrote:
>>>> This patch fixes the broken GATT UUID management, which incorrectly
>>>> remove valid UUIDs during the GATT profile probe process. This is
>>>> achieved by splitting UUIDs obtained via GATT and SDP into their own
>>>> lists that are internal to btd_device.
>>>> ---
>>>>  plugins/sixaxis.c |   2 +-
>>>>  src/adapter.c     |   8 +-
>>>>  src/device.c      | 367 +++++++++++++++++++++++++++++++++++-------------------
>>>>  src/device.h      |   6 +-
>>>>  4 files changed, 248 insertions(+), 135 deletions(-)
>>>>
>>>> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
>>>> index ac53ba9..0a4e3d8 100644
>>>> --- a/plugins/sixaxis.c
>>>> +++ b/plugins/sixaxis.c
>>>> @@ -289,7 +289,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>>>>
>>>>         device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
>>>>
>>>> -       if (g_slist_find_custom(btd_device_get_uuids(device), HID_UUID,
>>>> +       if (g_slist_find_custom(btd_device_get_sdp_uuids(device), HID_UUID,
>>>>                                                 (GCompareFunc)strcasecmp)) {
>>>>                 DBG("device %s already known, skipping", device_addr);
>>>>                 return true;
>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>> index d7e2550..379a80b 100644
>>>> --- a/src/adapter.c
>>>> +++ b/src/adapter.c
>>>> @@ -2912,9 +2912,13 @@ static void load_devices(struct btd_adapter *adapter)
>>>>
>>>>                 /* TODO: register services from pre-loaded list of primaries */
>>>>
>>>> -               list = btd_device_get_uuids(device);
>>>> +               list = btd_device_get_sdp_uuids(device);
>>>>                 if (list)
>>>> -                       device_probe_profiles(device, list);
>>>> +                       device_probe_profiles(device, list, false);
>>>> +
>>>> +               list = btd_device_get_gatt_uuids(device);
>>>> +               if (list)
>>>> +                       device_probe_profiles(device, list, true);
>>>>
>>>>  device_exist:
>>>>                 if (key_info) {
>>>> diff --git a/src/device.c b/src/device.c
>>>> index 4f702ba..9d1a480 100644
>>>> --- a/src/device.c
>>>> +++ b/src/device.c
>>>> @@ -193,7 +193,8 @@ struct btd_device {
>>>>         uint16_t        appearance;
>>>>         char            *modalias;
>>>>         struct btd_adapter      *adapter;
>>>> -       GSList          *uuids;
>>>> +       GSList          *sdp_uuids;
>>>> +       GSList          *gatt_uuids;
>>>>         GSList          *primaries;             /* List of primary services */
>>>>         GSList          *services;              /* List of btd_service */
>>>>         GSList          *pending;               /* Pending services */
>>>> @@ -335,6 +336,28 @@ static void update_technologies(GKeyFile *file, struct btd_device *dev)
>>>>                                                                 list, len);
>>>>  }
>>>>
>>>> +static char **store_service_uuids(GKeyFile *key_file, gchar *key, GSList *uuids)
>>>> +{
>>>> +       GSList *l;
>>>> +       char **array;
>>>> +       int i;
>>>> +
>>>> +       if (!uuids) {
>>>> +               g_key_file_remove_key(key_file, "General", key, NULL);
>>>> +               return NULL;
>>>> +       }
>>>> +
>>>> +       array = g_new0(char *, g_slist_length(uuids) + 1);
>>>> +
>>>> +       for (i = 0, l = uuids; l; l = g_slist_next(l), i++)
>>>> +               array[i] = l->data;
>>>> +
>>>> +       g_key_file_set_string_list(key_file, "General", key,
>>>> +                                               (const char **)array, i);
>>>> +
>>>> +       return array;
>>>> +}
>>>> +
>>>>  static gboolean store_device_info_cb(gpointer user_data)
>>>>  {
>>>>         struct btd_device *device = user_data;
>>>> @@ -344,7 +367,7 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>>         char device_addr[18];
>>>>         char *str;
>>>>         char class[9];
>>>> -       char **uuids = NULL;
>>>> +       char **sdp_uuids, **gatt_uuids;
>>>>         gsize length = 0;
>>>>
>>>>         device->store_id = 0;
>>>> @@ -387,18 +410,10 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>>         g_key_file_set_boolean(key_file, "General", "Blocked",
>>>>                                                         device->blocked);
>>>>
>>>> -       if (device->uuids) {
>>>> -               GSList *l;
>>>> -               int i;
>>>> -
>>>> -               uuids = g_new0(char *, g_slist_length(device->uuids) + 1);
>>>> -               for (i = 0, l = device->uuids; l; l = g_slist_next(l), i++)
>>>> -                       uuids[i] = l->data;
>>>> -               g_key_file_set_string_list(key_file, "General", "Services",
>>>> -                                               (const char **)uuids, i);
>>>> -       } else {
>>>> -               g_key_file_remove_key(key_file, "General", "Services", NULL);
>>>> -       }
>>>> +       sdp_uuids = store_service_uuids(key_file, "SDPServices",
>>>> +                                                       device->sdp_uuids);
>>>> +       gatt_uuids = store_service_uuids(key_file, "GATTServices",
>>>> +                                                       device->gatt_uuids);
>>>
>>> This change is not backward compatible which mean existing storage
>>> will not load properly if we change the storage fields, furthermore I
>>> think it is better to store the UUIDs per bearer_state since service
>>> discovered flag is stored there it seem logical to couple UUIDs founds
>>> there as well.
>>>
>>
>> You're right, I'm aware that this is not backwards compatible. I
>> didn't build any migration mechanism from the UUIDs field to the new
>> fields to sort of initiate this discussion with you. I'm still trying
>> to figure out how to resolve the current format. See my comments
>> below.
>>
>>>>         if (device->vendor_src) {
>>>>                 g_key_file_set_integer(key_file, "DeviceID", "Source",
>>>> @@ -420,7 +435,8 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>>         g_free(str);
>>>>
>>>>         g_key_file_free(key_file);
>>>> -       g_free(uuids);
>>>> +       g_free(sdp_uuids);
>>>> +       g_free(gatt_uuids);
>>>>
>>>>         return FALSE;
>>>>  }
>>>> @@ -574,7 +590,8 @@ static void device_free(gpointer user_data)
>>>>         btd_gatt_client_destroy(device->client_dbus);
>>>>         device->client_dbus = NULL;
>>>>
>>>> -       g_slist_free_full(device->uuids, g_free);
>>>> +       g_slist_free_full(device->sdp_uuids, g_free);
>>>> +       g_slist_free_full(device->gatt_uuids, g_free);
>>>>         g_slist_free_full(device->primaries, g_free);
>>>>         g_slist_free_full(device->attios, g_free);
>>>>         g_slist_free_full(device->attios_offline, g_free);
>>>> @@ -988,22 +1005,43 @@ static gboolean dev_property_get_uuids(const GDBusPropertyTable *property,
>>>>  {
>>>>         struct btd_device *dev = data;
>>>>         DBusMessageIter entry;
>>>> -       GSList *l;
>>>> +       GHashTable *uuids = NULL;
>>>> +       GSList *sl;
>>>> +       GList *keys, *l;
>>>>
>>>>         dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>>>                                 DBUS_TYPE_STRING_AS_STRING, &entry);
>>>>
>>>> -       if (dev->bredr_state.svc_resolved || dev->le_state.svc_resolved)
>>>> -               l = dev->uuids;
>>>> -       else if (dev->eir_uuids)
>>>> -               l = dev->eir_uuids;
>>>> -       else
>>>> -               l = dev->uuids;
>>>> +       if (!dev->bredr_state.svc_resolved && dev->le_state.svc_resolved &&
>>>> +                                                       dev->eir_uuids) {
>>>> +               for (sl = dev->eir_uuids; sl; sl = g_slist_next(sl))
>>>> +                       dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
>>>> +                                                               &sl->data);
>>>> +
>>>> +               goto done;
>>>> +       }
>>>> +
>>>> +       /* Insert GATT and SDP UUIDs into a set to prevent any duplicates */
>>>> +       uuids = g_hash_table_new(g_str_hash, bt_uuid_strcmp);
>>>>
>>>> -       for (; l != NULL; l = l->next)
>>>> +       for (sl = dev->sdp_uuids; sl; sl = g_slist_next(sl))
>>>> +               g_hash_table_add(uuids, sl->data);
>>>> +
>>>> +       for (sl = dev->gatt_uuids; sl; sl = g_slist_next(sl))
>>>> +               g_hash_table_add(uuids, sl->data);
>>>> +
>>>> +       /* Obtain a list of UUIDs and sort it */
>>>> +       keys = g_hash_table_get_keys(uuids);
>>>> +       keys = g_list_sort(keys, bt_uuid_strcmp);
>>>> +
>>>> +       for (l = keys; l; l = g_list_next(l))
>>>>                 dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
>>>> -                                                       &l->data);
>>>> +                                                               &l->data);
>>>> +
>>>> +       g_list_free(keys);
>>>> +       g_hash_table_destroy(uuids);
>>>>
>>>> +done:
>>>>         dbus_message_iter_close_container(iter, &entry);
>>>>
>>>>         return TRUE;
>>>> @@ -1130,7 +1168,8 @@ int device_unblock(struct btd_device *device, gboolean silent,
>>>>         if (!silent) {
>>>>                 g_dbus_emit_property_changed(dbus_conn, device->path,
>>>>                                                 DEVICE_INTERFACE, "Blocked");
>>>> -               device_probe_profiles(device, device->uuids);
>>>> +               device_probe_profiles(device, device->sdp_uuids, false);
>>>> +               device_probe_profiles(device, device->gatt_uuids, true);
>>>>         }
>>>>
>>>>         return 0;
>>>> @@ -2165,6 +2204,66 @@ failed:
>>>>         return str;
>>>>  }
>>>>
>>>> +static bool device_add_uuid(GSList **uuids, const char *uuid)
>>>> +{
>>>> +       if (g_slist_find_custom(*uuids, uuid, bt_uuid_strcmp))
>>>> +               return false;
>>>> +
>>>> +       *uuids = g_slist_insert_sorted(*uuids, g_strdup(uuid), bt_uuid_strcmp);
>>>> +
>>>> +       return true;
>>>> +}
>>>> +
>>>> +static bool device_add_uuids(struct btd_device *device, GSList **uuids,
>>>> +                                                       GSList *new_uuids)
>>>> +{
>>>> +       GSList *l;
>>>> +       bool changed = false;
>>>> +
>>>> +       for (l = new_uuids; l; l = g_slist_next(l)) {
>>>> +               if (device_add_uuid(uuids, l->data))
>>>> +                       changed = true;
>>>> +       }
>>>> +
>>>> +       if (changed)
>>>> +               g_dbus_emit_property_changed(dbus_conn, device->path,
>>>> +                                               DEVICE_INTERFACE, "UUIDs");
>>>> +
>>>> +       return changed;
>>>> +}
>>>> +
>>>> +static bool device_add_sdp_uuid(struct btd_device *device, const char *uuid)
>>>> +{
>>>> +       return device_add_uuid(&device->sdp_uuids, uuid);
>>>> +}
>>>> +
>>>> +static bool device_add_sdp_uuids(struct btd_device *device, GSList *uuids)
>>>> +{
>>>> +       return device_add_uuids(device, &device->sdp_uuids, uuids);
>>>> +}
>>>> +
>>>> +static bool device_add_gatt_uuid(struct btd_device *device, const char *uuid)
>>>> +{
>>>> +       return device_add_uuid(&device->gatt_uuids, uuid);
>>>> +}
>>>> +
>>>> +static bool device_add_gatt_uuids(struct btd_device *device, GSList *uuids)
>>>> +{
>>>> +       return device_add_uuids(device, &device->gatt_uuids, uuids);
>>>> +}
>>>> +
>>>> +static void device_remove_gatt_uuid(struct btd_device *device, const char *uuid)
>>>> +{
>>>> +       GSList *l;
>>>> +
>>>> +       l = g_slist_find_custom(device->gatt_uuids, uuid, bt_uuid_strcmp);
>>>> +       if (!l)
>>>> +               return;
>>>> +
>>>> +       g_free(l->data);
>>>> +       device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
>>>> +}
>>>> +
>>>>  static void load_info(struct btd_device *device, const char *local,
>>>>                         const char *peer, GKeyFile *key_file)
>>>>  {
>>>> @@ -2254,30 +2353,40 @@ next:
>>>>         if (blocked)
>>>>                 device_block(device, FALSE);
>>>>
>>>> -       /* Load device profile list */
>>>> -       uuids = g_key_file_get_string_list(key_file, "General", "Services",
>>>> -                                               NULL, NULL);
>>>> +       /* Load classic device profile list */
>>>> +       uuids = g_key_file_get_string_list(key_file, "General", "SDPServices",
>>>> +                                                               NULL, NULL);
>>>>         if (uuids) {
>>>>                 char **uuid;
>>>>
>>>> -               for (uuid = uuids; *uuid; uuid++) {
>>>> -                       GSList *match;
>>>> +               for (uuid = uuids; *uuid; uuid++)
>>>> +                       device_add_sdp_uuid(device, *uuid);
>>>>
>>>> -                       match = g_slist_find_custom(device->uuids, *uuid,
>>>> -                                                       bt_uuid_strcmp);
>>>> -                       if (match)
>>>> -                               continue;
>>>> -
>>>> -                       device->uuids = g_slist_insert_sorted(device->uuids,
>>>> -                                                               g_strdup(*uuid),
>>>> -                                                               bt_uuid_strcmp);
>>>> -               }
>>>>                 g_strfreev(uuids);
>>>>
>>>>                 /* Discovered services restored from storage */
>>>>                 device->bredr_state.svc_resolved = true;
>>>>         }
>>>>
>>>> +       /* Load GATT-based device profile list */
>>>> +       uuids = g_key_file_get_string_list(key_file, "General", "GATTServices",
>>>> +                                                               NULL, NULL);
>>>> +       if (uuids) {
>>>> +               char **uuid;
>>>> +
>>>> +               for (uuid = uuids; *uuid; uuid++)
>>>> +                       device_add_gatt_uuid(device, *uuid);
>>>> +
>>>> +               g_strfreev(uuids);
>>>> +
>>>> +               /*
>>>> +                * TODO: The GATT-based service UUIDs have been restored from
>>>> +                * storage but we don't have a populated gatt-db yet. Here we
>>>> +                * should mark le_state.svc_resolved as true, if we're bonded
>>>> +                * and we have a populated gatt_db.
>>>> +                */
>>>> +       }
>>>> +
>>>>         /* Load device id */
>>>>         source = g_key_file_get_integer(key_file, "DeviceID", "Source", NULL);
>>>>         if (source) {
>>>> @@ -2411,34 +2520,13 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data)
>>>>         *new_services = g_slist_append(*new_services, prim);
>>>>  }
>>>>
>>>> -static void device_add_uuids(struct btd_device *device, GSList *uuids)
>>>> -{
>>>> -       GSList *l;
>>>> -       bool changed = false;
>>>> -
>>>> -       for (l = uuids; l != NULL; l = g_slist_next(l)) {
>>>> -               GSList *match = g_slist_find_custom(device->uuids, l->data,
>>>> -                                                       bt_uuid_strcmp);
>>>> -               if (match)
>>>> -                       continue;
>>>> -
>>>> -               changed = true;
>>>> -               device->uuids = g_slist_insert_sorted(device->uuids,
>>>> -                                               g_strdup(l->data),
>>>> -                                               bt_uuid_strcmp);
>>>> -       }
>>>> -
>>>> -       if (changed)
>>>> -               g_dbus_emit_property_changed(dbus_conn, device->path,
>>>> -                                               DEVICE_INTERFACE, "UUIDs");
>>>> -}
>>>> -
>>>>  static void store_services(struct btd_device *device);
>>>>
>>>>  struct gatt_probe_data {
>>>>         struct btd_device *dev;
>>>>         bool all_services;
>>>> -       GSList *uuids;
>>>> +       GSList *new_uuids;
>>>> +       GSList *old_uuids;
>>>>         struct gatt_db_attribute *cur_attr;
>>>>         char cur_uuid[MAX_LEN_UUID_STR];
>>>>  };
>>>> @@ -2488,20 +2576,31 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>>>>  {
>>>>         struct gatt_probe_data *data = user_data;
>>>>         bt_uuid_t uuid;
>>>> -       GSList *l = NULL;
>>>> +       gpointer dup_uuid;
>>>>
>>>>         gatt_db_attribute_get_service_uuid(attr, &uuid);
>>>>         bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid));
>>>>
>>>>         data->cur_attr = attr;
>>>>
>>>> -       /*
>>>> -        * If we're probing for all services, store the UUID since device->uuids
>>>> -        * was cleared.
>>>> -        */
>>>> -       if (data->all_services)
>>>> -               data->uuids = g_slist_append(data->uuids,
>>>> +       if (data->all_services) {
>>>> +               GSList *l;
>>>> +
>>>> +               /*
>>>> +                * Check if the service is already in the old UUIDs list. If so,
>>>> +                * remove it.
>>>> +                */
>>>> +               l = g_slist_find_custom(data->old_uuids, data->cur_uuid,
>>>> +                                                               bt_uuid_strcmp);
>>>> +               if (l) {
>>>> +                       g_free(l->data);
>>>> +                       data->old_uuids = g_slist_delete_link(data->old_uuids,
>>>> +                                                                       l);
>>>> +               } else {
>>>> +                       data->new_uuids = g_slist_append(data->new_uuids,
>>>>                                                 g_strdup(data->cur_uuid));
>>>> +               }
>>>> +       }
>>>>
>>>>         /* Don't probe the profiles if a matching service already exists. */
>>>>         if (find_service_with_uuid(data->dev->services, data->cur_uuid))
>>>> @@ -2512,8 +2611,14 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>>>>         if (data->all_services)
>>>>                 return;
>>>>
>>>> -       l = g_slist_append(l, g_strdup(data->cur_uuid));
>>>> -       device_add_uuids(data->dev, l);
>>>> +       dup_uuid = g_strdup(data->cur_uuid);
>>>> +       if (!device_add_gatt_uuid(data->dev, dup_uuid)) {
>>>> +               g_free(dup_uuid);
>>>> +               return;
>>>> +       }
>>>> +
>>>> +       g_dbus_emit_property_changed(dbus_conn, data->dev->path,
>>>> +                                               DEVICE_INTERFACE, "UUIDs");
>>>>  }
>>>>
>>>>  static void device_probe_gatt_profile(struct btd_device *device,
>>>> @@ -2526,33 +2631,25 @@ static void device_probe_gatt_profile(struct btd_device *device,
>>>>         data.dev = device;
>>>>
>>>>         dev_probe_gatt_profile(attr, &data);
>>>> -       g_slist_free_full(data.uuids, g_free);
>>>>  }
>>>>
>>>>  static void remove_invalid_services(struct gatt_probe_data *data)
>>>>  {
>>>>         struct btd_device *dev = data->dev;
>>>>         struct btd_service *service;
>>>> -       struct btd_profile *profile;
>>>> -       GSList *l, *tmp;
>>>> +       GSList *l, *svc;
>>>>
>>>> -       for (l = dev->services; l != NULL;) {
>>>> -               service = l->data;
>>>> -               profile = btd_service_get_profile(service);
>>>> +       for (l = data->old_uuids; l; l = g_slist_next(l)) {
>>>> +               device_remove_gatt_uuid(dev, l->data);
>>>>
>>>> -               if (g_slist_find_custom(dev->uuids, profile->remote_uuid,
>>>> -                                                       bt_uuid_strcmp)) {
>>>> -                       l = g_slist_next(l);
>>>> +               svc = find_service_with_uuid(dev->services, l->data);
>>>> +               if (!svc)
>>>>                         continue;
>>>> -               }
>>>>
>>>> -               /* Service no longer valid, so remove it */
>>>> -               tmp = l->next;
>>>> -               dev->services = g_slist_delete_link(dev->services, l);
>>>> +               service = svc->data;
>>>> +               dev->services = g_slist_delete_link(dev->services, svc);
>>>>                 dev->pending = g_slist_remove(dev->pending, service);
>>>>                 service_remove(service);
>>>> -
>>>> -               l = tmp;
>>>>         }
>>>>  }
>>>>
>>>> @@ -2560,6 +2657,7 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>>>>  {
>>>>         struct gatt_probe_data data;
>>>>         char addr[18];
>>>> +       GSList *l;
>>>>
>>>>         ba2str(&device->bdaddr, addr);
>>>>
>>>> @@ -2573,23 +2671,24 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>>>>         data.dev = device;
>>>>         data.all_services = true;
>>>>
>>>> +       /* Copy current list of GATT UUIDs */
>>>> +       for (l = device->gatt_uuids; l; l = g_slist_next(l))
>>>> +               data.old_uuids = g_slist_append(data.old_uuids,
>>>> +                                                       g_strdup(l->data));
>>>>
>>>>         gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile,
>>>>                                                                         &data);
>>>>
>>>> -       /*
>>>> -        * Clear the UUIDs list
>>>> -        *
>>>> -        * FIXME: The management of UUIDs here iss currently broken, as clearing
>>>> -        * the entire list here will likely remove UUIDs that shouldn't be
>>>> -        * removed (e.g. those obtained via SDP for a dual-mode device).
>>>> -        */
>>>> -       g_slist_free_full(device->uuids, g_free);
>>>> -       device->uuids = NULL;
>>>> +       /* Whatever remains in data.old_uuids is stale and should be removed */
>>>> +       remove_invalid_services(&data);
>>>>
>>>> -       device_add_uuids(device, data.uuids);
>>>> -       g_slist_free_full(data.uuids, g_free);
>>>> +       /* Update the list with new UUIDs */
>>>> +       if (!device_add_gatt_uuids(device, data.new_uuids) && data.old_uuids)
>>>> +               g_dbus_emit_property_changed(dbus_conn, device->path,
>>>> +                                               DEVICE_INTERFACE, "UUIDs");
>>>>
>>>> -       remove_invalid_services(&data);
>>>> +       g_slist_free_full(data.new_uuids, g_free);
>>>> +       g_slist_free_full(data.old_uuids, g_free);
>>>>  }
>>>>
>>>>  static void device_accept_gatt_profiles(struct btd_device *device)
>>>> @@ -2675,46 +2774,39 @@ static gint prim_attr_cmp(gconstpointer a, gconstpointer b)
>>>>         return !(prim->range.start == start && prim->range.end == end);
>>>>  }
>>>>
>>>> -static gint prim_uuid_cmp(gconstpointer a, gconstpointer b)
>>>> -{
>>>> -       const struct gatt_primary *prim = a;
>>>> -       const char *uuid = b;
>>>> -
>>>> -       return bt_uuid_strcmp(prim->uuid, uuid);
>>>> -}
>>>> -
>>>>  static void gatt_service_removed(struct gatt_db_attribute *attr,
>>>>                                                                 void *user_data)
>>>>  {
>>>>         struct btd_device *device = user_data;
>>>>         GSList *l;
>>>> -       struct gatt_primary *prim;
>>>>         uint16_t start, end;
>>>> +       bt_uuid_t uuid;
>>>> +       char uuid_str[MAX_LEN_UUID_STR];
>>>>
>>>>         if (!bt_gatt_client_is_ready(device->client))
>>>>                 return;
>>>>
>>>> -       gatt_db_attribute_get_service_handles(attr, &start, &end);
>>>> +       gatt_db_attribute_get_service_data(attr, &start, &end, NULL, &uuid);
>>>>
>>>>         DBG("start: 0x%04x, end: 0x%04x", start, end);
>>>>
>>>>         /* Remove the corresponding gatt_primary */
>>>>         l = g_slist_find_custom(device->primaries, attr, prim_attr_cmp);
>>>> -       if (!l)
>>>> -               return;
>>>> +       if (l) {
>>>> +               g_free(l->data);
>>>> +               device->primaries = g_slist_delete_link(device->primaries, l);
>>>> +       }
>>>>
>>>> -       prim = l->data;
>>>> -       device->primaries = g_slist_delete_link(device->primaries, l);
>>>> +       bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
>>>>
>>>>         /* Get the UUID entry to be removed below */
>>>> -       l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
>>>> +       l = g_slist_find_custom(device->gatt_uuids, uuid_str, bt_uuid_strcmp);
>>>>
>>>>         /*
>>>>          * Remove the corresponding UUIDs entry and the profile, only if this
>>>>          * is the last service with this UUID.
>>>>          */
>>>> -       if (!g_slist_find_custom(device->primaries, prim->uuid,
>>>> -                                                       prim_uuid_cmp)) {
>>>> +       if (!gatt_db_get_service_with_uuid(device->db, &uuid)) {
>>>>                 /*
>>>>                  * If this happened since the db was cleared for a non-bonded
>>>>                  * device, then don't remove the btd_service just yet. We do
>>>> @@ -2727,13 +2819,11 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
>>>>                         device_remove_gatt_profile(device, attr);
>>>>
>>>>                 g_free(l->data);
>>>> -               device->uuids = g_slist_delete_link(device->uuids, l);
>>>> +               device->gatt_uuids = g_slist_delete_link(device->gatt_uuids, l);
>>>>                 g_dbus_emit_property_changed(dbus_conn, device->path,
>>>>                                                 DEVICE_INTERFACE, "UUIDs");
>>>>         }
>>>>
>>>> -       g_free(prim);
>>>> -
>>>>         store_device_info(device);
>>>>
>>>>         btd_gatt_client_service_removed(device->client_dbus, attr);
>>>> @@ -2988,8 +3078,13 @@ void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup)
>>>>         dev->trusted = dup->trusted;
>>>>         dev->blocked = dup->blocked;
>>>>
>>>> -       for (l = dup->uuids; l; l = g_slist_next(l))
>>>> -               dev->uuids = g_slist_append(dev->uuids, g_strdup(l->data));
>>>> +       for (l = dup->sdp_uuids; l; l = g_slist_next(l))
>>>> +               dev->sdp_uuids = g_slist_append(dev->sdp_uuids,
>>>> +                                                       g_strdup(l->data));
>>>> +
>>>> +       for (l = dup->gatt_uuids; l; l = g_slist_next(l))
>>>> +               dev->gatt_uuids = g_slist_append(dev->gatt_uuids,
>>>> +                                                       g_strdup(l->data));
>>>>
>>>>         if (dev->name[0] == '\0')
>>>>                 strcpy(dev->name, dup->name);
>>>> @@ -3240,9 +3335,14 @@ static gboolean record_has_uuid(const sdp_record_t *rec,
>>>>         return FALSE;
>>>>  }
>>>>
>>>> -GSList *btd_device_get_uuids(struct btd_device *device)
>>>> +GSList *btd_device_get_sdp_uuids(struct btd_device *device)
>>>>  {
>>>> -       return device->uuids;
>>>> +       return device->sdp_uuids;
>>>> +}
>>>> +
>>>> +GSList *btd_device_get_gatt_uuids(struct btd_device *device)
>>>> +{
>>>> +       return device->gatt_uuids;
>>>>  }
>>>>
>>>>  struct probe_data {
>>>> @@ -3280,7 +3380,8 @@ void device_probe_profile(gpointer a, gpointer b)
>>>>         if (profile->device_probe == NULL)
>>>>                 return;
>>>>
>>>> -       if (!device_match_profile(device, profile, device->uuids))
>>>> +       if (!device_match_profile(device, profile, device->sdp_uuids) &&
>>>> +               !device_match_profile(device, profile, device->gatt_uuids))
>>>>                 return;
>>>>
>>>>         service = service_create(device, profile);
>>>> @@ -3318,7 +3419,8 @@ void device_remove_profile(gpointer a, gpointer b)
>>>>         service_remove(service);
>>>>  }
>>>>
>>>> -void device_probe_profiles(struct btd_device *device, GSList *uuids)
>>>> +void device_probe_profiles(struct btd_device *device, GSList *uuids,
>>>> +                                                               bool gatt)
>>>>  {
>>>>         struct probe_data d = { device, uuids };
>>>>         char addr[18];
>>>> @@ -3335,7 +3437,10 @@ void device_probe_profiles(struct btd_device *device, GSList *uuids)
>>>>         btd_profile_foreach(dev_probe, &d);
>>>>
>>>>  add_uuids:
>>>> -       device_add_uuids(device, uuids);
>>>> +       if (gatt)
>>>> +               device_add_gatt_uuids(device, uuids);
>>>> +       else
>>>> +               device_add_sdp_uuids(device, uuids);
>>>>  }
>>>>
>>>>  static void store_sdp_record(GKeyFile *key_file, sdp_record_t *rec)
>>>> @@ -3431,7 +3536,7 @@ static int update_record(struct browse_req *req, const char *uuid,
>>>>         req->records = sdp_list_append(req->records, sdp_copy_record(rec));
>>>>
>>>>         /* Check if UUID is duplicated */
>>>> -       l = g_slist_find_custom(req->device->uuids, uuid, bt_uuid_strcmp);
>>>> +       l = g_slist_find_custom(req->device->sdp_uuids, uuid, bt_uuid_strcmp);
>>>>         if (l == NULL) {
>>>>                 l = g_slist_find_custom(req->profiles_added, uuid,
>>>>                                                         bt_uuid_strcmp);
>>>> @@ -3662,7 +3767,7 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
>>>>          * the full list of services and populate a client-role gatt_db over
>>>>          * BR/EDR.
>>>>          */
>>>> -       device_probe_profiles(device, req->profiles_added);
>>>> +       device_probe_profiles(device, req->profiles_added, false);
>>>>
>>>>         /* Propagate services changes */
>>>>         g_dbus_emit_property_changed(dbus_conn, req->device->path,
>>>> @@ -3927,6 +4032,8 @@ static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
>>>>
>>>>         if (device->browse)
>>>>                 register_gatt_services(device->browse);
>>>> +       else
>>>> +               device_probe_gatt_profiles(device);
>>>>
>>>>         device_accept_gatt_profiles(device);
>>>>
>>>> @@ -5127,13 +5234,13 @@ void btd_device_add_uuid(struct btd_device *device, const char *uuid)
>>>>         GSList *uuid_list;
>>>>         char *new_uuid;
>>>>
>>>> -       if (g_slist_find_custom(device->uuids, uuid, bt_uuid_strcmp))
>>>> +       if (g_slist_find_custom(device->sdp_uuids, uuid, bt_uuid_strcmp))
>>>>                 return;
>>>>
>>>>         new_uuid = g_strdup(uuid);
>>>>         uuid_list = g_slist_append(NULL, new_uuid);
>>>>
>>>> -       device_probe_profiles(device, uuid_list);
>>>> +       device_probe_profiles(device, uuid_list, false);
>>>>
>>>>         g_free(new_uuid);
>>>>         g_slist_free(uuid_list);
>>>> diff --git a/src/device.h b/src/device.h
>>>> index a7fefee..f250bfa 100644
>>>> --- a/src/device.h
>>>> +++ b/src/device.h
>>>> @@ -60,8 +60,10 @@ struct device_addr_type {
>>>>  };
>>>>
>>>>  int device_addr_type_cmp(gconstpointer a, gconstpointer b);
>>>> -GSList *btd_device_get_uuids(struct btd_device *device);
>>>> -void device_probe_profiles(struct btd_device *device, GSList *profiles);
>>>> +GSList *btd_device_get_sdp_uuids(struct btd_device *device);
>>>> +GSList *btd_device_get_gatt_uuids(struct btd_device *device);
>>>> +void device_probe_profiles(struct btd_device *device, GSList *profiles,
>>>> +                                                               bool gatt);
>>>
>>> Id leave this API as it is for now and internally classify the UUIDs,
>>> the classification function can be used when loading from the storage
>>> that way we don't need to break the storage format.
>>>
>>
>> Well, I'm just not sure how to do that classification. Especially in
>> the case of device_probe_profiles, which gets passed in a list of
>> UUIDs and the case where the UUIDs get loaded from storage, I don't
>> know how we can figure out which ones are LE and which ones are
>> BR/EDR, unless we do something complicated/inefficient like going
>> through the gatt-db and see if there's a matching service for each
>> UUID but this still becomes a problem when we load the UUIDs before
>> having done discovery (i.e. there's no populated gatt-db).

Yep, there is also the case of vendor UUIDs which we cannot classify
since they could appear in either SDP or GATT discovery, since this is
affecting only the removal of services perhaps it is better to not
remove anything for now.

>>>>  const sdp_record_t *btd_device_get_record(struct btd_device *device,
>>>>                                                 const char *uuid);
>>>>  struct gatt_primary *btd_device_get_primary(struct btd_device *device,
>>>> --
>>>> 2.2.0.rc0.207.ga3a616c
>>>>
>>>> --
>>>> 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  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>> Thanks,
>> Arman
>
> ping



-- 
Luiz Augusto von Dentz

[PATCH BlueZ v3 4/8] profiles/gap: Fix probe/accept behavior.

[Arman Uguray]

This patch fixes the GAP profile so that it assumes that there will be
one btd_service instance per-device rather than per-service-per-device.
---
 profiles/gap/gas.c | 127 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 71 insertions(+), 56 deletions(-)

diff --git a/profiles/gap/gas.c b/profiles/gap/gas.c
index 01b6ec2..7d81592 100644
--- a/profiles/gap/gas.c
+++ b/profiles/gap/gas.c
@@ -43,21 +43,25 @@
 #include "src/service.h"
 #include "src/log.h"
 
+#define GAP_UUID16 0x1800
+
 /* Generic Attribute/Access Service */
 struct gas {
 	struct btd_device *device;
 	struct gatt_db *db;
+	unsigned int db_id;
 	struct bt_gatt_client *client;
-	uint16_t start_handle, end_handle;
+	struct gatt_db_attribute *attr;
 };
 
 static GSList *devices;
 
 static void gas_free(struct gas *gas)
 {
-	btd_device_unref(gas->device);
+	gatt_db_unregister(gas->db, gas->db_id);
 	gatt_db_unref(gas->db);
 	bt_gatt_client_unref(gas->client);
+	btd_device_unref(gas->device);
 	g_free(gas);
 }
 
@@ -183,48 +187,31 @@ static void handle_characteristic(struct gatt_db_attribute *attr,
 
 static void handle_gap_service(struct gas *gas)
 {
-	struct gatt_db_attribute *attr;
-
-	attr = gatt_db_get_attribute(gas->db, gas->start_handle);
-	if (!attr) {
-		error("Service with handle 0x%04x not found in db",
-							gas->start_handle);
-		return;
-	}
-
-	gatt_db_service_foreach_char(attr, handle_characteristic, gas);
+	gatt_db_service_foreach_char(gas->attr, handle_characteristic, gas);
 }
 
 static int gap_driver_probe(struct btd_service *service)
 {
 	struct btd_device *device = btd_service_get_device(service);
 	struct gas *gas;
-	uint16_t start_handle, end_handle;
 	GSList *l;
 	char addr[18];
 
-	if (!btd_service_get_gatt_handles(service, &start_handle, &end_handle))
-		return -1;
-
 	ba2str(device_get_address(device), addr);
-	DBG("GAP profile probe (%s): start: 0x%04x, end 0x%04x", addr,
-						start_handle, end_handle);
+	DBG("GAP profile probe (%s)", addr);
 
-	/*
-	 * There can't be more than one instance of the GAP service on the same
-	 * device.
-	 */
+	/* Ignore, if we were probed for this device already */
 	l = g_slist_find_custom(devices, device, cmp_device);
 	if (l) {
-		error("More than one GAP service exists on device");
+		error("Profile probed twice for the same device!");
 		return -1;
 	}
 
 	gas = g_new0(struct gas, 1);
+	if (!gas)
+		return -1;
 
 	gas->device = btd_device_ref(device);
-	gas->start_handle = start_handle;
-	gas->end_handle = end_handle;
 	devices = g_slist_append(devices, gas);
 
 	return 0;
@@ -234,19 +221,11 @@ static void gap_driver_remove(struct btd_service *service)
 {
 	struct btd_device *device = btd_service_get_device(service);
 	struct gas *gas;
-	uint16_t start_handle, end_handle;
 	GSList *l;
 	char addr[18];
 
-	if (!btd_service_get_gatt_handles(service, &start_handle,
-								&end_handle)) {
-		error("Removed service is not a GATT service");
-		return;
-	}
-
 	ba2str(device_get_address(device), addr);
-	DBG("GAP profile remove (%s): start: 0x%04x, end 0x%04x", addr,
-						start_handle, end_handle);
+	DBG("GAP profile remove (%s)", addr);
 
 	l = g_slist_find_custom(devices, device, cmp_device);
 	if (!l) {
@@ -256,14 +235,58 @@ static void gap_driver_remove(struct btd_service *service)
 
 	gas = l->data;
 
-	if (gas->start_handle != start_handle ||
-						gas->end_handle != end_handle) {
-		error("Removed unknown GAP service");
+	devices = g_slist_remove(devices, gas);
+	gas_free(gas);
+}
+
+static void foreach_gap_service(struct gatt_db_attribute *attr, void *user_data)
+{
+	struct gas *gas = user_data;
+
+	if (gas->attr) {
+		error("More than one GAP service exists for this device");
 		return;
 	}
 
-	devices = g_slist_remove(devices, gas);
-	gas_free(gas);
+	gas->attr = attr;
+	handle_gap_service(gas);
+}
+
+static void service_added(struct gatt_db_attribute *attr, void *user_data)
+{
+	struct gas *gas = user_data;
+	bt_uuid_t uuid, gap_uuid;
+
+	if (!bt_gatt_client_is_ready(gas->client))
+		return;
+
+	gatt_db_attribute_get_service_uuid(attr, &uuid);
+	bt_uuid16_create(&gap_uuid, GAP_UUID16);
+
+	if (bt_uuid_cmp(&uuid, &gap_uuid))
+		return;
+
+	if (gas->attr) {
+		error("More than one GAP service added to device");
+		return;
+	}
+
+	DBG("GAP service added");
+
+	gas->attr = attr;
+	handle_gap_service(gas);
+}
+
+static void service_removed(struct gatt_db_attribute *attr, void *user_data)
+{
+	struct gas *gas = user_data;
+
+	if (gas->attr != attr)
+		return;
+
+	DBG("GAP service removed");
+
+	gas->attr = NULL;
 }
 
 static int gap_driver_accept(struct btd_service *service)
@@ -272,19 +295,12 @@ static int gap_driver_accept(struct btd_service *service)
 	struct gatt_db *db = btd_device_get_gatt_db(device);
 	struct bt_gatt_client *client = btd_device_get_gatt_client(device);
 	struct gas *gas;
-	uint16_t start_handle, end_handle;
 	GSList *l;
 	char addr[18];
-
-	if (!btd_service_get_gatt_handles(service, &start_handle,
-								&end_handle)) {
-		error("Service is not a GATT service");
-		return -1;
-	}
+	bt_uuid_t gap_uuid;
 
 	ba2str(device_get_address(device), addr);
-	DBG("GAP profile accept (%s): start: 0x%04x, end 0x%04x", addr,
-						start_handle, end_handle);
+	DBG("GAP profile accept (%s)", addr);
 
 	l = g_slist_find_custom(devices, device, cmp_device);
 	if (!l) {
@@ -294,21 +310,20 @@ static int gap_driver_accept(struct btd_service *service)
 
 	gas = l->data;
 
-	if (gas->start_handle != start_handle ||
-						gas->end_handle != end_handle) {
-		error("Accepting unknown GAP service");
-		return -1;
-	}
-
 	/* Clean-up any old client/db and acquire the new ones */
+	gas->attr = NULL;
+	gatt_db_unregister(gas->db, gas->db_id);
 	gatt_db_unref(gas->db);
 	bt_gatt_client_unref(gas->client);
 
 	gas->db = gatt_db_ref(db);
 	gas->client = bt_gatt_client_ref(client);
+	gas->db_id = gatt_db_register(db, service_added, service_removed, gas,
+									NULL);
 
-	/* Handle the service */
-	handle_gap_service(gas);
+	/* Handle the GAP services */
+	bt_uuid16_create(&gap_uuid, GAP_UUID16);
+	gatt_db_foreach_service(db, &gap_uuid, foreach_gap_service, gas);
 
 	return 0;
 }
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ v3 5/8] core: service: Remove GATT handle logic

[Arman Uguray]

This patch reverts the recent addition of GATT service handles to
btd_service.
---
 src/service.c | 38 --------------------------------------
 src/service.h |  7 -------
 2 files changed, 45 deletions(-)

diff --git a/src/service.c b/src/service.c
index 2ea2b7a..b66b1c9 100644
--- a/src/service.c
+++ b/src/service.c
@@ -53,8 +53,6 @@ struct btd_service {
 	void			*user_data;
 	btd_service_state_t	state;
 	int			err;
-	uint16_t		start_handle;
-	uint16_t		end_handle;
 };
 
 struct service_state_callback {
@@ -151,26 +149,6 @@ struct btd_service *service_create(struct btd_device *device,
 	return service;
 }
 
-struct btd_service *service_create_gatt(struct btd_device *device,
-						struct btd_profile *profile,
-						uint16_t start_handle,
-						uint16_t end_handle)
-{
-	struct btd_service *service;
-
-	if (!start_handle || !end_handle || start_handle > end_handle)
-		return NULL;
-
-	service = service_create(device, profile);
-	if (!service)
-		return NULL;
-
-	service->start_handle = start_handle;
-	service->end_handle = end_handle;
-
-	return service;
-}
-
 int service_probe(struct btd_service *service)
 {
 	char addr[18];
@@ -322,22 +300,6 @@ int btd_service_get_error(const struct btd_service *service)
 	return service->err;
 }
 
-bool btd_service_get_gatt_handles(const struct btd_service *service,
-							uint16_t *start_handle,
-							uint16_t *end_handle)
-{
-	if (!service || !service->start_handle || !service->end_handle)
-		return false;
-
-	if (start_handle)
-		*start_handle = service->start_handle;
-
-	if (end_handle)
-		*end_handle = service->end_handle;
-
-	return true;
-}
-
 unsigned int btd_service_add_state_cb(btd_service_state_cb cb, void *user_data)
 {
 	struct service_state_callback *state_cb;
diff --git a/src/service.h b/src/service.h
index 3a0db6e..c1f97f6 100644
--- a/src/service.h
+++ b/src/service.h
@@ -44,10 +44,6 @@ void btd_service_unref(struct btd_service *service);
 /* Service management functions used by the core */
 struct btd_service *service_create(struct btd_device *device,
 						struct btd_profile *profile);
-struct btd_service *service_create_gatt(struct btd_device *device,
-						struct btd_profile *profile,
-						uint16_t start_handle,
-						uint16_t end_handle);
 
 int service_probe(struct btd_service *service);
 void service_remove(struct btd_service *service);
@@ -63,9 +59,6 @@ struct btd_device *btd_service_get_device(const struct btd_service *service);
 struct btd_profile *btd_service_get_profile(const struct btd_service *service);
 btd_service_state_t btd_service_get_state(const struct btd_service *service);
 int btd_service_get_error(const struct btd_service *service);
-bool btd_service_get_gatt_handles(const struct btd_service *service,
-							uint16_t *start_handle,
-							uint16_t *end_handle);
 
 unsigned int btd_service_add_state_cb(btd_service_state_cb cb,
 							void *user_data);
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ v3 6/8] shared/gatt-db: Add "claimed" field to services

[Arman Uguray]

Added the ability to mark a service as "claimed". This is distinct from
"active", which denotes whether this service should be accessed at all
and is tied to the service added/removed events.
---
 src/shared/gatt-db.c | 20 ++++++++++++++++++++
 src/shared/gatt-db.h |  4 ++++
 2 files changed, 24 insertions(+)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index e8fb52c..a1b3ffd 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -102,6 +102,7 @@ struct gatt_db_attribute {
 struct gatt_db_service {
 	struct gatt_db *db;
 	bool active;
+	bool claimed;
 	uint16_t num_handles;
 	struct gatt_db_attribute **attributes;
 };
@@ -786,6 +787,25 @@ bool gatt_db_service_get_active(struct gatt_db_attribute *attrib)
 	return attrib->service->active;
 }
 
+bool gatt_db_service_set_claimed(struct gatt_db_attribute *attrib,
+								bool claimed)
+{
+	if (!attrib)
+		return false;
+
+	attrib->service->claimed = claimed;
+
+	return true;
+}
+
+bool gatt_db_service_get_claimed(struct gatt_db_attribute *attrib)
+{
+	if (!attrib)
+		return false;
+
+	return attrib->service->claimed;
+}
+
 void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
 							uint16_t end_handle,
 							const bt_uuid_t type,
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index dc1b819..a47882b 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -83,6 +83,10 @@ gatt_db_service_add_included(struct gatt_db_attribute *attrib,
 bool gatt_db_service_set_active(struct gatt_db_attribute *attrib, bool active);
 bool gatt_db_service_get_active(struct gatt_db_attribute *attrib);
 
+bool gatt_db_service_set_claimed(struct gatt_db_attribute *attrib,
+								bool claimed);
+bool gatt_db_service_get_claimed(struct gatt_db_attribute *attrib);
+
 typedef void (*gatt_db_attribute_cb_t)(struct gatt_db_attribute *attrib,
 							void *user_data);
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ v3 7/8] core: gatt: Use "claimed" instead of "active"

[Arman Uguray]

Using the "active" state of a gatt-db service to mark a service as
claimed by a profile somewhat abuses the API and has the additional
side-effect of sending a service_removed event. This causes errors in
the way btd_device manages profile/service lifetime. To fix this, this
patch uses the new "claimed" state to mark a service as claimed.

Also included is a minor fix to btd_gatt_client_* APIs that early return
if the client is not yet ready.
---
 src/device.c      | 7 +++++--
 src/gatt-client.c | 9 ++++++---
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/device.c b/src/device.c
index 9d1a480..1253986 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2566,7 +2566,7 @@ static void dev_probe_gatt(struct btd_profile *p, void *user_data)
 	}
 
 	/* Mark service as claimed */
-	gatt_db_service_set_active(data->cur_attr, false);
+	gatt_db_service_set_claimed(data->cur_attr, true);
 
 	data->dev->services = g_slist_append(data->dev->services, service);
 }
@@ -2603,8 +2603,11 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
 	}
 
 	/* Don't probe the profiles if a matching service already exists. */
-	if (find_service_with_uuid(data->dev->services, data->cur_uuid))
+	if (find_service_with_uuid(data->dev->services, data->cur_uuid)) {
+		/* Mark the service as claimed by the existing profile. */
+		gatt_db_service_set_claimed(data->cur_attr, true);
 		return;
+	}
 
 	btd_profile_foreach(dev_probe_gatt, data);
 
diff --git a/src/gatt-client.c b/src/gatt-client.c
index a12c656..cb8ddf6 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -52,6 +52,7 @@
 
 struct btd_gatt_client {
 	struct btd_device *device;
+	bool ready;
 	char devaddr[18];
 	struct gatt_db *db;
 	struct bt_gatt_client *gatt;
@@ -1373,7 +1374,7 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
 	struct btd_gatt_client *client = user_data;
 	struct service *service;
 
-	if (!gatt_db_service_get_active(attr))
+	if (gatt_db_service_get_claimed(attr))
 		return;
 
 	service = service_create(attr, client);
@@ -1461,13 +1462,15 @@ void btd_gatt_client_ready(struct btd_gatt_client *client)
 	bt_gatt_client_unref(client->gatt);
 	client->gatt = bt_gatt_client_ref(gatt);
 
+	client->ready = true;
+
 	create_services(client);
 }
 
 void btd_gatt_client_service_added(struct btd_gatt_client *client,
 					struct gatt_db_attribute *attrib)
 {
-	if (!client)
+	if (!client || !attrib || !client->ready)
 		return;
 
 	export_service(attrib, client);
@@ -1486,7 +1489,7 @@ void btd_gatt_client_service_removed(struct btd_gatt_client *client,
 {
 	uint16_t start_handle, end_handle;
 
-	if (!client || !attrib)
+	if (!client || !attrib || !client->ready)
 		return;
 
 	gatt_db_attribute_get_service_handles(attrib, &start_handle,
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ v3 8/8] doc/gatt-api.txt: Update error names

[Arman Uguray]

Updated possible error names that can be returned from
ReadValue/WriteValue methods to match those currently returned from the
experimental implementation.
---
 doc/gatt-api.txt | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index f7125a2..bfeaf6d 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -75,9 +75,8 @@ Methods		array{byte} ReadValue()
 
 			Possible Errors: org.bluez.Error.Failed
 					 org.bluez.Error.InProgress
-					 org.bluez.Error.ReadNotPermitted
+					 org.bluez.Error.NotPermitted
 					 org.bluez.Error.NotAuthorized
-					 org.bluez.Error.NotPaired
 					 org.bluez.Error.NotSupported
 
 		void WriteValue(array{byte} value)
@@ -87,10 +86,9 @@ Methods		array{byte} ReadValue()
 
 			Possible Errors: org.bluez.Error.Failed
 					 org.bluez.Error.InProgress
-					 org.bluez.Error.WriteNotPermitted
+					 org.bluez.Error.NotPermitted
 					 org.bluez.Error.InvalidValueLength
 					 org.bluez.Error.NotAuthorized
-					 org.bluez.Error.NotPaired
 					 org.bluez.Error.NotSupported
 
 		void StartNotify()
@@ -175,9 +173,8 @@ Methods		array{byte} ReadValue()
 
 			Possible Errors: org.bluez.Error.Failed
 					 org.bluez.Error.InProgress
-					 org.bluez.Error.ReadNotPermitted
+					 org.bluez.Error.NotPermitted
 					 org.bluez.Error.NotAuthorized
-					 org.bluez.Error.NotPaired
 					 org.bluez.Error.NotSupported
 
 		void WriteValue(array{byte} value)
@@ -187,10 +184,9 @@ Methods		array{byte} ReadValue()
 
 			Possible Errors: org.bluez.Error.Failed
 					 org.bluez.Error.InProgress
-					 org.bluez.Error.WriteNotPermitted
+					 org.bluez.Error.NotPermitted
 					 org.bluez.Error.InvalidValueLength
 					 org.bluez.Error.NotAuthorized
-					 org.bluez.Error.NotPaired
 					 org.bluez.Error.NotSupported
 
 Properties	string UUID [read-only]
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH BlueZ v3 0/8] Implement doc/gatt-api.txt for client

[Luiz Augusto von Dentz]

Hi Arman,

On Wed, Jan 14, 2015 at 5:30 AM, Arman Uguray <armansito@chromium.org> wrote:
> *v3:
>   - Fixed broken GATT service UUID management in its own patch.
>   - Other small fixes.
>
> Arman Uguray (8):
>   shared/gatt-db: Add service getter by UUID
>   core: device: Fix GATT profile probing
>   core: device: Fix broken GATT UUID management
>   profiles/gap: Fix probe/accept behavior.
>   core: service: Remove GATT handle logic
>   shared/gatt-db: Add "claimed" field to services
>   core: gatt: Use "claimed" instead of "active"
>   doc/gatt-api.txt: Update error names
>
>  doc/gatt-api.txt     |  12 +-
>  plugins/sixaxis.c    |   2 +-
>  profiles/gap/gas.c   | 127 +++++++------
>  src/adapter.c        |   8 +-
>  src/device.c         | 498 +++++++++++++++++++++++++++++----------------------
>  src/device.h         |   6 +-
>  src/gatt-client.c    |   9 +-
>  src/service.c        |  38 ----
>  src/service.h        |   7 -
>  src/shared/gatt-db.c |  46 +++++
>  src/shared/gatt-db.h |   7 +
>  11 files changed, 428 insertions(+), 332 deletions(-)
>
> --
> 2.2.0.rc0.207.ga3a616c

Applied, note that I did exclude core: device: Fix broken GATT UUID
management and instead remove the code that deal with invalidating the
services, we have similar behavior for SDP. I found a problem with the
current code it does not exclude services properly once I restart
bluetoothd, most likely because we don't reload the services and once
we reconnect they are not updated since register_gatt_services is only
called if device->browse which is not the case while reconnecting.

Furthermore I would do some cleanup in the way the service are
registered, maybe move some code to src/gatt-client.c and Im also
thinking in creating the D-Bus objects as they are found so we don't
have different code paths creating them.


-- 
Luiz Augusto von Dentz